WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(17.27 KB, patch)
2020-09-02 04:26 PDT
,
Sergio Villar Senin
dbates
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Carlos Alberto Lopez Perez
Comment 1
2020-04-06 18:56:35 PDT
Likely also causing issues on WPT test
https://wpt.live/css/css-flexbox/flex-minimum-height-flex-items-013.html
Sergio Villar Senin
Comment 2
2020-09-01 13:09:23 PDT
Created
attachment 407707
[details]
Patch
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
Created
attachment 407755
[details]
Patch
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
Committed
r266695
: <
https://trac.webkit.org/changeset/266695
>
Radar WebKit Bug Importer
Comment 10
2020-09-07 02:21:18 PDT
<
rdar://problem/68453730
>
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.
Top of Page
Format For Printing
XML
Clone This Bug