Bug 210089

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