1343449 - incremental reflow fails to uncollapse margins after unifying lazy frame construction with restyle

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Bobby Holley (:bholley)]

I'll attach an IRC log for posterity.

Comment 1

[Bobby Holley (:bholley)]

Attachment: debugginglog

Comment 2

[Bobby Holley (:bholley)]

NI dbaron to advise on next steps.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So I took a look at your restyling patch, may it be that we're _not_ creating some frames we should?

Particularly, your patch at bug 1338921 doesn't look at the flag in text nodes at all, which does happen (see for bug 1338678 for a minimized test that triggers lazy frame construction on text nodes). Presumably that would be broken with your patch.

Maybe fixing that helps? How does the frame tree look like before and after that patch for this test?

Of course doing that is not as easy as adding a ReconstructFrame hint, because we have assertions to prevent text nodes from posting change hints. Perhaps we could relax that assertion for that case.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> So I took a look at your restyling patch, may it be that we're _not_
> creating some frames we should?
> 
> Particularly, your patch at bug 1338921 doesn't look at the flag in text
> nodes at all, which does happen (see for bug 1338678 for a minimized test
> that triggers lazy frame construction on text nodes). Presumably that would
> be broken with your patch.

I think you're looking at an old WIP version of the patch (the one from the try push?). I'll upload a more up-to-date version in the bug.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> > So I took a look at your restyling patch, may it be that we're _not_
> > creating some frames we should?
> > 
> > Particularly, your patch at bug 1338921 doesn't look at the flag in text
> > nodes at all, which does happen (see for bug 1338678 for a minimized test
> > that triggers lazy frame construction on text nodes). Presumably that would
> > be broken with your patch.
> 
> I think you're looking at an old WIP version of the patch (the one from the
> try push?). I'll upload a more up-to-date version in the bug.

Right, thanks for that. I believe that patch is pretty much what I had in mind when writing the above comment.

Comment 6

[Bobby Holley (:bholley)]

Did a try push with my stylo changes as well as the 1-line-fix dbaron suggested for this bug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e147a0ab4401d970208d71a5a626972b6866495d

The regular linux64 test results there should tell us what (if any) layout tests break when we change that.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: a related testcase

So a little bit more data:  I tried debugging this similar testcase in a tree without Bobby's patch, to understand why we didn't have the bug without the patch.

The reason it works here is that maybeReflowingForFirstTime is true, so we call MarkPreviousMarginDirty on the <div id="two"> here:
https://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/layout/generic/nsBlockFrame.cpp#2392

It's not clear to me why that's not happening with Bobby's patch.  Though I suspect there might be more than one thing going on.  (I wonder if the first early-return in nsCSSFrameConstructor::ConstructFramesFromItem is somehow broken in stylo?)

(I think this test is basically the same as layout/reftests/margin-collapsing/dynamic-add-text-1.html except with the added overflow:hidden to make sure we're not seeing side-effects from the scrollbars.)


Next I'm going to try debugging a variant of the testcase with a float added, in the hopes that that will mean the line won't be new.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: a testcase that shows the bug without Bobby's patch

Actually, right after writing that, I realized that floats (which I'd tested but hadn't yet debugged) affect the dirtiness of lines, but abs pos elements don't.

That led me to the discovery that this testcase shows the bug without Bobby's patch.  In other words, in this testcase, after the one second timer, the layout will be incorrect (insufficient margin), but resizing the window will fix it.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So in stylo with Bobby's patch, the whitespace frame suppression is indeed broken, and the frame tree prior to the dynamic changes is different from what is in in Gecko, and there's a whitespace-only line in the middle of the block.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Stylo without Bobby's patch is fine, though.

So Bobby's patch is somehow breaking the optimization that suppresses construction of certain whitespace-only frames.  Breaking that optimization exposes this existing block bug in an additional testcase.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #10)
> So Bobby's patch is somehow breaking the optimization that suppresses
> construction of certain whitespace-only frames.  Breaking that optimization
> exposes this existing block bug in an additional testcase.

I suspect, although I haven't fully dug into it, that this is because:

 * the existing code in nsCSSFrameConstructor::CreateNeededFrames merges multiple consecutive nodes that need frames into a single ContentRangeInserted or ContentAppended call.  (In this case, ContentAppended.)

 * the new code doesn't do that, but instead does a separate ContentInserted (which translates to a ContentRangeInserted) for each frame

 * AtLineBoundary can't figure out that a whitespace node that's in a single FCItem on its own is at a line boundary; it can only determine this when the adjacent block items are present.  (It will never return true if aIter.AtStart() && aIter.item().mContent->GetPrevousSibling && (next = aIter, next.Next(), next.IsDone()) && aIter.item().mContent->GetNextSibling(), and I believe that's all true when processing the whitespace between blocks a a single insertion.)


Given how *much* we seem to process using CreateNeededFrames, perhaps it's important that we actually process ranges together here?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: bholley's lazy frame construction patch

For the record, attaching the patches being discussed.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to fix this case in block code

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also note that attachment 8843153 [details] [diff] [review] reverts https://hg.mozilla.org/mozilla-central/rev/e151141ff28b (bug 393655), and it causes:
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/bugs/393655-1.html == layout/reftests/bugs/393655-1-ref.html
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/bugs/393655-2.html == layout/reftests/bugs/393655-2-ref.html

So neither emptiness test seems correct, although I still need to look into what was happenig in those tests.

Comment 15

[Bobby Holley (:bholley)]

Coalescing siblings as dbaron suggested fixes the behavior difference for me with my stylo changes, so I'm no longer blocked on this. Sounds like there's still a bug here though.