New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 784059 link

Starred by 25 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug


Participants' hotlists:
layout-priority


Sign in to add a comment

[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 description

UserAgent: 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.
 
Cc: kkaluri@chromium.org
Labels: M-64 Needs-Milestone OS-Linux OS-Mac
Status: Untriaged (was: Unconfirmed)
Able to this issue on Windows 10, Ubuntu 14.04 and mac 10.12.6 with chrome Stable #62.0.3202.89, canary #64.0.3267.0 and also in earlier M50-#50.0.2624.0 
This is a non-regression issue, hence marking it as untriaged.
784059.PNG
99 KB View Download

Comment 2 by e...@chromium.org, Nov 13 2017

Cc: cbiesin...@chromium.org jfernan...@igalia.com
Components: -Blink>Layout Blink>Layout>Grid
Status: Available (was: Untriaged)
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.

Comment 4 by r...@igalia.com, Dec 18 2017

Cc: e...@chromium.org r...@igalia.com svil...@igalia.com pbomm...@chromium.org
 Issue 795260  has been merged into this issue.

Comment 5 by r...@igalia.com, 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.

Labels: ReleaseBlock-Stable FoundIn-65 FoundIn-64 Target-64 FoundIn-63

Comment 7 by r...@igalia.com, Dec 19 2017

Components: -Blink>Layout>Grid Blink>Layout>Flexbox
Summary: [css-flex] Flex item is considered to have indefinite height with "flex-basis: auto" despite of "flex-grow: 1" (was: [css-grid] A grid nested in a vertical flexbox gives all its grid items/children an incorrect height of 0)
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.

flexbox-item-indefinite-height.html
396 bytes View Download
flexbox-item-indefinite-height.png
34.2 KB View Download
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...

Comment 9 by r...@igalia.com, 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
Friendly ping to get an update on this issue as it is marked as stable blocker.
Thanks..!

Comment 11 by e...@chromium.org, Dec 28 2017

Labels: -Pri-2 -ReleaseBlock-Stable OS-Android OS-Chrome Pri-1
Owner: cbiesin...@chromium.org
Status: Assigned (was: Available)
Given the reset spec-change this should not be considered a stable-blocker. Updating status and assigning to our resident flexbox expert.

Comment 12 by e...@chromium.org, Jan 16 2018

Have you had a chance to look into this yet Christian? 

Comment 13 by e...@chromium.org, Jan 31 2018

Labels: -Pri-1 Pri-2

Comment 14 by r...@igalia.com, Mar 14 2018

Cc: jfernandez@chromium.org
 Issue 786986  has been merged into this issue.
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.
Cc: cbiesinger@google.com viswa.karala@chromium.org
 Issue 876763  has been merged into this issue.
Cc: -cbiesinger@google.com
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
Project Member

Comment 19 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Project Member

Comment 21 by bugdroid1@chromium.org, 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

Status: Assigned (was: Fixed)
Patch was reverted due to a regression
Cc: loislo@chromium.org dholb...@gmail.com
 Issue 428049  has been merged into this issue.
 Issue 707568  has been merged into this issue.
Cc: susan.boorgula@chromium.org
 Issue 872624  has been merged into this issue.
 Issue 847152  has been merged into this issue.
 Issue 899261  has been merged into this issue.
Project Member

Comment 28 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Project Member

Comment 30 by bugdroid1@chromium.org, 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