Bug 219679

Summary: Support aspect-ratio on flexbox items
Product: WebKit Reporter: Rob Buis <rbuis>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, svillar, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 47738    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Rob Buis 2020-12-09 05:51:00 PST 
Support aspect-ratio on flexbox children.
Comment 1 Rob Buis 2020-12-09 05:52:33 PST 
Created attachment 415748 [details]
Patch
Comment 2 Rob Buis 2020-12-09 08:32:38 PST 
Created attachment 415761 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2020-12-16 05:51:16 PST 
<rdar://problem/72382311>
Comment 4 Rob Buis 2021-01-06 05:25:56 PST 
Created attachment 417080 [details]
Patch
Comment 5 Rob Buis 2021-01-08 04:28:39 PST 
Created attachment 417261 [details]
Patch
Comment 6 Rob Buis 2021-01-18 11:22:06 PST 
Created attachment 417841 [details]
Patch
Comment 7 Rob Buis 2021-01-20 07:13:54 PST 
Created attachment 417966 [details]
Patch
Comment 8 Rob Buis 2021-01-20 08:29:52 PST 
Created attachment 417971 [details]
Patch
Comment 9 Rob Buis 2021-01-27 09:18:09 PST 
Created attachment 418553 [details]
Patch
Comment 10 Rob Buis 2021-01-29 09:03:44 PST 
Created attachment 418734 [details]
Patch
Comment 11 Rob Buis 2021-02-11 10:59:07 PST 
Created attachment 420005 [details]
Patch
Comment 12 Rob Buis 2021-02-16 05:38:40 PST 
Created attachment 420456 [details]
Patch
Comment 13 Rob Buis 2021-02-17 03:32:35 PST 
Created attachment 420624 [details]
Patch
Comment 14 Rob Buis 2021-02-17 04:52:43 PST 
Created attachment 420628 [details]
Patch
Comment 15 Rob Buis 2021-02-18 02:08:45 PST 
Created attachment 420814 [details]
Patch
Comment 16 Rob Buis 2021-02-19 01:00:20 PST 
Created attachment 420937 [details]
Patch
Comment 17 Sergio Villar Senin 2021-02-19 03:26:46 PST 
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
Comment 18 Rob Buis 2021-02-19 05:43:57 PST 
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.
Comment 19 Rob Buis 2021-02-19 05:58:42 PST 
Created attachment 420958 [details]
Patch
Comment 20 Sergio Villar Senin 2021-02-19 07:44:14 PST 
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.
Comment 21 Rob Buis 2021-02-19 09:28:29 PST 
Created attachment 420979 [details]
Patch
Comment 22 EWS 2021-02-19 22:34:11 PST 
Committed r273193: <https://commits.webkit.org/r273193>

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