1503660 - Don't bother suppressing scrollbar when the scroll container has a height of 0

Status: RESOLVED DUPLICATE of bug 1365806

(Core :: Layout: Scrolling and Overflow, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

In bug 1376536, we get into trouble in part because we get a "measuring" reflow of an overflow-y:scroll area with a (temporary) computed height of 0.

And in that special case, we get up to some silly antics - we realize that we're supposed to show the scrollbar, but we also recognize that it won't fit (because we have a computed height of 0) -- so we try to reflow without the scrollbar to see if that makes us consistent, and because it might help some content be visible that the scrollbar would otherwise be covering up.

This is silly, because if we've got a 0-size area, hiding the scrollbar isn't going to free up any space, so it's kind of pointless to try that.  And it's expensive, too, because the act of hiding the scrollbar causes the containing-block width to change for the descendants, which means their height measurements 
become invalid and have to be recalculated even if they're not dirty.

So, I think we should handle this "0-sized scroll container" scenario with a special case, and we shouldn't bother disabling the scrollbar to try to make things fit in that case.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1503660 part 1: Add curly braces to some scrollbar-handling logic to match mozilla coding style. r?mats

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1503660 part 2: Don't treat scrollframe layout as inconsistent simply due to the scrollbar not fitting, if the scrollport is 0-sized. r?mats

This should prevent us from doing unnecessary reflows with scrollbars toggled
off (which can be expensive due to e.g. changing the available width for
content when we hide a veritcal scrollbar). In this case where the scrollbar is
requested but the scrollport is 0-sized, we can tell that the hiding the
scrollbar isn't going to give us any more space for content, so there's no
point.

Depends on D10423

Comment 3

[Daniel Holbert [:dholbert]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b20a0db91bb1fc58714744f4e5332cbac6c057

Comment 4

[Daniel Holbert [:dholbert]]

Hmm, apparently this is web-observable change... even in a height:0 element, web content can detect the presence of scrollbars by checking .clientWidth, which reports the inner width (which is a smaller value, if we don't hide the scrollbar).

This causes layout/xul/test/test_bug563416.html to fail, as shown in comment 3's Try run -- it's expecting clientWidth to be exactly the width of a 0-height scrollable area (a textarea in this case), even though there's vertical overflow which would normally cause a scrollbar.

i.e. it's expecting us to trigger the "scrollbar can't possibly fit vertically inside of our height, so let's hide it" fallback behavior that I was hoping to avoid here.

I was assuming that we'd make it to the "show all scrollbars that we can" throw-up-our-hands final TryLayout call, because I didn't think any previous layout would be considered consistent -- but apparently, current-Firefox stops at an earlier "turn off all scrollbars" TryLayout call before that one, because we judge the no-scrollbars layout as being consistent. (It *seems* like it'd be judged as inconsistent, due to the overflow property & the actual-overflowing-content, both of which prompt us to need a scrollbar -- but we hit the special case that sets " wantVScrollbar = false;" that my patch adjusts, and that special case makes us judge the scrollbarless scenario as being consistent.)

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase #1

It changes the outer size though...  I think it's a bit weird that
these two blocks have different height.

Comment 6

[Mats Palmgren (inactive)]

Oh, that's probably what you're explaining in comment 4 (I didn't see that before posting).

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Here's a more extensive testcase to compare with other UAs.

I kind of like the consistency in Chrome and Edge that they
render the scrollbars for all sizes.  Even though the scrollbar
might be to small to be clickable, at least it gives a hint
to the user that it's a scrollable block.

Safari is also consistent for all sizes in its own way.
Overlay scrollbars are hard to use at these small sizes
though, and less discoverable (as usual).

So perhaps we should consider removing the if-statements
that you modified?

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #7)
> I kind of like the consistency in Chrome and Edge that they
> render the scrollbars for all sizes.  Even though the scrollbar
> might be to small to be clickable, at least it gives a hint
> to the user that it's a scrollable block.

True! I like that as well.  That, combined with the perf hit of doing a "test run" without the scrollbar (with a skinnier width & hence potentially-invalidated layout on the descendant subtree), makes me lean towards this approach.

> So perhaps we should consider removing the if-statements
> that you modified?

I'll see what Try thinks.

Comment 9

[Daniel Holbert [:dholbert]]

Try run for the "if"-removal suggested in comment 7:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96a093bcb5a78e2b5a62a5a3b5630f6c1cef6412

It shows a failure in 416752-1.html (link: https://hg.mozilla.org/mozilla-central/raw-file/c291143e24019097d087f9307e59b49facaf90cb/layout/reftests/bugs/416752-1.html ).

That testcase has a auto-height scroll container wrapping a (short) fixed-height child, which has a taller (overflowing) thing inside of it. Its reference case is the same, but it puts the fixed height on the scroll container itself, instead of having it shrinkwrap the fixed-height child.  Seems like those should reasonably be expected to produce the same layout.

Also: I'm noticing that (at least on Ubuntu 18.10) we seem to have oddly large "minimum-height" values for vertical scrollbars. e.g. this renders with the scrollable area as 58px tall in Nightly, vs. 17px tall & wide in Chrome:
  data:text/html,<div style="overflow:scroll;float:left;border:1px solid black">

That could cause problems if we were to start proactively showing scrollbars in cases like testcase 2, because the scrollbars will "prop up" the auto-heights of those elements and make them extra-tall in Firefox.

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #3

What I don't understand is why this testcase now has a horizontal
scrollbar.  Clearly "XX" should fit inside 500px, so there's no
reason to create a scrollbar for it in that dimension.
The 'line-height: 1' creates some vertical overflow though,
same as in other UAs, but they correctly only create a vertical
scrollbar.  This seems like a regression that is unrelated to
the scrollbar minimum size (I think).
(This is with the later fix of just removing the if-statements.)

Comment 11

[Mats Palmgren (inactive)]

Hmm, this is tricky.  It seems that we correctly decide that
we need a vertical scrollbar due to the overflow, so we reflow
with the v-scrollbar enabled.  This makes the scrollport taller
because of the minimum scrollbar size, so now we decide that
the content doesn't overflow vertically after all.  This makes
our algorithm confused and we end up in the fallback:
https://searchfox.org/mozilla-central/rev/39cb1e96cf97713c444c5a0404d4f84627aee85d/layout/generic/nsGfxScrollFrame.cpp#791-797
that enables both scrollbars.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase #4

We don't include the scrollbar in the intrinsic size, unlike Chrome.
Edge renders it more like we do though.
Arguably this is a bug in Chrome, since figuring out that we'll
have a vertical scrollbar in this case requires a reflow.

Comment 13

[Mats Palmgren (inactive)]

Attachment: wip

This removes the if-statements discussed above, and fixes the "unnecessary
horizontal scrollbar" case discussed in comment 11 by making the scrollbar
"minimum size" zero in its scroll direction.  I think this fixes all
issues except the intrinsic size, which we perhaps should discuss with
Chrome / spec folks before deciding what to do.

WDYT?

Comment 14

[Mats Palmgren (inactive)]

BTW, the "wip" patch above makes this testcase have zero
content-box height and a visible horizontal scrollbar and
an invisible (zero height) vertical scrollbar:

data:text/html,<div style="overflow:scroll">

That's exactly how Chrome and Edge renders it too, so it
seems like the right layout for compat reasons.  Safari
also has zero height, but with no visible scrollbar at all
since it uses overlay scrollbars also for overflow:scroll
apparently.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 9022007 [details] [diff] [review]
wip

This seems to give pretty reasonable behavior, on the testcases posted here!

Comparing our resulting rendering (with this patch) vs. Chrome, the main rendering difference I'm noticing is the thing you brought up in comment 12, which I agree is a bug in Chrome.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 9022007 [details] [diff] [review]
wip

Review of attachment 9022007 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/nsNativeThemeGTK.cpp
@@ +1546,5 @@
>      aWidgetType = StyleAppearance::Menulist;
>    }
>  
> +  // NOTE for scrollbar parts: we only enforce a minimum breadth so that
> +  // the scrollbar can shrink to zero size in its scroll direction.

This needs s minor rephrasing -- right now, a reader can parse this with a different meaning than what you intend.  It can be read as saying "The only reason we enforce a minimum breadth is so that..." which ends up being confusing because it's wrong. :)

Maybe clarify/reword as: "we only enforce a minimum breadth.  We intentionally do not enforce a minimum length, so that [...]"

Comment 17

[Daniel Holbert [:dholbert]]

Feel free to reassign this bug to yourself & proceed with this approach if you like -- or I'm happy to take your patch and flesh it out, too.

(I think it just needs code for the other widget backends, plus maybe some tests, plus an update to the TryLayout documentation to remove "condition 3"[1], which I think corresponds to the if-checks that we're dropping here.)

[1] https://searchfox.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#340,345-347

Comment 18

[Mats Palmgren (inactive)]

> or I'm happy to take your patch and flesh it out, too.

Yes, please.  I was just curious why we created that unnecessary
horizontal scrollbar and had to debug it to understand the issue.

Comment 19

[Daniel Holbert [:dholbert]]

Sure, sounds good.

BTW, some unsurprising-but-still-good news, perf-wise:
I'm adding some logging in bug 1504361 to warn when we get a cache-miss on the flex item bsize measurement (that's the expensive/problematic thing that is caused by this bug's scrollbar-hiding-attempts, as described in comment 0). And the unsurprising/good news is that your attached 'wip' patch does make that warning stop appearing in my reduced versions of WhatsApp Web, which means it should indeed address bug 1376536.  (This is a result of the if-statement removals suggested in comment 7.)

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: Bug 1503660 part 1: Don't suppress scrollbars when their minimum length is longer than the scrollable area. r?mats

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: Bug 1503660 part 2: Reduce minimum size of scrollbar parts to 0, in the direction of the scrollbar.

Depends on D10800

Comment 22

[Mats Palmgren (inactive)]

There's actually a very good reason why we should always render the scrollbar
no matter how small it is.  When a <select> dropdown menu is in a very small
viewport we size it to fit inside by only showing a few <option>s and scrolling
the rest.  If there's only room for 1 or 2 options the scrollbar doesn't
render which makes it appear as if those are the only options.  (Rare edge
case perhaps, but still...)

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:dholbert, could you have a look please?

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #23)

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:dholbert, could you have a look please?

The current patches are partly strawman/investigative, and I don't think they're ready to land.

I was originally hoping the patches wouldn't have any detectable effect, but they do, and more work needs to be done to figure out if that's acceptable & to what extent we need to do more to make them OK.

Anyway, that's why there's still an open needinfo=me, but other bugs have been demanding my attention which is why I haven't closed this out.

Comment 25

[Daniel Holbert [:dholbert]]

Turns out Emilio's addressing this over in bug 1365806 (which is a just an earlier & more general version of this bug, really).