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

Issue 596743 link

Starred by 40 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug

Blocked on:
issue 581553
issue 892757

Blocking:
issue 487302



Sign in to add a comment

min-height: auto not applied to nested flexboxes.

Reported by utasirob...@gmail.com, Mar 22 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36

Steps to reproduce the problem:
1. open testcase https://jsfiddle.net/utasir/ow9e4ffb/
2. sroll down and see the first 2 items are only have height of grandparent and not flexed 
3. 

What is the expected behavior?
all of 3 items in the subflex are equal height

What went wrong?
flex growing

Did this work before? N/A 

Chrome version: 49.0.2623.87  Channel: canary
OS Version: 10.0
Flash Version: Shockwave Flash 21.0 r0

This is a cross-browser-compatibility issue as well, works o.k. in firefox and Edge
 
Cc: rnimmagadda@chromium.org
Components: Blink>Layout>Scrollbars Blink
Labels: M-50 OS-Linux OS-Mac
Status: Untriaged (was: Unconfirmed)
Able to repro this issue on Windows 7, MAC (10.11.3) & Ubuntu Trusty (14.04) for Google Chrome Stable Version - 49.0.2623.87 

This is a Non-Regression issue existing from M30 - # - 30.0.1549.0
Components: -Blink>Layout>Scrollbars -Blink Blink>Layout>Flexbox
Labels: -Pri-2 Pri-3
Status: Available (was: Untriaged)
Summary: min-height: auto not applied to nested flexboxes. (was: wrong height for subflex when grandparent flexholder has fixed height and overflow:scroll;)
min-height issue :(

Giving min-height: min-content to .wrapper makes this work as expected. Alternatively, flex-shrink: 0.

Summary is incorrect, it's unrelated to scroll. It's just that the container has a fixed height and we currently don't apply min-height to nested flexboxes. So in this case we shrink the inner element due to flex-shrink: 1 (the default), and the reason this works in firefox is because they do apply min-height and therefore don't shrink it.
Cc: nyerramilli@chromium.org cbiesin...@chromium.org tkonch...@chromium.org yosin@chromium.org
 Issue 618578  has been merged into this issue.
Blockedon: 581553
Cc: msrchandra@chromium.org
 Issue 626645  has been merged into this issue.
 Issue 644621  has been merged into this issue.
Cc: rbasuvula@chromium.org
 Issue 656624  has been merged into this issue.
 Issue 718089  has been merged into this issue.
 Issue 737088  has been merged into this issue.
Blocking: 487302
Cc: jmukthavaram@chromium.org
 Issue 676985  has been merged into this issue.
Blocking: -487302
Blocking: 487302
Cc: e...@chromium.org r...@igalia.com tabatkins@chromium.org foolip@chromium.org
 Issue 487302  has been merged into this issue.
Owner: cbiesin...@chromium.org
Status: Assigned (was: Available)
So let me summarize the issue here. We explicitly disable applying min-block-size: auto to a child of a column flexbox that is a flex item:
https://chromium.googlesource.com/chromium/src/+/b883e94b36fe4f3f3f71e24626ed9e0678372085%5E%21/third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp

And the reason is, basically, that we do not correctly dynamic changes. See this patch (description is incorrect, we still fail):
https://chromium-review.googlesource.com/c/chromium/src/+/1246730
We fail css3/flexbox/nested-flexbox-min-size-auto.html

There is also some more context in  bug 580196 

Basically, the issue is that we read an outdated IntrinsicContentLogicalHeight() from the child; so we need to force a layout on the child. Maybe (?) it is enough to do that if we have percent height descendants... I can try that. In general I am very worried about performance impacts of doing extra layouts.
Labels: Hotlist-Interop
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 27

Status: Fixed (was: Assigned)
Fixed for now... we'll see if it sticks
 Issue 811080  has been merged into this issue.
 Issue 876344  has been merged into this issue.
 Issue 866505  has been merged into this issue.
 Issue 707568  has been merged into this issue.
Cc: rbyers@chromium.org
 Issue 775263  has been merged into this issue.
Cc: dgro...@chromium.org mstensho@chromium.org sindhu.chelamcherla@chromium.org
 Issue 829308  has been merged into this issue.
Project Member

Comment 24 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/201a5758d93e85e6c939d84738652b7c4ccfec22

commit 201a5758d93e85e6c939d84738652b7c4ccfec22
Author: Wenzhao (Colin) Zang <wzang@chromium.org>
Date: Fri Sep 28 00:49:05 2018

Revert "[css-flexbox] Apply min-height: auto to nested flexboxes again"

This reverts commit 35b382aa26d503c46963fdd6fa41bffe42cc7074.

Reason for revert:  crbug.com/890100 

Original change's description:
> [css-flexbox] Apply min-height: auto to nested flexboxes again
> 
> To avoid the previous regression (see analysis in bug), we force
> layout in the case where we otherwise would get an outdated result.
> 
> Bug:  596743 
> Change-Id: I9cf47675f7fcd88f94b2fe76b46bceae17e36756
> Reviewed-on: https://chromium-review.googlesource.com/1246730
> Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
> Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#594752}

TBR=cbiesinger@chromium.org,eae@chromium.org,mstensho@chromium.org

Change-Id: I8ef1e6a3e5f191c495c0b36d32366de3f0323a39
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  596743 
Reviewed-on: https://chromium-review.googlesource.com/1250207
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594936}
[modify] https://crrev.com/201a5758d93e85e6c939d84738652b7c4ccfec22/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/201a5758d93e85e6c939d84738652b7c4ccfec22/third_party/WebKit/LayoutTests/css3/flexbox/nested-flexbox-min-size-auto.html
[delete] https://crrev.com/f63f90fb1fe4f149ddf07ecbe7c1c695a1240dbb/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/flex-minimum-height-flex-items-009.html
[modify] https://crrev.com/201a5758d93e85e6c939d84738652b7c4ccfec22/third_party/blink/renderer/core/layout/layout_flexible_box.cc
[modify] https://crrev.com/201a5758d93e85e6c939d84738652b7c4ccfec22/third_party/blink/renderer/core/layout/layout_flexible_box.h

Cc: wzang@chromium.org
Labels: -Pri-3 Pri-2
Status: Available (was: Fixed)
Sadly didn't stick, see  issue 890100 .
Project Member

Comment 27 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c5fd98f7949fb1933193440b202b7c23c3fe1b23

commit c5fd98f7949fb1933193440b202b7c23c3fe1b23
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Fri Sep 28 21:47:56 2018

[css-flexbox] Move a min-size: auto testcase to WPT

This relands just the testcase portion of
https://chromium-review.googlesource.com/c/chromium/src/+/1246730,
so that it won't be reverted in case the patch reland
will be reverted again.

R=eae@chromium.org, mstensho@chromium.org

Bug:  596743 
Change-Id: I7c0d76794d65278c96de7eb650a717e3802bbf97
Reviewed-on: https://chromium-review.googlesource.com/1252681
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595224}
[modify] https://crrev.com/c5fd98f7949fb1933193440b202b7c23c3fe1b23/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/192f116fd51a4f62d6614a420260f72a73d6f89a/third_party/WebKit/LayoutTests/css3/flexbox/nested-flexbox-min-size-auto.html
[add] https://crrev.com/c5fd98f7949fb1933193440b202b7c23c3fe1b23/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/flex-minimum-height-flex-items-009.html

Project Member

Comment 28 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f033b7d2fc1c1a44006a9c30afc864abc9e7571b

commit f033b7d2fc1c1a44006a9c30afc864abc9e7571b
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Fri Sep 28 21:51:14 2018

[css-flexbox] Apply min-height: auto to nested flexboxes again

To avoid the previous regression (see analysis in bug), we force
layout in the case where we otherwise would get an outdated result.

This relands https://chromium-review.googlesource.com/c/chromium/src/+/1246730
with an additional fix for the Chrome UI CSS. I also split out the
test change into its own patch.

IF THIS BREAKS ANY FURTHER CHROME UI:
Please don't revert this patch; instead, add min-height: 0 to any
inner nested flexboxes that may be affected by this patch. This is
an important change for web interop with the other browsers.

Bug:  596743 , 890100 
Change-Id: Ice629c2a7823ef07d075fa23b99022b4012c6200
Reviewed-on: https://chromium-review.googlesource.com/1252682
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595225}
[modify] https://crrev.com/f033b7d2fc1c1a44006a9c30afc864abc9e7571b/chrome/browser/resources/chromeos/wallpaper_manager/css/wallpaper_manager.css
[modify] https://crrev.com/f033b7d2fc1c1a44006a9c30afc864abc9e7571b/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/f033b7d2fc1c1a44006a9c30afc864abc9e7571b/third_party/blink/renderer/core/layout/layout_flexible_box.cc
[modify] https://crrev.com/f033b7d2fc1c1a44006a9c30afc864abc9e7571b/third_party/blink/renderer/core/layout/layout_flexible_box.h

Status: Fixed (was: Available)
Project Member

Comment 30 by bugdroid1@chromium.org, Oct 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58266dadab879824045b0a24816fe30fbb366b34

commit 58266dadab879824045b0a24816fe30fbb366b34
Author: Wenzhao (Colin) Zang <wzang@chromium.org>
Date: Thu Oct 04 17:04:22 2018

Revert "[css-flexbox] Apply min-height: auto to nested flexboxes again"

This reverts commit f033b7d2fc1c1a44006a9c30afc864abc9e7571b.

Reason for revert:  crbug.com/891988 

Original change's description:
> [css-flexbox] Apply min-height: auto to nested flexboxes again
> 
> To avoid the previous regression (see analysis in bug), we force
> layout in the case where we otherwise would get an outdated result.
> 
> This relands https://chromium-review.googlesource.com/c/chromium/src/+/1246730
> with an additional fix for the Chrome UI CSS. I also split out the
> test change into its own patch.
> 
> IF THIS BREAKS ANY FURTHER CHROME UI:
> Please don't revert this patch; instead, add min-height: 0 to any
> inner nested flexboxes that may be affected by this patch. This is
> an important change for web interop with the other browsers.
> 
> Bug:  596743 , 890100 
> Change-Id: Ice629c2a7823ef07d075fa23b99022b4012c6200
> Reviewed-on: https://chromium-review.googlesource.com/1252682
> Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#595225}

TBR=cbiesinger@chromium.org,eae@chromium.org,wzang@chromium.org,mstensho@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  596743 ,  890100 
Change-Id: I8b3ce623a49fe476dbc309b9d780fee80b233c3e
Reviewed-on: https://chromium-review.googlesource.com/c/1261919
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596719}
[modify] https://crrev.com/58266dadab879824045b0a24816fe30fbb366b34/chrome/browser/resources/chromeos/wallpaper_manager/css/wallpaper_manager.css
[modify] https://crrev.com/58266dadab879824045b0a24816fe30fbb366b34/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/58266dadab879824045b0a24816fe30fbb366b34/third_party/blink/renderer/core/layout/layout_flexible_box.cc
[modify] https://crrev.com/58266dadab879824045b0a24816fe30fbb366b34/third_party/blink/renderer/core/layout/layout_flexible_box.h

Blockedon: 892757
Status: Assigned (was: Fixed)
Will retry landing after the M71 branch
Project Member

Comment 33 by bugdroid1@chromium.org, Oct 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/155c1f2bf45369bc7130c89b88c75c2d3c915410

commit 155c1f2bf45369bc7130c89b88c75c2d3c915410
Author: Christian Biesinger <cbiesinger@chromium.org>
Date: Mon Oct 22 12:40:09 2018

[css-flexbox] Apply min-height: auto to nested flexboxes again

This was made possible by crrev.com/c/1283482, which avoids the
performance and correctness issues this previously caused.

This relands crrev.com/c/1246730 / crrev.com/c/1252682. The ChromeOS UI
breakage was fixed by crrev.com/c/1281726 and crrev.com/c/1282462

IF THIS BREAKS ANY FURTHER CHROME UI:
Please don't revert this patch; instead, add min-height: 0 to any
inner nested flexboxes that may be affected by this patch. This is
an important change for web interop with the other browsers.

Bug:  596743 
Change-Id: I9afe54dce82d41da452d1fdca8150ca22ebb6f9c
Reviewed-on: https://chromium-review.googlesource.com/c/1269235
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601540}
[modify] https://crrev.com/155c1f2bf45369bc7130c89b88c75c2d3c915410/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/155c1f2bf45369bc7130c89b88c75c2d3c915410/third_party/WebKit/LayoutTests/external/wpt/css/css-flexbox/flex-minimum-height-flex-items-011.xht
[modify] https://crrev.com/155c1f2bf45369bc7130c89b88c75c2d3c915410/third_party/blink/renderer/core/layout/flexible_box_algorithm.cc

Status: Fixed (was: Assigned)

Sign in to add a comment