1526567 - [css-grid] min-height:auto clamped to 0 by indefinite max-height percentage

Status: RESOLVED FIXED

(Core :: Layout: Grid, enhancement, P3)

Description

[Oriol Brufau [:Oriol]]

Attachment: testcase

Consider

.grid {
  display: grid;
  height: 100px;
  width: 100px;
  border: solid thick;
}
.item {
  max-height: 100%;
  max-width: 100%;
  background: cyan;
}
.content {
  height: 200px;
  width: 200px;
  border: dashed thick magenta;
}

<div class="grid">
  <div class="item">
    <div class="content"></div>
  </div>
</div>

When resolving intrinsic track sizes, the base size is set to the minimum contribution, which is the automatic minimum size. This will be

the min-content size in the relevant axis, [...] clamped by the same-axis max size property if that is definite.

Here 100% is not definite, because it would be resolved with respect to the grid area, which hasn't been calculated yet.

Therefore, the row should be 200px tall and the column 200px wide, even if this overflows the grid container.

However, in Firefox, the row is only 100px tall. It seems the indefinite max-height percentage is clamping the auto min-height to 0.

Chromium and Edge do it correctly.

Comment 1

[Mats Palmgren (inactive)]

Looks like we resolve the max-height percentage to zero while
calculating the min-content contribution in the block-axis.

Oriol, is this from a WPT or should I write some?

Comment 2

[Oriol Brufau [:Oriol]]

This is from https://crbug.com/890278, I don't think there is a WPT.

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix+wpt

I'm not really sure why this breaks layout/reftests/css-grid/grid-item-table-stretch-002.html on Android only, given that the tests contains no percentages whatsoever. As far as I know we don't have any Android-specific code in Grid or Table layout so I suspect this is an unrelated existing issue. I'll just mark it as failing for now and file a follow-up bug for it. Let me know if you have any clues as to what the problem might be...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1d0695703248bf578193117958ff42f724fa09e&selectedJob=228010498

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 9043458 [details] [diff] [review]
fix+wpt

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

r=me with nits:

::: layout/reftests/css-grid/reftest.list
@@ +122,5 @@
>  == grid-item-overflow-stretch-006.html grid-item-overflow-stretch-006-ref.html
>  == grid-item-canvas-001.html grid-item-canvas-001-ref.html
>  skip-if(Android) == grid-item-button-001.html grid-item-button-001-ref.html
>  == grid-item-table-stretch-001.html grid-item-table-stretch-001-ref.html
> +fails-if(Android) == grid-item-table-stretch-002.html grid-item-table-stretch-002-ref.html # Bug XXX

Just a reminder to fill in this XXX with a new bug number before landing.

(Also: it'd probably be worth giving the test 5-10 retriggers on Try, to check that 'fails' is really appropriate, rather than 'random', if e.g. it's just a race condition that we often-but-not-always lose on Android.)

::: testing/web-platform/tests/css/css-grid/grid-items/grid-item-percentage-sizes-001-ref.html
@@ +34,5 @@
> +  height: 30px;
> +  width: 30px;
> +}
> +
> +.hl  .item { writing-mode: horizontal-tb; direction:ltr; }

This testcase never actually use this `.hl` rule right now.

I'd suggest either using it (wrapping the first chunk of grids in `<div class="hl">` like you do for the other writing modes), or removing it, or putting it in a /**/ comment with a note saying "These are the default styles:".

(This applies to all of the grid-item-percentage-sizes-* test files included here.)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #3)

I'm not really sure why this breaks layout/reftests/css-grid/grid-item-table-stretch-002.html [...]
Let me know if you have any clues as to what the problem might be...

I don't have any clues, but I did discover one possibly-related bug while investigating, and I filed bug 1527734 on it. (I don't have any confidence that bug 1527734 is the explanation for the failure you're seeing here, though.)

Comment 6

[Mats Palmgren (inactive)]

Also: it'd probably be worth giving the test 5-10 retriggers on Try, to check that 'fails' is really appropriate

Yup, I did so in an earlier Try push and it is indeed a permafail, not random.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e919946fd913
[css-grid] Make the block-axis percentage basis be indefinite for measuring reflows.  r=dholbert

Comment 8

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failures on browser_webconsole_sidebar_scroll.js

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/659c7fae6ae49dc00e950a6755b7f724bcb9084e
Push link:https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&selectedJob=229063893&revision=e919946fd91306287e4a6197c52a77b6361676ce
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229063893&repo=mozilla-inbound&lineNumber=4268

Comment 9

[Mats Palmgren (inactive)]

Hmmm, so that test fails on OSX and Windows only? But passed on Linux / Android?
Fwiw, the test works fine when I run it locally on Linux.

Comment 10

[Mats Palmgren (inactive)]

The failed test is due to a dependence in a devtools panel on this bug.
STR

1. open devtools Console
2. type "document <ENTER>"
3. open the context menu on the result line from 2
4. type V (Open in Sidebar)
   There should be a vertical scrollbar but it was missing.

Comment 11

[Mats Palmgren (inactive)]

Attachment: Follow-up fix for devtools bug

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 9045650 [details] [diff] [review]
Follow-up fix for devtools bug

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

::: devtools/client/themes/webconsole.css
@@ +1002,5 @@
>  
>  .sidebar-contents {
>    grid-row: 2 / 3;
>    overflow: auto;
> +  min-height: 0;

Do we actually need `min-height:0` here?

This element seems to be a grid item, and it has `overflow:auto` (specified on the line right above your min-height decl here), which I think means that its automatic minimum height is already 0.

I tried just the other change in this patch (the `height:100vh`) by itself (applied manually/interactively in browser-toolbox-devtools in a build with only your first patch), and it seemed to fix the bug with your "document[Enter]" etc. STR, without any need for this adjustment here.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 9045650 [details] [diff] [review]
Follow-up fix for devtools bug

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

::: testing/web-platform/tests/css/css-grid/grid-items/grid-item-overflow-auto-max-height-percentage.html
@@ +4,5 @@
> +     http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +<html><head>
> +  <meta charset="utf-8">
> +  <title>Testcase for bug 1526567</title>

It'd probably be helpful to have a more-descriptive title (or an HTML comment, or a rel="assert" tag) talking about the scenario that you're aiming to test, so that this isn't quite so opaque to folks looking at the test.

Maybe just a brief mention of how indefinite max-height and content contributions and clamping are expected to interact here?

@@ +6,5 @@
> +<html><head>
> +  <meta charset="utf-8">
> +  <title>Testcase for bug 1526567</title>
> +  <link rel="author" title="Mats Palmgren" href="mailto:[e-mail]">
> +  <link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1526567">

I think this needs a spec rel="help" link, too. At least, reviewbot complains on phabricator if new WPT tests lack such a link (see e.g. https://phabricator.services.mozilla.com/D19108#inline-108463 )


Maybe https://drafts.csswg.org/css-grid-1/#min-size-auto would be a reasonable help link here?

Comment 14

[Mats Palmgren (inactive)]

Attachment: Follow-up fix for devtools bug

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 9045808 [details] [diff] [review]
Follow-up fix for devtools bug

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

r=me. Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/739201563949
[css-grid] Make the block-axis percentage basis be indefinite for measuring reflows.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/31404c0c060e
Fix devtools dependence on this bug.  r=dholbert

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/739201563949
https://hg.mozilla.org/mozilla-central/rev/31404c0c060e

Comment 18

[Emilio Cobos Álvarez (:emilio)]

James, do you know why a PR for exporting these tests wasn't opened?

Comment 19

[James Graham [:jgraham]]

Yes, because there were merge conflicts (there should really by a comment for that). The reason there's merge conflicts appears to be that the changes from Bug 1513959 didn't end up fully synced; it looks like we ended up not updating for the changes to the patch after it was backed out.So I'll fix that and then this one should be unblocked.

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15764 for changes under testing/web-platform/tests

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged