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

NEW
Unassigned

Status

()

P3
normal
2 years ago
a year ago

People

(Reporter: bholley, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

2 years ago
I'll attach an IRC log for posterity.
(Reporter)

Comment 1

2 years ago
Created attachment 8842293 [details]
debugginglog
(Reporter)

Comment 2

2 years ago
NI dbaron to advise on next steps.
Flags: needinfo?(dbaron)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Comment 6

2 years ago
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.
Created attachment 8843106 [details]
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.
Created attachment 8843113 [details]
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.
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.
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.
(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?
Created attachment 8843152 [details] [diff] [review]
bholley's lazy frame construction patch

For the record, attaching the patches being discussed.
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.
(Reporter)

Comment 15

2 years ago
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.
No longer blocks: 1338921
Flags: needinfo?(dbaron)
(Reporter)

Updated

2 years ago
Summary: stylo: incremental reflow fails to uncollapse margins after unifying lazy frame construction with restyle → incremental reflow fails to uncollapse margins after unifying lazy frame construction with restyle
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.