Closed Bug 1487537 Opened 6 years ago Closed 6 years ago

nsHTMLScrollFrame::ReflowContents reflows contents more times than necessary


(Core :: Layout: Scrolling and Overflow, defect)

Not set





(Reporter: bradwerth, Assigned: bradwerth)


(Blocks 1 open bug)



(3 files, 2 obsolete files)

nsHTMLScrollFrame::ReflowContents does an initial reflow of its contents, then makes successive calls to ::TryLayout to determine if additional reflow is necessary due to scrollbars. Those calls follow a blind heuristic that will reflow contents 5 times in a worst-case scenario. By changing the heuristic to derive more information from each call to TryLayout, the worst case scenario can be reduced to 3 reflows, without worsening the behavior in other cases.

1) Initial reflow with no scrollbars
2) Reflow with horizontal scrollbar on
3) Reflow with vertical scrollbar on
4) Reflow with both scrollbars on
5) Forced reflow with scrollbars except for overflow-{x,y}:hidden

1) Initial reflow with no scrollbars
2) Reflow with scrollbars matching what was noted as necessary after reflow 1
3) Forced reflow with scrollbars except for overflow-{x,y}:hidden
This changes nsHTMLScrollFrame::TryLayout to use outparams for the
requested hscroll and vscroll settings, so that proposed fixes can
be signaled to the caller. ::ReflowContents uses these outparams
to reduce the number of calls to ::TryLayout from 5 down to 3.
Attachment #9005674 - Attachment is obsolete: true
Attached file breaktest 1
Here's a testcase that this patch currently breaks, I think.

In unmodified mozilla-central, this testcase shows a horizontal scrollbar on the box here (which is useful/necessary to scroll to allow the user to see the img element's rightmost edge -- its right black border).

If I apply your patch, that scrollbar doesn't show up anymore -- though some of the inner box *does* still overflow (off the right side). So we end up clipping without providing any scrolling mechanism for the overflowing stuff, which is undesirable.

So, that seems to be a regression that this patch would cause, in its current form.
Flags: needinfo?(bwerth)
Note / thinking out loud: I am not entirely sure that it's possible to reliably do better than what our code currently does, without regressing some scenario, particularly for cases like the attached "breaktest 1" where the containing-block size in one dimension affects the desired size in the other dimension.

I can imagine hammering out some optimizations to skip some of the 5 scrollbar-configuration-reflows in special cases, e.g. if we can prove that our content won't change its desired size when we adjust the containing block's block-size.  (This is typically the case for CSS blocks.)  But those special cases might be effectively optimized away already, because e.g. changing the BSize of a block is a pretty cheap reflow (since it doesn't dirty the children) in most cases.

i.e. to the extent that we can carve out special cases to optimize here, I wouldn't be surprised if those special cases are already somewhat optimized.
I'm beginning to agree. The calling pattern in the testcase doesn't give a guess that leads to the correct output, so no number of iterations of repeated guessing will work. Our existing code which tries all combinations in some order does work.

Since this whole issue was raised by me and the opportunity I saw was entirely speculative, I'm comfortable closing this issue as INVALID.
Closed: 6 years ago
Flags: needinfo?(bwerth)
Resolution: --- → INVALID
Sounds good to me.

I might land a version of the attached patch as a reftest (probably with "overflow-y:hidden; overflow-x: scroll" in the reference case), since it sounds like we didn't have a reftest that caught the latest patch's behavior-change yet and it'd be nice to have that codepath/scenario covered.
> I might land a version of the attached patch as a reftest 

sorry, s/attached patch/attached breaktest/
Flags: needinfo?(dholbert)
Brad, mind reviewing this patch with a reftest (per comment 9)?

It's got three rows of examples, and the examples only differ in their scrollframe's width.

The second row is the "interesting" one -- it's the one where there's the weird paradoxical horiz-scrollbar-is-needed-but-makes-itself-unnecessary situation.

I'm running this through Try now -- I anticipate that it might need a slight tweak to pass on platforms with overlay scrollbars, but I'm not sure:
Attachment #9011918 - Flags: review?(bwerth)
(For the record: I tried applying Brad's latest code-patch from Phabricator and running this reftest, to be sure that this test fails, and it does -- here's the reftest log snippet from that test-failure.)
Attachment #9011920 - Attachment description: reftest failure log, showing how this reftest fails with the latest patch here → reftest failure log, showing how this reftest fails with the latest phabricator patch
Flags: needinfo?(dholbert)
Comment on attachment 9011918 [details] [diff] [review]
reftest patch (v1)

Review of attachment 9011918 [details] [diff] [review]:

Thanks for finding the case that showed the problem with my proposed "optimization" of the scrollable container layout code.
Attachment #9011918 - Flags: review?(bwerth) → review+
Sure! Thanks for the review.

BTW, I think you want to mark your phabricator page here ( ) as "abandoned" so that it's not left in limbo.

I believe "Abandon" should appear as one of the actions in your "Add Action..." menu at the bottom of the page (above the comment box), though I'm not 100% sure.
Attachment #9005460 - Attachment is obsolete: true
Looks like my reftest passes everywhere except for Android, where it fails reliably.

The failure is in the "interesting" second row of the test -- it looks like Android shows a wider horizontal scrollbar [i.e. it thinks there's less overflow] in the testcase (which has overflow:auto) vs. the reference case (which has overflow-x: scroll).

This might be a subtle bug in Android scrollbars (or in overlay scrollbars in general) -- I'll spin off a helper bug and mark the test as fails-if(Android) with a note about that bug.
Pushed by
Add reftest to exercise a scenario where presence of scrollbars influences the need for scrollbars. r=bradwerth
Note: I'm exploring some other possible scrollbar-toggling changes in bug 1503660, though I'm not 100% sure it'll go anywhere.
See Also: → 1503660
You need to log in before you can comment on or make changes to this bug.