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
Likely also causing issues on WPT test https://wpt.live/css/css-flexbox/flex-minimum-height-flex-items-013.html
Created attachment 407707 [details] Patch
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.
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;
Created attachment 407755 [details] Patch
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.
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.
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.
Committed r266695: <https://trac.webkit.org/changeset/266695>
<rdar://problem/68453730>
(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?
(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?
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.
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.
This regressed input type range, see bug 220608.
This caused exponential runtime cost with nested flexboxes which affects twitch.tv: r222202