Bug 243209 – Fix aspect-ratio/flex-aspect-ratio-031.html

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

Fix aspect-ratio/flex-aspect-ratio-031.html.

[Rob Buis]

Created attachment 461220 [details]
Patch

[Rob Buis]

Created attachment 461224 [details]
Patch

[Rob Buis]

Created attachment 461247 [details]
Patch

[Darin Adler]

Comment on attachment 461247 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:930
> +        std::optional<LayoutUnit> childSize = mainAxisIsChildInlineAxis(child) ? child.computePercentageLogicalHeight(crossSizeLength) : adjustBorderBoxLogicalWidthForBoxSizing(valueForLength(crossSizeLength, contentWidth()), crossSizeLength.type());

Slightly better if we just use auto here, I think.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:947
> +            if (child.style().boxSizingForAspectRatio() == BoxSizing::ContentBox)
> +                crossSize -= isHorizontalFlow() ? child.verticalBorderAndPaddingExtent() : child.horizontalBorderAndPaddingExtent();
> +            else
> +                borderAndPadding = isHorizontalFlow() ? child.horizontalBorderAndPaddingExtent() : child.verticalBorderAndPaddingExtent();

Do we have enough test cases that we know we got all 4 of these cases right?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:955
> +        return std::max(LayoutUnit(), LayoutUnit(crossSize * ratio) - borderAndPadding);

Could consider 0_lu instead of LayoutUnit().

[Rob Buis]

Created attachment 461291 [details]
Patch

[Rob Buis]

Comment on attachment 461247 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:930
>> +        std::optional<LayoutUnit> childSize = mainAxisIsChildInlineAxis(child) ? child.computePercentageLogicalHeight(crossSizeLength) : adjustBorderBoxLogicalWidthForBoxSizing(valueForLength(crossSizeLength, contentWidth()), crossSizeLength.type());
> 
> Slightly better if we just use auto here, I think.

Done.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:947
>> +                borderAndPadding = isHorizontalFlow() ? child.horizontalBorderAndPaddingExtent() : child.verticalBorderAndPaddingExtent();
> 
> Do we have enough test cases that we know we got all 4 of these cases right?

Yes, this is tested by flex-aspect-ratio-025.html and flex-aspect-ratio-026.html. Sadly although this patch improves those tests more work is needed to make them fully pass.

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:955
>> +        return std::max(LayoutUnit(), LayoutUnit(crossSize * ratio) - borderAndPadding);
> 
> Could consider 0_lu instead of LayoutUnit().

Done.

[EWS]

Committed 252949@main (e1869c79a82b): <https://commits.webkit.org/252949@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/97787509>