Bug 219679 – Support aspect-ratio on flexbox items

• Component: CSS
• Status: RESOLVED FIXED

[Rob Buis]

Support aspect-ratio on flexbox children.

[Rob Buis]

Created attachment 415748 [details]
Patch

[Rob Buis]

Created attachment 415761 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/72382311>

[Rob Buis]

Created attachment 417080 [details]
Patch

[Rob Buis]

Created attachment 417261 [details]
Patch

[Rob Buis]

Created attachment 417841 [details]
Patch

[Rob Buis]

Created attachment 417966 [details]
Patch

[Rob Buis]

Created attachment 417971 [details]
Patch

[Rob Buis]

Created attachment 418553 [details]
Patch

[Rob Buis]

Created attachment 418734 [details]
Patch

[Rob Buis]

Created attachment 420005 [details]
Patch

[Rob Buis]

Created attachment 420456 [details]
Patch

[Rob Buis]

Created attachment 420624 [details]
Patch

[Rob Buis]

Created attachment 420628 [details]
Patch

[Rob Buis]

Created attachment 420814 [details]
Patch

[Rob Buis]

Created attachment 420937 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 420937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420937&action=review

Looking great, just one question

> Source/WebCore/rendering/RenderBox.cpp:682
> +    if (logicalMinHeight.isAuto() && shouldComputeLogicalHeightFromAspectRatio() && intrinsicContentHeight && intrinsicContentHeight != LayoutUnit::max() && styleToUse.overflowBlockDirection() == Overflow::Visible)

In which cases intrinsicContentHeight is max() ? Shouldn't it just use nullopt to mean an indefinite size? Looks like we're kind of duplicating checks

[Rob Buis]

Comment on attachment 420937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420937&action=review

>> Source/WebCore/rendering/RenderBox.cpp:682
>> +    if (logicalMinHeight.isAuto() && shouldComputeLogicalHeightFromAspectRatio() && intrinsicContentHeight && intrinsicContentHeight != LayoutUnit::max() && styleToUse.overflowBlockDirection() == Overflow::Visible)
> 
> In which cases intrinsicContentHeight is max() ? Shouldn't it just use nullopt to mean an indefinite size? Looks like we're kind of duplicating checks

This is initiated by RenderFlexibleBox::layoutFlexItems calling mainAxisContentExtent(LayoutUnit::max()). You have a great point however looking at the code I think it will be difficult to introduce Optional here.

[Rob Buis]

Created attachment 420958 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 420958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420958&action=review

Awesome addition! Just one nit.

> Source/WebCore/rendering/RenderBox.cpp:2947
> +            if (computedValues.m_extent != LayoutUnit::max())

Mind adding a comment here explaining that using max() is the way callers have to tell this method to use a nullopt intrinsicHeight? Maybe even pointing to the RenderFlexibleBox code.

Code should be self-explanatory but in this case the context is not enough and I think it pays off to add a comment and ease the job of future reviewers :)

> Source/WebCore/rendering/RenderFlexibleBox.cpp:550
> +}

I guess it's OK to have this here now. I hope we could eventually add the style().hasAspectRatio() check to the RenderBox::hasAspectRatio() in the future, but that's a far more intrusive change indeed.

[Rob Buis]

Created attachment 420979 [details]
Patch

[EWS]

Committed r273193: <https://commits.webkit.org/r273193>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420979 [details].