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

Rob Buis
Reported 2020-12-09 05:51:00 PST
Support aspect-ratio on flexbox children.
Attachments
Patch (8.52 KB, patch)
2020-12-09 05:52 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (8.53 KB, patch)
2020-12-09 08:32 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (8.74 KB, patch)
2021-01-06 05:25 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (9.70 KB, patch)
2021-01-08 04:28 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.14 KB, patch)
2021-01-18 11:22 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (12.01 KB, patch)
2021-01-20 07:13 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (12.01 KB, patch)
2021-01-20 08:29 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (12.02 KB, patch)
2021-01-27 09:18 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.31 KB, patch)
2021-01-29 09:03 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.79 KB, patch)
2021-02-11 10:59 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (10.86 KB, patch)
2021-02-16 05:38 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.50 KB, patch)
2021-02-17 03:32 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.59 KB, patch)
2021-02-17 04:52 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.36 KB, patch)
2021-02-18 02:08 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.70 KB, patch)
2021-02-19 01:00 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.51 KB, patch)
2021-02-19 05:58 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (11.68 KB, patch)
2021-02-19 09:28 PST, Rob Buis 		no flags
DetailsFormatted DiffDiff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.
Rob Buis
Comment 1 2020-12-09 05:52:33 PST
Created attachment 415748 [details] Patch
Rob Buis
Comment 2 2020-12-09 08:32:38 PST
Created attachment 415761 [details] Patch
Radar WebKit Bug Importer
Comment 3 2020-12-16 05:51:16 PST
<rdar://problem/72382311>
Rob Buis
Comment 4 2021-01-06 05:25:56 PST
Created attachment 417080 [details] Patch
Rob Buis
Comment 5 2021-01-08 04:28:39 PST
Created attachment 417261 [details] Patch
Rob Buis
Comment 6 2021-01-18 11:22:06 PST
Created attachment 417841 [details] Patch
Rob Buis
Comment 7 2021-01-20 07:13:54 PST
Created attachment 417966 [details] Patch
Rob Buis
Comment 8 2021-01-20 08:29:52 PST
Created attachment 417971 [details] Patch
Rob Buis
Comment 9 2021-01-27 09:18:09 PST
Created attachment 418553 [details] Patch
Rob Buis
Comment 10 2021-01-29 09:03:44 PST
Created attachment 418734 [details] Patch
Rob Buis
Comment 11 2021-02-11 10:59:07 PST
Created attachment 420005 [details] Patch
Rob Buis
Comment 12 2021-02-16 05:38:40 PST
Created attachment 420456 [details] Patch
Rob Buis
Comment 13 2021-02-17 03:32:35 PST
Created attachment 420624 [details] Patch
Rob Buis
Comment 14 2021-02-17 04:52:43 PST
Created attachment 420628 [details] Patch
Rob Buis
Comment 15 2021-02-18 02:08:45 PST
Created attachment 420814 [details] Patch
Rob Buis
Comment 16 2021-02-19 01:00:20 PST
Created attachment 420937 [details] Patch
Sergio Villar Senin
Comment 17 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
Rob Buis
Comment 18 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.
Rob Buis
Comment 19 2021-02-19 05:58:42 PST
Created attachment 420958 [details] Patch
Sergio Villar Senin
Comment 20 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.
Rob Buis
Comment 21 2021-02-19 09:28:29 PST
Created attachment 420979 [details] Patch
EWS
Comment 22 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].
Note You need to log in before you can comment on or make changes to this bug.