Closed Bug 1227285 Opened 4 years ago Closed 4 years ago

[css-grid] Grid item with 100%-wide image ends up hugely tall (approximately nscoord_MAX)

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: dholbert, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

STR:
 1. Load attached testcase.
 2. See how tall the grid item (red border) & grid (gray border) are.

EXPECTED RESULTS:
Grid & grid item should be as tall as its content, the image.  (Or maybe a little taller? In Chrome, they're a few pixels taller for some reason -- there's a strip of white between the img and the bottom borders.)

ACTUAL RESULTS:
Grid & grid item are hugely tall, sticking way off the bottom of the viewport.  My debug build says the grid is 1013742296 app units tall, which is in the neighborhood of nscoord_MAX.

I'm using latest Nightly, 45.0a1 (2015-11-23).

(Noticed this bug while tweaking testcase for bug 1227140.)

I'm guessing we're messing things up when computing the "transferred size", or something like that.

Tentatively assigning to Mats, since he's likely who'll end up looking at this.
Attached file testcase 1
We need the min-content block-size for the the grid item, which requires a reflow
in this case, so we reflow it with an "infinite" inlinine-size (to force it to
not take any break opportunities other than explicit ones).  The problem is that
the grid item is a block frame, and with the default justify-self:stretch it will
happily set its desired inline-size to that huge value (INFINITE_ISIZE_COORD).
That's what its children in turn will use as the available inline-size.  In this
case the image with width:100% will get that width and its transferred size
(height) will be the same, which makes the block (grid item) block-size huge too.
Hmm, this is kind of the opposite of what we intended :-)

So I think the fix here is to use the ComputeSizeFlags::eShrinkWrap flag to
avoid the block (grid item) itself to grow beyond what its content demands.
Does that sound right?

(it fixes the problem here, and pass the grid reftests locally)
This adds a generic mechanism for requesting ComputeSizeFlags::eShrinkWrap
behavior when creating a nsHTMLReflowState, by giving COMPUTE_SIZE_SHRINK_WRAP
as a flag to the ctor.
Attachment #8692243 - Flags: review?(dholbert)
Comment on attachment 8692243 [details] [diff] [review]
part 1 - Add a nsHTMLReflowState ctor flag to request shrink-wrap behavior

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

r=me
Attachment #8692243 - Flags: review?(dholbert) → review+
Comment on attachment 8692245 [details] [diff] [review]
[css-grid] Request shrink-wrap behavior when doing a measuring reflow to figure out a grid item's block-size

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

r=me
Attachment #8692245 - Flags: review?(dholbert) → review+
Comment on attachment 8692249 [details] [diff] [review]
reftests

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

Two nits on the test patch:

::: layout/reftests/css-grid/grid-min-max-content-sizing-002-ref.html
@@ +79,5 @@
> +</div>
> +
> +
> +
> +</body>

Nit: consider collapsing these 3 blank lines down to just 1 blank line, before </body> in the testcase & reference case here.

::: layout/reftests/css-grid/grid-min-max-content-sizing-002.html
@@ +7,5 @@
> +  <meta charset="utf-8">
> +  <title>CSS Grid Test: Testing grid minmax(min-content,max-content) column/rows</title>
> +  <link rel="author" title="Mats Palmgren" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1227285">
> +  <link rel="help" href="http://dev.w3.org/csswg/css-grid/#valdef-grid-template-columns-max-content">
> +  <link rel="match" href="grid-min-max-content-sizing-001-ref.html">

This "match" line has the wrong reference filename. (needs s/001/002/)
I fixed the test nits, thanks.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.