1487537 - nsHTMLScrollFrame::ReflowContents reflows contents more times than necessary

Status: RESOLVED INVALID

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

Description

[Brad Werth [:bradwerth]]

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.

Currently:
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

Proposed:
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

Comment 1

[Brad Werth [:bradwerth]]

Attachment: Bug 1487537: Make nsHTMLScrollFrame::ReflowContents smarter about next steps when ::TryLayout fails. r=dholbert!

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.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1487537 Part 1: Rename parameters in nsHTMLScrollFrame::TryLayout. r=dholbert!

Comment 3

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a84b1fc533a90b1b40494bd4136cd46b7da2ed5

Comment 4

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18cca68db0f2005a3e8602bcbb642c5fcc0894af

Comment 5

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95782edf4204740e4548da6f03953fd2dc195e35

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Brad Werth [:bradwerth]]

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.

Comment 9

[Daniel Holbert [:dholbert]]

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.

Comment 10

[Daniel Holbert [:dholbert]]

> I might land a version of the attached patch as a reftest 

sorry, s/attached patch/attached breaktest/

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: reftest patch (v1)

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:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1196af83f651b5a4231c4e5d9fda3ab958429d60

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: reftest failure log, showing how this reftest fails with the latest phabricator patch

(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.)

Comment 13

[Brad Werth [:bradwerth]]

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.

Comment 14

[Daniel Holbert [:dholbert]]

Sure! Thanks for the review.

BTW, I think you want to mark your phabricator page here ( https://phabricator.services.mozilla.com/D4720 ) 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.

Comment 15

[Daniel Holbert [:dholbert]]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b79b0c12ac0
Add reftest to exercise a scenario where presence of scrollbars influences the need for scrollbars. r=bradwerth

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/7b79b0c12ac0

Comment 18

[Daniel Holbert [:dholbert]]

Note: I'm exploring some other possible scrollbar-toggling changes in bug 1503660, though I'm not 100% sure it'll go anywhere.