[css-grid] min-height:auto clamped to 0 by indefinite max-height percentage
Categories
(Core :: Layout: Grid, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: Oriol, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: testcase)
Attachments
(3 files, 1 obsolete file)
|
351 bytes,
text/html
|
Details | |
|
51.20 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
|
4.59 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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•5 years 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?
|
Reporter | |
Comment 2•5 years ago
|
||
This is from https://crbug.com/890278, I don't think there is a WPT.
|
|
Assignee | |
Comment 3•5 years ago
|
||
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...
|
||
Comment 4•5 years ago
|
||
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•5 years ago
|
||
(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•5 years 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.
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
|
||
Comment 8•5 years ago
|
||
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
|
|
Assignee | |
Comment 9•5 years 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•5 years ago
|
||
The failed test is due to a dependence in a devtools panel on this bug.
STR
- open devtools Console
- type "document <ENTER>"
- open the context menu on the result line from 2
- type V (Open in Sidebar)
There should be a vertical scrollbar but it was missing.
|
|
Assignee | |
Comment 11•5 years ago
|
||
|
|
||
Comment 12•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
|
|
||
Comment 15•5 years ago
|
||
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•5 years 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•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/739201563949
https://hg.mozilla.org/mozilla-central/rev/31404c0c060e
|
||
Comment 18•5 years ago
|
||
James, do you know why a PR for exporting these tests wasn't opened?
|
||
Comment 19•5 years ago
|
||
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.
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15764 for changes under testing/web-platform/tests
Upstream PR merged
Description
•