1418727 - [css-grid] Grid item overflows grid column when <pre> overflows width

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[keithjgrant]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171118100420

Steps to reproduce:

Reduced test case at https://codepen.io/keithjgrant/pen/YEYEJK?editors=1100#

I built a grid with auto-fill and min-max grid-template-columns. One of the grid items contains a <pre> with long lines of content.


Actual results:

The grid item containing the <pre> tag overflows the grid column.


Expected results:

The grid item should not be wider than the grid column.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 3

[Mats Palmgren (inactive)]

The reason for the overflow is that the grid item has min-width:auto
so we apply "Automatic Minimum Size of Grid Items" to it:
https://drafts.csswg.org/css-grid/#min-size-auto

This used to be the correct rendering, but there was a spec change
in Feb 2017 which added "and which span at least one track whose
min track sizing function is 'auto'".
https://github.com/w3c/csswg-drafts/blame/773d31327a15cb4c3c590e73244b7d397685ffe1/css-grid/Overview.bs#L1238
Per bullet 2 in the proposal here:
https://github.com/w3c/csswg-drafts/issues/283#issuecomment-268125974

We haven't implemented that additional condition yet.
(The tests here use "repeat(auto-fill, minmax(Npx, 1fr)" which isn't
'auto' so we should treat the min width as zero.  Meanwhile, you can
specify min-width:0 on the item to work around it.)

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Introduce StateBitsForRange() that collects the union of the state bits for a range of tracks (idempotent change)

(HasIntrinsicButNoFlexSizingInRange has an optimization to bail out
if it finds a flex max-sizing, but since we must now look at all
min-sizing functions anyway there's no point in keeping it.)

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Require that an item spans at least one track with an 'auto' min sizing function for Automatic Minimum Size to apply

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Reftest updates

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

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8934128 [details] [diff] [review]
part 1 - [css-grid] Introduce StateBitsForRange() that collects the union of the state bits for a range of tracks (idempotent change)

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

r=me, one nit:

::: layout/generic/nsGridContainerFrame.cpp
@@ +1094,3 @@
>     */
> +   TrackSize::StateBits StateBitsForRange(const LineRange& aRange) const;
> +  

whitespace on blank line

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8934129 [details] [diff] [review]
part 2 - [css-grid] Require that an item spans at least one track with an 'auto' min sizing function for Automatic Minimum Size to apply

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

r=me

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8934130 [details] [diff] [review]
part 3 - [css-grid] Reftest updates

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

Skimmed the test changes; I'm willing to trust that they're sane. r=me, one nit:

::: layout/reftests/css-grid/grid-item-auto-min-size-clamp-005-ref.html
@@ +145,5 @@
>  wrap.className = 'larger';
>  wrap.appendChild(n);
>  document.body.appendChild(wrap);
>  
> +/*

You're commenting out half of the script here, and I don't immediately see why.  Should the commented-out section just be removed?

(If you want to keep it, maybe add an //TODO comment explaining why it's here & why it's disabled?)

::: layout/reftests/css-grid/grid-item-auto-min-size-clamp-005.html
@@ +136,5 @@
>  wrap.className = 'larger';
>  wrap.appendChild(n);
>  document.body.appendChild(wrap);
>  
> +/*

Same here.

Comment 10

[Mats Palmgren (inactive)]

Right, I forgot to add a TODO there.  I figured I should sort out
who is right about this issue first:
https://bugs.chromium.org/p/chromium/issues/detail?id=789927
(to avoid having to update these tests twice)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db0445690ceb
part 1 - [css-grid] Introduce StateBitsForRange() that collects the union of the state bits for a range of tracks (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/440408ffbb8c
part 2 - [css-grid] Require that an item spans at least one track with an 'auto' min sizing function for Automatic Minimum Size to apply.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d41ae02660f
part 3 - [css-grid] Reftest updates.

Comment 12

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (bug 1418727) for unexpected passes, e.g. wpt's /css/css-grid/alignment/grid-alignment-implies-size-change-007.html r=backout on a CLOSED TREE

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8de338c0f72eeac5ce39afa5d32e70f7832c53b

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2dba9f7cb4fa093225a500d0c7325d2541f2e04c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d57b532c6cd0
part 1 - [css-grid] Introduce StateBitsForRange() that collects the union of the state bits for a range of tracks (idempotent change).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a14343b5393
part 2 - [css-grid] Require that an item spans at least one track with an 'auto' min sizing function for Automatic Minimum Size to apply.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa4b66ef6331
part 3 - [css-grid] Reftest updates.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f37b86659a8b
part 4 - [css-grid] Update WPT expected result (automated update).  r=me

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d57b532c6cd0
https://hg.mozilla.org/mozilla-central/rev/2a14343b5393
https://hg.mozilla.org/mozilla-central/rev/fa4b66ef6331
https://hg.mozilla.org/mozilla-central/rev/f37b86659a8b