Including margins that collapse through nested and self-collapsing children.
Determine block-end adjacency after line reflow and fix up the affected
used margins, positions, overflow and collapsed-margin state.
Details
Details
- Reviewers
emilio dshin - Group Reviewers
layout-reviewers firefox-style-system-reviewers - Bugzilla Bug ID
- 2051240
- Merge Conflict Status
- Landing may fail: this does not merge cleanly into the target branchConflicts when merged against target branch tip c95c2b35c942, starting from 0b76543aaeee.Last checked Thu, Oct 15, 10:09 PMDiff 1364227, based on 0b76543aaeee
Diff Detail
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
- Branch
- HEAD
| Time | Test | |
|---|---|---|
| 0 ms | code-review::general WARNING: A generic error occurred in the code review bot. |
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Mostly questions... I'm a bit confused as to why do we need the prediction machinery and the retry stuff. It seems to me that we could apply margins unconditionally, then only if you don't find a non-empty line (and don't need to fragment etc) undo the block dir advance of the last margin(s), to "un-account" for those. Would that approach not work? Seems it would avoid needing retries / predictions / looking eagerly at lines to come up with an index / etc, but maybe I'm missing something?
| accessible/tests/mochitest/attributes/a11y.toml | ||
|---|---|---|
| 14 | Why is this needed? Are you sure this works? (At least a while ago prefs only worked in the [DEFAULT] section) | |
| layout/generic/BlockReflowState.h | ||
| 276 | I see, so the point of this is to get the line index that's adjacent to the block end first, then checking against here. But how does this work? Inline lines can't fragment so don't you get into trouble if you hit an inline line that then you have to break? Seems you'd end up with the wrong index, am I missing something? | |
| 434 | Could get some comment... | |
| layout/generic/nsBlockFrame.cpp | ||
| 2144 | This comment now needs moving probably. | |
| 2146 | Can you elaborate why we need to mark all the lines dirty eagerly? | |
| 2153 | Are we supposed to do more than one of these? Why do we need to retry at all? | |
| layout/generic/nsBlockFrame.h | ||
| 393 | This could get some documentation... It returns a line index, relative to mLines. But it also accounts for fragmentation etc... why? | |
| layout/generic/nsBlockReflowContext.cpp | ||
| 75 | This is only getting computed if there's no padding-block-start right? That seems... bogus at a glance? Why would a frame with a non-zero padding-start be unconditionally a block-end-margin-root/ | |
| accessible/tests/mochitest/attributes/a11y.toml | ||
|---|---|---|
| 14 | Moved to [DEFAULT]. | |
Comment Actions
Okay, it seems that the prediction stuff wasn't really needed after all. I was worried about the post reflow fix-ups, but it didn't turn out to be that complex. I removed it (as well as the retries) and its much simpler now. Thanks for the suggestion!
Comment Actions
Mostly coming to understanding, a few nits, but float overflow handling happening after scrolled content check + return in UpdateMarginTrimLineOverflow seems suspect?
| layout/generic/BlockReflowState.cpp | ||
|---|---|---|
| 69 | Question: This is referring to (aReflowInput.mFlags.mTrimBStartMargin && !mFlags.mIsBStartMarginRoot);, right? Maybe worth adding "block-start margins" | |
| 75 | Hm, how does IB-split affect margin collapsing? I guess collapsing through block elements in IB split sibling shouldn't happen, so this makes sense? | |
| layout/generic/BlockReflowState.h | ||
| 118 | Nit: I think this comment particularly makes more sense close to the call site? Or maybe this comment can be something like "Compute margin-trim related flags based on this frame's position within its parent?" | |
| layout/generic/nsBlockFrame.cpp | ||
| 80 | Nit: Maybe a really weak preference towards using MarginRootBlockSides = EnumSet<LogicalEdge>;. | |
| 4540 | Nit: This comment feels out of place, when in fact ComputeChildBStartMarginTrim propagates the trim flag through zero-sized child? | |
| 4973 | (So I guess this happens for descendants that set margin-trim) | |
| 5889 | (IsAdjacentWithBStart seems to only care about the coordinates being adjacent to block start) | |
| 5901 | Would this be useful here? | |
| 5917 | (Right, trimming is blocked by border/padding, same as margin collapsing, then) | |
| 5922 | So as far as I understand, once the line is trimmed, we update the in-flow bounds (Which includes the margin rect) | |
| 5945 | Shouldn't this happen even if we're not scrolled content (Line 5936)? | |
| 5957 | The loop shape is largely shared with functions around here, and are expected to be in sync, I wonder if it can be easily combined? | |
| 5979 | Right, this needs to loop through collapsing through empty block lines. | |
| 6038 | 🤔 I guess empty next-in-flows if the div was way bigger than number of lines? I guess it's harder (Impossible?) to cause something like that with prev-in-flows. | |
| 6104 | Worth referencing bug 2051245 I think | |
| layout/generic/nsBlockFrame.h | ||
| 390 | Nit: Maybe prefixing Style? As in StyleTrimsChildBStartMargins, etc? | |
| layout/generic/nsBlockFrame.cpp | ||
|---|---|---|
| 80 | Yeah, this is quite nice, thanks! | |
| 4540 | Comment now explains that block-start margins are collapsed before the child gets its own BlockReflowState so the right trimming is passed through ReflowInput. | |
| 5922 | Yes. Trimming changes the child margin box so the cached in-flow bounds are recomputed if scrolled. | |
| 5945 | As far as I understand, normal line overflow (including float overflow) is updated before the early return and only the special in-flow child bounds is limited to scrolled blocks? I updated the comment a bit, assuming my assumptions are correct. | |
| 6038 | So the forward scan ensures that the trailing margin is not treated as adjoining the overall block end when a later continuation still contains non-empty content. | |
| layout/generic/nsBlockFrame.h | ||
| 390 | What is the motivation? | |
| layout/generic/BlockReflowState.cpp | ||
|---|---|---|
| 75 | That's how I understand it as well. GetPrevInFlow() only prevents a later frag from being considered adjacent to block-start. block-container-block-in-inline-00[1..4].html tests these cases I believe. | |
| layout/generic/nsBlockFrame.cpp | ||
| 5957 | The loops update different things, but also they stop under slightly different conditions, so I don't think it will simplify. | |
| 5979 | Correct, empty block lines remain part of the adjoining margin set. | |
Comment Actions
I think the algorithm & its helpers are correct, but at least some of them can be merged? May be tricky, but I have concerns about procedures that seem mostly-the-same being done in different places, specifically for when re-computing line overflows & checking if the frame may be margin trimmed.
| layout/generic/nsBlockFrame.cpp | ||
|---|---|---|
| 5922 | Hmm, I think/hope then it's mergable with this? The in-flow bounds stuff is tricky as it is and it'd be great if we had very select places where we update them.. | |
| 5945 | True, yeah, SetInFlowChildBounds is in scrolled content context. | |
| 6038 | Ah, ok. | |
| layout/generic/nsBlockFrame.h | ||
| 390 | This elsewhere: mFlags.mTrimChildBStartMargins =
nsBlockFrame::TrimsChildBStartMargins(aFrame) ||
(aReflowInput.mFlags.mTrimBStartMargin && !mFlags.mIsBStartMarginRoot);Feels it reads a bit better as "If this frame's style specifies margin trim, or we're in a margin trimming context, set the flag." | |
| layout/style/nsComputedDOMStyle.cpp | ||
| 2100 | This parent traversal feels kinda unfortunate. OTOH I think/hope not a lot of people run gCS looking at margin in a tight loop? Maybe a use of frame property is granted? | |
Comment Actions
The overflow handling seems rather suspect, I'd rather not add a whole "recompute the whole overflow areas of the line out of band" without very good justification. Why don't we need to do that when doing margin collapsing? Can we use the same approach? If not, can't we reuse some of the other overflow handling somehow?
It seems like margin trimming is, conceptually, basically margin-collapsing without any previous margin (i.e., collapsing into the void), right... What are the differences that make us not able to reuse more of that mechanism?
The generational margin terminology really confused me, but that's of course not new... @jfkthame should probably double-check the interaction there.
I think the rest looks in good shape, but... Would it be possible to maybe split this into separate patches so that this can land incrementally?
Something like:
- Doing the plumbing through ReflowInput and such.
- Implementing the basic trimming.
- Fix any overflow / margin collapsing edge cases that need more work.
That would make it a bit easier to digest, but I guess no worries if it's too hard.
| layout/generic/nsBlockFrame.cpp | ||
|---|---|---|
| 3729 | Can we not use foundAnyClears, or the check right above? | |
| 3917 | Same above. I guess this is subtly different from LineHasClear, is that intentional? | |
| 4974 | What does "generational" margin here mean? I guess that terminology is used elsewhere. Does it just mean "parent-crossing" or so? | |
| 5924 | This is basically recomputing the line overflow from scratch, right? That doesn't seem like it should be called UpdateMarginTrimLineOverflow? What am I missing? In general... Can we avoid this? If not... How do we guarantee that UpdateOverflow etc remain in sync. | |
| 6125 | Can just be bool(marginTrim & BLOCK_END) which reads a lot nicer, same above. | |