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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Mats Palmgren (inactive)]

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)

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 1 - Add a nsHTMLReflowState ctor flag to request shrink-wrap behavior

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.

Comment 4

[Mats Palmgren (inactive)]

Attachment: [css-grid] Request shrink-wrap behavior when doing a measuring reflow to figure out a grid item's block-size

Comment 5

[Mats Palmgren (inactive)]

Attachment: reftests

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

Comment 6

[Daniel Holbert [: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

Comment 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd03238ebdb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba839fc1b5d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf48c73a02ea

Comment 10

[Mats Palmgren (inactive)]

I fixed the test nits, thanks.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd03238ebdb5
https://hg.mozilla.org/mozilla-central/rev/ba839fc1b5d7
https://hg.mozilla.org/mozilla-central/rev/cf48c73a02ea