Mats Palmgren (inactive) Assignee
	Description • 6 years ago

Percentage size grid item with overflow != visible is reporting the wrong min-size contribution.

Testcase: https://wptest.center/#/18osz0

https://drafts.csswg.org/css-grid/#min-size-contribution
The percentage 'height' in this test "behaves as auto" so we should use the used value of "min-height:auto" as the specified size (the spec currently says "computed" but I suspect they mean "used")

https://drafts.csswg.org/css-grid/#min-size-auto
AMS doesn't apply when overflow != visible though, so we should use "the automatic minimum size is otherwise zero, as usual".  That gives us
the used min-height value zero, which we'll then use as the 'height'
when calculating the min-size contribution.

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

I'll preserve our current behavior for calc() percentages for now and
address that later in bug 1463700 since that's a general bug not specific
to grid.  Also, the new spec text [1] is still a bit vague so I'm waiting
for clarification [2] before attacking that bug.
[1] https://drafts.csswg.org/css-sizing-3/#percentage-sizing
[2] https://github.com/w3c/csswg-drafts/issues/2674

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

The second hunk is what fixes the bug.  It's makes width/height
percentages in https://drafts.csswg.org/css-grid/#min-size-contribution
"behave as auto".  Note that the percentage basis for the item's size
at this point is always indefinite since it resolves against the grid
area's size in the same axis, which is what we're currently calculating.

The first hunk just makes MinSizeContributionForAxis deal with images
etc since it didn't have to do that before when we took the min-content
contribution path.  As noted above, this is likely not according to spec
for calc() percentages, but I'll punt on that to bug 1463700.

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

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

	Daniel Holbert [:dholbert]
	Comment 4 • 6 years ago

Comment on attachment 8981168 [details] [diff] [review]
part 1 - Factor out "percentage on replaced box that should resolve against zero" test (idempotent patch)

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

::: layout/base/nsLayoutUtils.cpp
@@ -5154,5 @@
> -        aFrame->IsFrameOfType(nsIFrame::eReplacedSizing)) ||
> -       (aStyleSize.HasPercent() &&
> -        FormControlShrinksForPercentISize(aFrame)))) {
> -    // A percentage width or max-width on replaced elements means they
> -    // can shrink to 0.

Right now this comment isn't getting preserved anywhere.  Maybe it'd be good to keep it around here, at least these first two lines, so that our intent is still documented in a human-readable way?   (Alongside the XXX comment that you've added to clarify RE correctness)

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

(In reply to Daniel Holbert [:dholbert] from comment #4)
> > -    // A percentage width or max-width on replaced elements means they
> > -    // can shrink to 0.
> 
> Right now this comment isn't getting preserved anywhere.  Maybe it'd be good
> to keep it around here, at least these first two lines, so that our intent
> is still documented in a human-readable way?

I removed it b/c what it says is actually wrong per spec, so I didn't see
the point in preserving it.  We can document this better in bug 1463700
after we've updated the code to follow the spec.

	Pulsebot
	Comment 6 • 6 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34278b2ad4b9
part 1 - Factor out "percentage on replaced box that should resolve against zero" test (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/547112cf7003
part 2 - [css-grid] Handle [max-]width/height percentages in min-size contributions according to spec.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/55e6a68a5056
part 3 - [css-grid] Update reftest reference.

	Andreea Pavel [:apavel]
	Comment 7 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/34278b2ad4b9
https://hg.mozilla.org/mozilla-central/rev/547112cf7003
https://hg.mozilla.org/mozilla-central/rev/55e6a68a5056