Chris Reporter
	Description • 7 years ago

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

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

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

	Mats Palmgren (inactive) Assignee
	Comment 2 • 7 years 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.

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

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.

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

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.

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

This adds reftests that cover both issues mentioned above.

	Mats Palmgren (inactive) Assignee
	Comment 7 • 7 years 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.

	Julien Cristau [:jcristau]
	Comment 8 • 7 years ago

Tracking this compat issue for 53/54.

	Daniel Holbert [:dholbert]
	Comment 9 • 7 years 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

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

r=me

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

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

	Pulsebot
	Comment 11 • 7 years 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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 7 years 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

	Mats Palmgren (inactive) Assignee
	Comment 13 • 7 years 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

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

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

	Gerry Chang [:gchang]
	Comment 15 • 7 years 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

Fix a CSS Grid layout issue. Aurora54+ and Beta53+.

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

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

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

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