Bug 220582

Summary: Handle min-width/min-height:auto for aspect-ratio
Product: WebKit Reporter: Rob Buis <rbuis>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, esprehn+autocc, ews-watchlist, glenn, koivisto, kondapallykalyan, pdr, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
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

Description Rob Buis 2021-01-13 01:50:36 PST 
Handle min-width/min-height:auto for aspect-ratio.
Comment 1 Rob Buis 2021-01-13 01:53:36 PST 
Created attachment 417516 [details]
Patch
Comment 2 Rob Buis 2021-01-17 12:19:13 PST 
Created attachment 417781 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2021-01-20 01:51:12 PST 
<rdar://problem/73395736>
Comment 4 Rob Buis 2021-01-20 07:58:27 PST 
Created attachment 417968 [details]
Patch
Comment 5 Rob Buis 2021-01-20 13:06:02 PST 
Created attachment 417992 [details]
Patch
Comment 6 Rob Buis 2021-01-27 06:46:01 PST 
Created attachment 418541 [details]
Patch
Comment 7 Rob Buis 2021-02-04 03:48:26 PST 
Created attachment 419265 [details]
Patch
Comment 8 Rob Buis 2021-02-09 00:46:55 PST 
Created attachment 419685 [details]
Patch
Comment 9 Antti Koivisto 2021-02-11 01:14:20 PST 
Comment on attachment 419685 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:665
> +    Length minLength = styleToUse.logicalMinWidth();

could use auto

> Source/WebCore/rendering/RenderBox.cpp:3810
> +    if (widthType == MinSize && logicalWidth.isAuto()) {
> +        if (shouldComputeLogicalWidthFromAspectRatio()) {
> +            LayoutUnit minLogicalWidth;
> +            LayoutUnit maxLogicalWidth;
> +            computeIntrinsicLogicalWidths(minLogicalWidth, maxLogicalWidth);
> +            logicalWidth = Length(minLogicalWidth, Fixed);
> +        } else
> +            logicalWidth = Length(0, Fixed);
> +    } else if (widthType == MainOrPreferredSize && logicalWidth.isAuto() && shouldComputeLogicalWidthFromAspectRatio())
>          logicalWidth = Length(computeLogicalWidthFromAspectRatio(), Fixed);
>      else if (logicalWidth.isIntrinsic())
>          logicalWidth = Length(computeIntrinsicLogicalWidthUsing(logicalWidth, containerLogicalWidth, bordersPlusPadding) - bordersPlusPadding, Fixed);

This sort of code can often made read better by using lambdas and early returns.
Comment 10 Antti Koivisto 2021-02-11 01:15:58 PST 
Comment on attachment 419685 [details]
Patch

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

> LayoutTests/TestExpectations:4421
> +webkit.org/b/214463 imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/flex-aspect-ratio-008.html [ ImageOnlyFailure ]

Why does this break?
Comment 11 Rob Buis 2021-02-11 01:19:03 PST 
Comment on attachment 419685 [details]
Patch

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

>> LayoutTests/TestExpectations:4421
>> +webkit.org/b/214463 imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/flex-aspect-ratio-008.html [ ImageOnlyFailure ]
> 
> Why does this break?

Flex support still needs to land:
https://bugs.webkit.org/show_bug.cgi?id=219679
I am waiting on an Igalia colleague for some flexbox fixes so that patch can be rebased and made smaller.
Comment 12 Rob Buis 2021-02-11 01:54:31 PST 
Created attachment 419966 [details]
Patch
Comment 13 EWS 2021-02-11 02:55:25 PST 
Committed r272718: <https://commits.webkit.org/r272718>

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