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

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: mats)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8690993 [details]
testcase 1
(Assignee)

Comment 2

3 years ago
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)
(Assignee)

Comment 3

3 years ago
Created attachment 8692243 [details] [diff] [review]
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.
Attachment #8692243 - Flags: review?(dholbert)
(Assignee)

Comment 4

3 years ago
Created 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
Attachment #8692245 - Flags: review?(dholbert)
(Reporter)

Comment 6

3 years ago
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+
(Reporter)

Comment 7

3 years ago
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+
(Reporter)

Comment 8

3 years ago
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/)
(Assignee)

Comment 10

3 years ago
I fixed the test nits, thanks.
Flags: in-testsuite+

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fd03238ebdb5
https://hg.mozilla.org/mozilla-central/rev/ba839fc1b5d7
https://hg.mozilla.org/mozilla-central/rev/cf48c73a02ea
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.