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

RESOLVED FIXED in Firefox 54

Status

()

P3
minor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: forkest, Assigned: mats)

Tracking

(Blocks: 1 bug, {testcase})

52 Branch
mozilla55
testcase
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

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

Updated

2 years ago
Blocks: 616605
Component: Untriaged → Layout
Product: Firefox → Core
Summary: A grid with overflow:auto in a limited height flex receives a horizontal scrollbar → [css-grid] A grid with overflow:auto in a limited height flex receives a horizontal scrollbar
(Reporter)

Comment 1

2 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
(Assignee)

Comment 2

2 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.
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Summary: [css-grid] A grid with overflow:auto in a limited height flex receives a horizontal scrollbar → [css-grid] A grid with overflow:auto in limited height receives a horizontal scrollbar briefly
(Assignee)

Updated

2 years ago
Keywords: testcase
(Assignee)

Comment 3

2 years ago
Created attachment 8858695 [details]
Testcase #1

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

Comment 4

2 years ago
Created attachment 8858698 [details] [diff] [review]
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
Assignee: nobody → mats
(Assignee)

Comment 6

2 years ago
Created attachment 8858810 [details] [diff] [review]
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.
Attachment #8858810 - Flags: review?(dholbert)
(Assignee)

Comment 7

2 years ago
Created attachment 8858811 [details] [diff] [review]
part 2 - [css-grid] Reftest.
(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 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
Attachment #8858810 - Flags: review?(dholbert) → review+
(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.)
(Reporter)

Comment 11

2 years ago
Does this also fix my first example, where the scrollbar is displayed constantly, not briefly?
(Assignee)

Comment 12

2 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

Comment 13

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

Comment 14

2 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/
(Assignee)

Updated

2 years ago
Flags: in-testsuite+

Comment 15

2 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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 16

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

Updated

2 years ago
Attachment #8858810 - Flags: approval-mozilla-beta?
(Reporter)

Comment 17

2 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.
status-firefox54: --- → affected
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.
Attachment #8858810 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.