forkest Reporter
	Description • 7 years ago

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.

	forkest Reporter
	Comment 1 • 7 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 2 • 7 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 3 • 7 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 4 • 7 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 5 • 7 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 6 • 7 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 8 • 7 years ago

(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?

	Daniel Holbert [:dholbert]
	Comment 9 • 7 years ago

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

	Daniel Holbert [:dholbert]
	Comment 10 • 7 years ago

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

	forkest Reporter
	Comment 11 • 7 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 12 • 7 years ago

(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

	Pulsebot
	Comment 13 • 7 years ago

Pushed by mpalmgren@mozilla.com:
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.

	Mats Palmgren (inactive) Assignee
	Comment 14 • 7 years ago

(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/

	Carsten Book [:Tomcat]
	Comment 15 • 7 years ago
bugherder

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

	Mats Palmgren (inactive) Assignee
	Comment 16 • 7 years ago

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.

	forkest Reporter
	Comment 17 • 7 years ago

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

	Gerry Chang [:gchang]
	Comment 18 • 7 years ago

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.

	Carsten Book [:Tomcat]
	Comment 19 • 7 years ago
bugherder uplift

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

	Andrei Vaida [:avaida]
	Comment 20 • 7 years ago

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