Closed Bug 1418727 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect, P3, minor)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: keithjgrant, Assigned: mats)

Details

(Keywords: testcase)

Attachments

(5 files)

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.
Component: Untriaged → Layout
Product: Firefox → Core
Attached file Testcase #1
Assignee: nobody → mats
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached file Testcase #2
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.)
Severity: normal → minor
Keywords: testcase
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Grid item overflows grid column when <pre> overflows width → [css-grid] Grid item overflows grid column when <pre> overflows width
Priority: -- → P3
(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.)
Attachment #8934128 - Flags: review?(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
Attachment #8934128 - Flags: review?(dholbert) → review+
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
Attachment #8934129 - Flags: review?(dholbert) → review+
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.
Attachment #8934130 - Flags: review?(dholbert) → review+
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)
Pushed by mpalmgren@mozilla.com:
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.
Flags: in-testsuite+
Pushed by mpalmgren@mozilla.com:
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
Flags: needinfo?(mats)
You need to log in before you can comment on or make changes to this bug.