RESOLVED FIXED 210089
[css-flexbox] min-height: auto not applied to nested flexboxes.
https://bugs.webkit.org/show_bug.cgi?id=210089
Summary [css-flexbox] min-height: auto not applied to nested flexboxes.
Carlos Alberto Lopez Perez
Reported 2020-04-06 18:50:05 PDT
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 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 This is tested also by WPT test https://wpt.live/css/css-flexbox/flex-minimum-height-flex-items-011.xht This issue has been originally reported (and fixed) for Chrome at https://crbug.com/596743
Attachments
Patch (12.43 KB, patch)
2020-09-01 13:09 PDT, Sergio Villar Senin
no flags
Patch (17.27 KB, patch)
2020-09-02 04:26 PDT, Sergio Villar Senin
dbates: review+
Carlos Alberto Lopez Perez
Comment 1 2020-04-06 18:56:35 PDT
Sergio Villar Senin
Comment 2 2020-09-01 13:09:23 PDT
Javier Fernandez
Comment 3 2020-09-01 14:18:04 PDT
Comment on attachment 407707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407707&action=review It seems the bots detected 2 failures; at least one seems related to this change. > Source/WebCore/rendering/RenderFlexibleBox.cpp:932 > + // definiteness might be incorrect. Nit: maybe avoid line wrapping in this comment. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1248 > + // If this condition is true, then computeMainAxisExtentForChild will call Nit: Could we reduce the line-wrapping in this comment ? > Source/WebCore/rendering/RenderFlexibleBox.cpp:1590 > + if (!mainAxisLengthIsDefinite(child, childFlexBasis) || childMinSize.isIntrinsic() || childMaxSize.isIntrinsic()) The childFlexBasis variable is only used as argument of mainAxisLengthIsDefinite; Have you considered calling the function directly ? If we move this conditional clause to the end, we may even avoid the call at all.
Darin Adler
Comment 4 2020-09-01 15:08:46 PDT
Comment on attachment 407707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407707&action=review > Source/WebCore/rendering/RenderFlexibleBox.cpp:444 > + auto min = isHorizontalFlow() ? child.style().minWidth(): child.style().minHeight(); Missing space before ":". > Source/WebCore/rendering/RenderFlexibleBox.cpp:449 > + > + if (!min.isAuto()) > + return false; > + > + return mainAxisOverflowForChild(child) == Overflow::Visible; Not sure others agree, but I would write this as a one liner: return min.isAuto() && mainAxisOverflowForChild(child) == Overflow::Visible;
Sergio Villar Senin
Comment 5 2020-09-02 04:26:26 PDT
Sergio Villar Senin
Comment 6 2020-09-02 04:30:49 PDT
Daniel, could you please take a look at the updated expectations? Are they correct? I am not sure since this is a mac/iOS specific styling stuff.
Daniel Bates
Comment 7 2020-09-05 14:36:00 PDT
Comment on attachment 407755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407755&action=review > LayoutTests/platform/ios/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt:51 > - RenderBlock {DIV} at (0,12) size 20x14 [color=#000000CC] > + RenderBlock {DIV} at (0,12) size 95x14 [color=#000000CC] This may be OK because I think there is clipping + I disable scrolling, but a picture would allow this to be confirmed immediately. Same response for the changes below.
Daniel Bates
Comment 8 2020-09-05 14:54:12 PDT
Comment on attachment 407755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407755&action=review Patch looks good. Optional, check the spelling of the ChangeLog and in comments check for sentence capitalization. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1581 > + Length childMinSize = isHorizontalFlow() ? child.style().minWidth() : child.style().minHeight(); > + Length childMaxSize = isHorizontalFlow() ? child.style().maxWidth() : child.style().maxHeight(); OK as-is. No need to change anything. Just for me: future cleanup idea; just noticed the same result can be achieved using one branch. Also these locals could be moved after the !mainAxisLengthIsDefinite branch since they aren't used if that evaluates to true. > Source/WebCore/rendering/RenderFlexibleBox.h:119 > Length flexBasisForChild(const RenderBox& child) const; > + bool shouldApplyMinSizeAutoForChild(const RenderBox& child) const; > LayoutUnit crossAxisExtentForChild(const RenderBox& child) const; > LayoutUnit crossAxisIntrinsicExtentForChild(const RenderBox& child) const; > LayoutUnit childIntrinsicLogicalHeight(const RenderBox& child) const; OK as-is. No change needed. Just for me: future cleanup idea; the parameter name can be removed because its purpose is encoded in the names of these functions.
Sergio Villar Senin
Comment 9 2020-09-07 02:20:55 PDT
Radar WebKit Bug Importer
Comment 10 2020-09-07 02:21:18 PDT
Daniel Bates
Comment 11 2020-09-07 07:13:36 PDT
(In reply to Daniel Bates from comment #7) > Comment on attachment 407755 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407755&action=review > > > LayoutTests/platform/ios/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt:51 > > - RenderBlock {DIV} at (0,12) size 20x14 [color=#000000CC] > > + RenderBlock {DIV} at (0,12) size 95x14 [color=#000000CC] > > This may be OK because I think there is clipping + I disable scrolling, but > a picture would allow this to be confirmed immediately. Same response for > the changes below. I never checked this. Can a picture be obtained?
Sergio Villar Senin
Comment 12 2020-09-07 09:48:44 PDT
(In reply to Daniel Bates from comment #11) > (In reply to Daniel Bates from comment #7) > > Comment on attachment 407755 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=407755&action=review > > > > > LayoutTests/platform/ios/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt:51 > > > - RenderBlock {DIV} at (0,12) size 20x14 [color=#000000CC] > > > + RenderBlock {DIV} at (0,12) size 95x14 [color=#000000CC] > > > > This may be OK because I think there is clipping + I disable scrolling, but > > a picture would allow this to be confirmed immediately. Same response for > > the changes below. > > I never checked this. Can a picture be obtained? Oh I missunderstood your comment, I thought you've already checked it. I don't have a mac available ATM, maybe you could try to get it?
Darin Adler
Comment 13 2020-09-07 09:51:22 PDT
Comment on attachment 407755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407755&action=review >>>> LayoutTests/platform/ios/fast/forms/auto-fill-button/input-strong-password-auto-fill-button-expected.txt:51 >>>> + RenderBlock {DIV} at (0,12) size 95x14 [color=#000000CC] >>> >>> This may be OK because I think there is clipping + I disable scrolling, but a picture would allow this to be confirmed immediately. Same response for the changes below. >> >> I never checked this. Can a picture be obtained? > > Oh I missunderstood your comment, I thought you've already checked it. I don't have a mac available ATM, maybe you could try to get it? Yes, I checked what it looks like. Not a problem.
Simon Fraser (smfr)
Comment 14 2020-12-14 21:25:44 PST
Before this fix, pages on YouTube.com with comments would get nested scrollers because <ytd-page-manager id=“page-manager”> became independently scrollable. This fixed that.
zalan
Comment 15 2021-01-13 16:17:27 PST
This regressed input type range, see bug 220608.
Simon Fraser (smfr)
Comment 16 2021-02-20 11:13:56 PST
This caused exponential runtime cost with nested flexboxes which affects twitch.tv: r222202
Note You need to log in before you can comment on or make changes to this bug.