[css-grid] FR Unit in grid-template-row not filling viewport

RESOLVED FIXED in Firefox 53

Status

()

Core
Layout
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: Chris, Assigned: mats)

Tracking

(Blocks: 1 bug, {compat, testcase})

52 Branch
mozilla55
compat, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 wontfix, firefox53+ fixed, firefox54+ fixed, firefox55 fixed, firefox-esr45 disabled, firefox-esr52 wontfix)

Details

(Whiteboard: [parity-chrome])

Attachments

(6 attachments)

(Reporter)

Description

3 months ago
Created attachment 8846497 [details]
Screen Shot 2017-03-13 at 18.17.44.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.98 Safari/537.36

Steps to reproduce:

1. Create a div [a] with display grid
2. Create a child also with display grid [b]
3. Add another div [c] with some content in it, a heading, and a paragraph with dummy copy
3. On the parent [a] give the grid grid-template-rows: 1fr;
4. On the child [b] add align-items: center


Actual results:

The the div [b] never stretched to the bottom of the viewport, so alignment appeared to be broken (when the issue was actually the FR unit not applying)


Expected results:

The element [b] to stretch the viewport like it does in Chrome 57 and Safari TP.
See demo in different browsers here:
http://codepen.io/chriswrightdesign/pen/ea81b46110728f5b010234e1bdb75691?editors=1100

Updated

3 months ago
Blocks: 616605
Component: Untriaged → Layout
Product: Firefox → Core
Summary: CSS Grid - FR Unit in grid-template-row not filling viewport → [css-grid] FR Unit in grid-template-row not filling viewport

Updated

3 months ago
Flags: needinfo?(mats)
(Assignee)

Comment 1

3 months ago
Created attachment 8846662 [details]
Reduced testcase

Hmm, I'll have to check what the spec says about this case...
Assignee: nobody → mats
Flags: needinfo?(mats)
(Assignee)

Comment 2

3 months ago
The spec sections that apply are:
https://drafts.csswg.org/css-grid/#fr-unit
"When the available space is infinite (which happens when the grid container’s width or height is indefinite), ..."
which dictates we should use "If the free space is an indefinite length:" section here:
https://drafts.csswg.org/css-grid/#algo-flex-tracks
Which ends with:
"
If using this flex fraction would cause the grid to be smaller than the grid container’s min-width/height (or larger than the grid container’s max-width/height), then redo this step, treating the free space as definite and the available grid space as equal to the grid container’s content box size when it’s sized to its min-width/height (max-width/height).
"

We implement this clause here (from bug 1309407):
http://searchfox.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp#4780
When I single-step through here it seems we never make origSizes.isSome()
true though, so we never execute the step that applies min/max.
In the for-loop that precedes it, we have one flex-track and we have:
applyMinMax = true
fr = 120
flexFactor = 1
flexLength = 120
base = @0x7ffd6e68a770: 120
so we don't execute the "if (flexLength > base)" code that would emplace it.

Hmm, I don't really recall why I did that... maybe I thought that if no mBase
was growing then applying min/max wouldn't matter?

Anyway, it doesn't seem like a valid optimization.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

3 months ago
Created attachment 8847278 [details] [diff] [review]
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size

There are actually two separate logical errors in this method,
so I'll fix them in separate patches for easier reviewing.

The first part is that "origSizes.isSome()" is simply a bogus
requirement for applying min/max-sizes here.  I'm still keeping
the optimization of not needlessly copying the mSizes array
(as originally intended) since it's a quite common case.
Attachment #8847278 - Flags: review?(dholbert)
(Assignee)

Comment 4

3 months ago
Created attachment 8847283 [details] [diff] [review]
part 2 - [css-grid] Don't require 'fr' to be non-zero to apply min/max-size

The second bug is that min/max-sizes were only applied under
the "if (fr != 0.0f)" block.  This is bogus since the calculated
'fr' value depends on 'aAvailableSize' which might change by
applying min/max-sizes and thus 'fr' could become non-zero in
the second round.  So this patch just moves "applyMinMax" block
out one level.
Attachment #8847283 - Flags: review?(dholbert)
(Assignee)

Comment 5

3 months ago
Created attachment 8847285 [details] [diff] [review]
part 2 (wdiff)
(Assignee)

Comment 6

3 months ago
Created attachment 8847286 [details] [diff] [review]
part 3 - [css-grid] Additional reftests for min/max-sizes affecting flexible track sizing

This adds reftests that cover both issues mentioned above.
(Assignee)

Comment 7

3 months ago
Given that this is compat issue with Chrome we should probably uplift this
straight to beta (v53).  It's a low-risk change that clearly only affects
CSS Grid layout.
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → disabled
status-firefox-esr52: --- → wontfix
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
Keywords: compat, testcase
Whiteboard: [parity-chrome]
Tracking this compat issue for 53/54.
tracking-firefox53: ? → +
tracking-firefox54: ? → +
Comment on attachment 8847278 [details] [diff] [review]
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size

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

r=me
Attachment #8847278 - Flags: review?(dholbert) → review+
Comment on attachment 8847283 [details] [diff] [review]
part 2 - [css-grid] Don't require 'fr' to be non-zero to apply min/max-size

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

r=me

(Thanks for the wdiff, too.)
Attachment #8847283 - Flags: review?(dholbert) → review+

Comment 11

3 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b49afac936ad
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/73be2cb53669
part 2 - [css-grid] Don't require 'fr' to be non-zero to apply min/max-size.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/534086689e77
part 3 - [css-grid] Additional reftests for min/max-sizes affecting flexible track sizing.
(Assignee)

Updated

3 months ago
Flags: in-testsuite+

Comment 12

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b49afac936ad
https://hg.mozilla.org/mozilla-central/rev/73be2cb53669
https://hg.mozilla.org/mozilla-central/rev/534086689e77
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 13

3 months ago
Comment on attachment 8847278 [details] [diff] [review]
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: possible rendering errors in CSS Grid layout; it would be nice to take this for compat with Chrome
[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]:
[Is the change risky?]:no
[Why is the change risky/not risky?]: minor changes in a single function in nsGridContainerFrame.cpp, so it can't cause regressions in anything else than grid layout
[String changes made/needed]:none
Attachment #8847278 - Flags: approval-mozilla-beta?
Attachment #8847278 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 14

3 months ago
(the uplift request is for all parts in case that wasn't clear)
Comment on attachment 8847278 [details] [diff] [review]
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size

Fix a CSS Grid layout issue. Aurora54+ and Beta53+.
Attachment #8847278 - Flags: approval-mozilla-beta?
Attachment #8847278 - Flags: approval-mozilla-beta+
Attachment #8847278 - Flags: approval-mozilla-aurora?
Attachment #8847278 - Flags: approval-mozilla-aurora+

Comment 16

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0129d0167217
https://hg.mozilla.org/releases/mozilla-aurora/rev/87a507d29b84
https://hg.mozilla.org/releases/mozilla-aurora/rev/930aa449c30f
status-firefox54: affected → fixed

Comment 17

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/82d420722b84
https://hg.mozilla.org/releases/mozilla-beta/rev/07e2144b72e0
https://hg.mozilla.org/releases/mozilla-beta/rev/6fca76f3b46b
status-firefox53: affected → fixed
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1350855
You need to log in before you can comment on or make changes to this bug.