1350925 - [css-grid] A grid with overflow:auto in limited height receives a horizontal scrollbar briefly

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[forkest]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170316213902

Steps to reproduce:

Here's an example: https://codepen.io/anon/pen/YZOZyj


Actual results:

When you resize the resulting page so that a vertical scrollbar appears, the horizontal scrollbar appears too.

I've tried to fix it by setting height:0 to the grid (flex child) element:
https://codepen.io/anon/pen/RpYpMO
But encountered another, much more strange bug: the horizontal scrollbar only appears on the first calculation of the width. I mean, the horizontal scrollbar is there if the element has enough content to overflow on a page load (add more cell elements to the example if needed for your screen size), but after starting to resize the page the scrollbar disappears. And then blinks again when you resize it from big to small at the moment when the vertical scrollbar appears. Actually, both scrollbars appear, then you continue resizing, and the horizontal scrollbar disappears.


Expected results:

No horizontal scrollbar should appear together with the vertical scrollbar.

Comment 1

[forkest]

I've tried to remake it using another grid instead of flex, but the result is the same as in the second example:
https://codepen.io/anon/pen/aJeeNW

Comment 2

[Mats Palmgren (inactive)]

I can reproduce this in a local trunk build on Linux too.
The flexbox isn't needed just a grid that fills the viewport shows the same problem.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Triggering another reflow using zoom-in/out makes the horizontal
scrollbar go away...

Comment 4

[Mats Palmgren (inactive)]

Attachment: WIP

This fixes it for me.  I tend to think this a bug in the caller in this case
though.  nsHTMLScrollFrame::ReflowContents should reset the overflow areas
between each reflow of the child if it re-uses the ReflowOutput object,
which it does:
http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/layout/generic/nsGfxScrollFrame.cpp#663-665,697

Comment 5

[Mats Palmgren (inactive)]

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

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 1 - Reset the child ReflowOutput overflow areas before re-using it for another child reflow.

I tend to think this is the right fix and that nsHTMLScrollFrame is
violating the invariant that ReflowOutput::mOverflowAreas should be
clear when calling Reflow on a frame.

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Reftest.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #6)
> I tend to think this is the right fix and that nsHTMLScrollFrame is
> violating the invariant that ReflowOutput::mOverflowAreas should be
> clear when calling Reflow on a frame.

Could we add an assertion to enforce this invariant near the beginning of nsContainerFrame::ReflowChild, perhaps?  That would've caught this, I think.  (And it'll help prove that this supposed invariant [which superficially seems reasonable to me] is really an invariant.)

If that's trivial, could you perhaps do that a "part 2" here with rs=me (and bump the reftest to be part 3)?

Or, if that doesn't pass a Try run due to things violating this hypothetical invariant, perhaps spin that off into its own bug?

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8858810 [details] [diff] [review]
part 1 - Reset the child ReflowOutput overflow areas before re-using it for another child reflow.

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

Commit message nit:
> Bug 1350925 part 1 - Reset the child ReflowOutput overflow areas before re-using it for another child reflow.  r=dholbert
 
s/child/scrolled frame's/

r=me with that

Comment 10

[Daniel Holbert [:dholbert]]

(That was for the first instance of "child" in the commit message, I mean.  Point being: this isn't a general fix about all parents/children -- it's specific to scrollframes.)

Comment 11

[forkest]

Does this also fix my first example, where the scrollbar is displayed constantly, not briefly?

Comment 12

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Could we add an assertion to enforce this invariant near the beginning of
> nsContainerFrame::ReflowChild, perhaps?

Yes, good idea, it seems to hold:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4068744be75f6c419a2dab89a8f5efb5eea71c53&selectedJob=92383164

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d71c10dda1ae
part 1 - Reset the scrolled frame's ReflowOutput overflow areas before re-using it for another child reflow.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0ccddf5ff5
part 2 - Assert that we're given clean ReflowOutput overflow areas in ReflowChild.  rs=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1852d46ea730
part 3 - [css-grid] Reftest.

Comment 14

[Mats Palmgren (inactive)]

(In reply to forkest from comment #11)
> Does this also fix my first example, where the scrollbar is displayed
> constantly, not briefly?

I'm pretty sure it does, but feel free to verify Nightly yourself in a few days
(after this bug is resolved as FIXED).
https://www.mozilla.org/en-US/firefox/channel/desktop/

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d71c10dda1ae
https://hg.mozilla.org/mozilla-central/rev/1b0ccddf5ff5
https://hg.mozilla.org/mozilla-central/rev/1852d46ea730

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8858810 [details] [diff] [review]
part 1 - Reset the child ReflowOutput overflow areas before re-using it for another child reflow.

Approval Request Comment
[Feature/Bug causing the regression]: CSS Grid feature
[User impact if declined]:broken grid layout
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:we should take 1356820, 1348857, 1350925 in that order to avoid conflicts (they are technically independent though)
[Is the change risky?]:no
[Why is the change risky/not risky?]:trivial fix
[String changes made/needed]:none

I'd like to uplift all three of bug 1356820, bug 1348857, bug 1350925
to Beta v54 to fix some Grid layout bugs that web developers have reported.

Comment 17

[forkest]

(In reply to Mats Palmgren (:mats) from comment #14)
> (In reply to forkest from comment #11)
> > Does this also fix my first example, where the scrollbar is displayed
> > constantly, not briefly?
> 
> I'm pretty sure it does, but feel free to verify Nightly yourself in a few
> days
> (after this bug is resolved as FIXED).
> https://www.mozilla.org/en-US/firefox/channel/desktop/

Just tested. Confirming, fixed. Thank you.

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8858810 [details] [diff] [review]
part 1 - Reset the child ReflowOutput overflow areas before re-using it for another child reflow.

Fix a css grid issue and was verified. Beta54+. Should be in 54 beta 2.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8c70d11ecd10
https://hg.mozilla.org/releases/mozilla-beta/rev/5763b4d39e44
https://hg.mozilla.org/releases/mozilla-beta/rev/9fc3f176a12e

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Mats Palmgren (:mats) from comment #16)
> [Feature/Bug causing the regression]: CSS Grid feature
> [User impact if declined]:broken grid layout
> [Is this code covered by automated tests?]:yes
> [Has the fix been verified in Nightly?]:yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Mats' assessment on manual testing needs and the fact that this fix has automated coverage.