Closed Bug 1526567 Opened 6 years ago Closed 6 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: Oriol, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: testcase)

Attachments

(3 files, 1 obsolete file)

Attached 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.

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

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

Flags: needinfo?(oriol-bugzilla)
Attached 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.)

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.

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

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.

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)
Attached patch Follow-up fix for devtools bug (obsolete) — Splinter Review
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?
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+
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
Status: NEW → RESOLVED
Closed: 6 years ago
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
Duplicate of this bug: 1381512
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: