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

RESOLVED FIXED in Firefox 67

Status

()

P3
normal
RESOLVED FIXED
a month ago
2 days ago

People

(Reporter: Oriol, Assigned: mats)

Tracking

({testcase})

unspecified
mozilla67
testcase
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a month ago
Posted file 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.

(Assignee)

Comment 1

a month ago

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?

Assignee: nobody → mats
Flags: needinfo?(oriol-bugzilla)
Keywords: testcase
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
(Reporter)

Comment 2

a month ago

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

Flags: needinfo?(oriol-bugzilla)
(Assignee)

Comment 3

a month ago
Posted patch fix+wptSplinter Review

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

Attachment #9043458 - Flags: review?(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.)
Attachment #9043458 - Flags: review?(dholbert) → review+

(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.)

(Assignee)

Comment 6

a month ago

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

28 days ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e919946fd913
[css-grid] Make the block-axis percentage basis be indefinite for measuring reflows.  r=dholbert
(Assignee)

Comment 9

28 days ago

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.

(Assignee)

Comment 10

25 days ago

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.
Flags: needinfo?(mats)
(Assignee)

Comment 11

25 days ago
Attachment #9045650 - Flags: review?(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 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:mats@mozilla.com">
> +  <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?
(Assignee)

Comment 14

25 days ago
Attachment #9045650 - Attachment is obsolete: true
Attachment #9045650 - Flags: review?(dholbert)
Attachment #9045808 - Flags: review?(dholbert)
Comment on attachment 9045808 [details] [diff] [review]
Follow-up fix for devtools bug

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

r=me. Thanks!
Attachment #9045808 - Flags: review?(dholbert) → review+

Comment 16

24 days ago
Pushed by mpalmgren@mozilla.com:
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

24 days ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 24 days ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Flags: needinfo?(james)

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.

Flags: needinfo?(james)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15764 for changes under testing/web-platform/tests
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.