[css-flex] Flex item is considered to have indefinite height with "flex-basis: auto" despite of "flex-grow: 1"
Reported by
l.pmdi...@gmail.com,
Nov 11 2017
|
|||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.30 Safari/537.36 Steps to reproduce the problem: Go to https://jsfiddle.net/m7bfhxds/4/ What is the expected behavior? The four grid items with a green background should cover the red grid area. What went wrong? The grid items have the correct width of half the grid but a computed height of 0. Did this work before? No Does this work in other browsers? Yes Chrome version: 64.0.3264.0 Channel: canary OS Version: 10.0 Flash Version: Works in Edge 16 and Firefox Nightly.
,
Nov 13 2017
,
Nov 13 2017
Confirmed bug in trunk, even using 'auto' tracks, so the issue is not specific if flexible tracks. I'll try to provide a simplified test case later.
,
Dec 18 2017
Issue 795260 has been merged into this issue.
,
Dec 18 2017
Yeah it seems that the available height of the grid container is wrongly computed when it's a flex item. Note that if we use "flex-basis: 0;" the issue is gone. Check the following example: https://codepen.io/mrego/pen/YYwGaK?editors=1100 * The first grid container uses "flex-grow: 1; flex-basis: content;": The height of the grid container is properly computed, but the available space for rows is wrong as the "1fr" row doesn't take the whole height (the same happens with a "auto" row that doesn't get stretched as expected). * The second grid container uses "flex-grow: 1; flex-basis: 0;": In this case both the height of the grid container and the available space for the rows seem correct. We need to investigate what's going on and why this behaves differently. BTW, this works fine in Firefox, Safari and Edge. It seems Chromium is the only one with problems here.
,
Dec 18 2017
,
Dec 19 2017
It seems the problem is that the flex item's height is considered indefinite
with "flex-basis: auto;" (or "content") while it's considered definite
with "flex-basis: 0;".
I'm attaching a simplified example that only uses Flexbox (no grid layout involved)
which hits the same codepath. The percentage height of an element inside
the flex item is not computed, as it's considered to have an indefinite height.
This is the same behavior in Blink and WebKit, however in Firefox and Edge
the percentage is resolved.
Regarding the code the key is in LayoutFlexibleBox::MainSizeForPercentageResolution()
which always returns -1 for "flex-basis: auto".
@cbiesinger what do you think? Should that case be considered definite too?
This affects grid because of the code in
LayoutBlock::AvailableLogicalHeightForPercentageComputation():
LayoutUnit stretched_flex_height(-1);
if (IsFlexItem())
stretched_flex_height =
ToLayoutFlexibleBox(Parent())
->ChildLogicalHeightForPercentageResolution(*this);
Which is used to check if the flex item has a definite or indefinite height.
,
Dec 20 2017
So... we implemented this change: https://drafts.csswg.org/css-flexbox/#change-201403-definite-flexing But I just found out that this text changed more recently again and that a flex item's main size is now definite in most cases: https://github.com/w3c/csswg-drafts/commit/5b5db39d21f3658ae2f4d7992daaf822aca178d8 So we need to update our flexbox impl for that...
,
Dec 20 2017
I've just seen there's a WPT test verifying this behavior: http://w3c-test.org/css/css-flexbox/percentage-heights-003.html
,
Dec 28 2017
Friendly ping to get an update on this issue as it is marked as stable blocker. Thanks..!
,
Dec 28 2017
Given the reset spec-change this should not be considered a stable-blocker. Updating status and assigning to our resident flexbox expert.
,
Jan 16 2018
Have you had a chance to look into this yet Christian?
,
Jan 31 2018
,
Mar 14 2018
,
Jul 23
I have a scenario where setting "flex-basis: 0" doesn't seem to help if the grid's immediate "display: flex" parent has a "min-height": https://stackoverflow.com/questions/51445055/why-does-setting-a-min-height-on-this-flex-element-cause-its-grid-to-collapse https://jsfiddle.net/bom7r0sy/14/ Would be great to hear any workarounds.
,
Sep 4
,
Sep 24
,
Sep 24
bengelliott: Your issue is likely unrelated. I haven't fully debugged it but I suspect your code was relying on min-height: auto previously. You may need a height: 100% on the flexbox or something
,
Sep 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d572ce1da8fb90cdbab05eb3d2902af0c0a97d07 commit d572ce1da8fb90cdbab05eb3d2902af0c0a97d07 Author: Christian Biesinger <cbiesinger@chromium.org> Date: Mon Sep 24 22:05:42 2018 [css-flex] Update to newer spec about definite flex item sizes Implements this change: https://github.com/w3c/csswg-drafts/commit/5b5db39d21f3658ae2f4d7992daaf822aca178d8 external/wpt/css/css-flexbox/percentage-heights-003.html ostensibly tests this, but I don't think the test is correct (and we don't pass it) TESTED=css3/flexbox/definite-main-size.html Bug: 784059 Change-Id: I8ee0ee797b54a8166849ab6e9b9f019b9e43760b Reviewed-on: https://chromium-review.googlesource.com/1240871 Commit-Queue: Christian Biesinger <cbiesinger@chromium.org> Commit-Queue: Emil A Eklund <eae@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#593707} [modify] https://crrev.com/d572ce1da8fb90cdbab05eb3d2902af0c0a97d07/third_party/WebKit/LayoutTests/css3/flexbox/definite-main-size.html [modify] https://crrev.com/d572ce1da8fb90cdbab05eb3d2902af0c0a97d07/third_party/blink/renderer/core/layout/layout_flexible_box.cc
,
Sep 24
,
Sep 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/82ef33772f758883cda6379610b24f27e7ff3bbe commit 82ef33772f758883cda6379610b24f27e7ff3bbe Author: Christian Biesinger <cbiesinger@chromium.org> Date: Wed Sep 26 18:30:55 2018 Revert "[css-flex] Update to newer spec about definite flex item sizes" This reverts commit d572ce1da8fb90cdbab05eb3d2902af0c0a97d07. Reason for revert: Caused regression crbug.com/889435 Original change's description: > [css-flex] Update to newer spec about definite flex item sizes > > Implements this change: > https://github.com/w3c/csswg-drafts/commit/5b5db39d21f3658ae2f4d7992daaf822aca178d8 > > external/wpt/css/css-flexbox/percentage-heights-003.html ostensibly tests > this, but I don't think the test is correct (and we don't pass it) > > TESTED=css3/flexbox/definite-main-size.html > > Bug: 784059 > Change-Id: I8ee0ee797b54a8166849ab6e9b9f019b9e43760b > Reviewed-on: https://chromium-review.googlesource.com/1240871 > Commit-Queue: Christian Biesinger <cbiesinger@chromium.org> > Commit-Queue: Emil A Eklund <eae@chromium.org> > Reviewed-by: Emil A Eklund <eae@chromium.org> > Cr-Commit-Position: refs/heads/master@{#593707} TBR=cbiesinger@chromium.org,dgrogan@chromium.org,eae@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 784059 , 889435 Change-Id: Ia74fb5cf500aebcb876672497cce5295b0959b43 Reviewed-on: https://chromium-review.googlesource.com/1246291 Commit-Queue: Christian Biesinger <cbiesinger@chromium.org> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org> Cr-Commit-Position: refs/heads/master@{#594399} [modify] https://crrev.com/82ef33772f758883cda6379610b24f27e7ff3bbe/third_party/WebKit/LayoutTests/css3/flexbox/definite-main-size.html [modify] https://crrev.com/82ef33772f758883cda6379610b24f27e7ff3bbe/third_party/blink/renderer/core/layout/layout_flexible_box.cc
,
Sep 26
Patch was reverted due to a regression
,
Sep 26
,
Sep 27
Issue 707568 has been merged into this issue.
,
Sep 27
,
Sep 27
Issue 847152 has been merged into this issue.
,
Oct 29
Issue 899261 has been merged into this issue.
,
Nov 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d519ce9b1818139c082ff339c7c5d3af18b1baa7 commit d519ce9b1818139c082ff339c7c5d3af18b1baa7 Author: Christian Biesinger <cbiesinger@chromium.org> Date: Tue Nov 06 01:18:25 2018 Reland "[css-flex] Update to newer spec about definite flex item sizes" This reverts commit 82ef33772f758883cda6379610b24f27e7ff3bbe. Implements this change: https://github.com/w3c/csswg-drafts/commit/5b5db39d21f3658ae2f4d7992daaf822aca178d8 external/wpt/css/css-flexbox/percentage-heights-003.html ostensibly tests this, but I don't think the test is correct (and we don't pass it) To fix the regression from the original change, I updated the devtools code to specify a flex-basis of auto. It previously defaulted to 0%, which used to be resolved to auto, but with this change resolves to 0px, which is not what the code wants. TESTED=css3/flexbox/definite-main-size.html Bug: 784059 , 900459 Change-Id: I773877f34b281dd6bfe4ac02b9aad90451c3acf9 Reviewed-on: https://chromium-review.googlesource.com/c/1247184 Reviewed-by: Joel Einbinder <einbinder@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Christian Biesinger <cbiesinger@chromium.org> Cr-Commit-Position: refs/heads/master@{#605553} [modify] https://crrev.com/d519ce9b1818139c082ff339c7c5d3af18b1baa7/third_party/WebKit/LayoutTests/css3/flexbox/definite-main-size.html [modify] https://crrev.com/d519ce9b1818139c082ff339c7c5d3af18b1baa7/third_party/blink/renderer/core/layout/layout_flexible_box.cc [modify] https://crrev.com/d519ce9b1818139c082ff339c7c5d3af18b1baa7/third_party/blink/renderer/devtools/front_end/animation/animationTimeline.css [modify] https://crrev.com/d519ce9b1818139c082ff339c7c5d3af18b1baa7/third_party/blink/renderer/devtools/front_end/coverage/coverageView.css [modify] https://crrev.com/d519ce9b1818139c082ff339c7c5d3af18b1baa7/third_party/blink/renderer/devtools/front_end/devtools_compatibility.js [modify] https://crrev.com/d519ce9b1818139c082ff339c7c5d3af18b1baa7/third_party/blink/renderer/devtools/front_end/ui/treeoutline.css
,
Nov 6
,
Nov 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4a7d0521fee1786cd634d0ad909a245dd962ed8c commit 4a7d0521fee1786cd634d0ad909a245dd962ed8c Author: Christian Biesinger <cbiesinger@chromium.org> Date: Tue Nov 06 19:01:50 2018 [css-flex] percentage-heights-003 now passes I should've removed this line in crrev.com/c/1247184 R=dgrogan@chromium.org Bug: 784059 Change-Id: Iad283f72794abce0dd46835afed1a90c77e0fc91 Reviewed-on: https://chromium-review.googlesource.com/c/1320213 Commit-Queue: Christian Biesinger <cbiesinger@chromium.org> Commit-Queue: David Grogan <dgrogan@chromium.org> Reviewed-by: David Grogan <dgrogan@chromium.org> Cr-Commit-Position: refs/heads/master@{#605767} [modify] https://crrev.com/4a7d0521fee1786cd634d0ad909a245dd962ed8c/third_party/WebKit/LayoutTests/TestExpectations |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by kkaluri@chromium.org
, Nov 13 2017Labels: M-64 Needs-Milestone OS-Linux OS-Mac
Status: Untriaged (was: Unconfirmed)
99 KB
99 KB View Download