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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris]

Attachment: 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

Comment 1

[Mats Palmgren (inactive)]

Attachment: Reduced testcase

Hmm, I'll have to check what the spec says about this case...

Comment 2

[Mats Palmgren (inactive)]

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.

Comment 3

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 4

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 2 (wdiff)

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Additional reftests for min/max-sizes affecting flexible track sizing

This adds reftests that cover both issues mentioned above.

Comment 7

[Mats Palmgren (inactive)]

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.

Comment 8

[Julien Cristau [:jcristau]]

Tracking this compat issue for 53/54.

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Daniel Holbert [:dholbert]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b49afac936ad
https://hg.mozilla.org/mozilla-central/rev/73be2cb53669
https://hg.mozilla.org/mozilla-central/rev/534086689e77

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Mats Palmgren (inactive)]

(the uplift request is for all parts in case that wasn't clear)

Comment 15

[Gerry Chang [:gchang]]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Carsten Book [:Tomcat]]

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