Closed Bug 1462854 Opened Last year Closed Last year

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

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mats, Assigned: mats)

References

Details

(Keywords: testcase)

Attachments

(3 files)

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.
Priority: -- → P3
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
Attachment #8981168 - Flags: review?(dholbert)
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.
Attachment #8981169 - Flags: review?(dholbert)
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)
Attachment #8981168 - Flags: review?(dholbert) → review+
Attachment #8981169 - Flags: review?(dholbert) → review+
(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.
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.
Depends on: 1480254
You need to log in before you can comment on or make changes to this bug.