Bug 243321 – [css-aspect-ratio] The transferred min/max sizes should be constrained by defined sizes

• Component: CSS
• Status: RESOLVED FIXED

[cathiechen]

The transferred min/max sizes should be constrained by defined sizes.
https://www.w3.org/TR/css-sizing-4/#aspect-ratio-size-transfers

[cathiechen]

Created attachment 461283 [details]
WIP-patch

WIP Patch.

[Rob Buis]

Comment on attachment 461283 [details]
WIP-patch

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

> Source/WebCore/rendering/RenderBox.cpp:5576
> +    if (!style().hasAspectRatio())

Can this happen? Should it be an ASSERT?

[Rob Buis]

Comment on attachment 461283 [details]
WIP-patch

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

> Source/WebCore/rendering/RenderBox.cpp:5557
>  std::pair<LayoutUnit, LayoutUnit> RenderBox::computeMinMaxLogicalWidthFromAspectRatio() const

I think this can be merged with computeMinMaxLogicalHeightFromAspectRatio.

> Source/WebCore/rendering/RenderBox.cpp:5559
> +    if (!style().hasAspectRatio())

Can this happen? Should it be an ASSERT?

[cathiechen]

Comment on attachment 461283 [details]
WIP-patch

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

>> Source/WebCore/rendering/RenderBox.cpp:5559
>> +    if (!style().hasAspectRatio())
> 
> Can this happen? Should it be an ASSERT?

Done, thanks

>> Source/WebCore/rendering/RenderBox.cpp:5576
>> +    if (!style().hasAspectRatio())
> 
> Can this happen? Should it be an ASSERT?

Done

[cathiechen]

Created attachment 461333 [details]
WIP-patch

[cathiechen]

Created attachment 461335 [details]
WIP-patch

[Rob Buis]

Comment on attachment 461335 [details]
WIP-patch

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

> LayoutTests/TestExpectations:-4602
> -webkit.org/b/214463 imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/block-aspect-ratio-045.html [ ImageOnlyFailure ]

Possibly it fixes select-element-001.html (on Mac).

> Source/WebCore/rendering/RenderBox.cpp:679
> +        return;

Should this be an ASSERT too?

> Source/WebCore/rendering/RenderBox.cpp:693
> +    auto shouldCheckTransferredSizes = [&](const Length& dependedLength, const Length& determinedLength) {

dependend*

> Source/WebCore/rendering/RenderBox.h:726
> +    enum class ConstrainWidthOrHeight { Width, Height };

This seems a bit superfluous. Something like ConstrainDimension { Width, Height }; ?

[cathiechen]

Comment on attachment 461335 [details]
WIP-patch

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

>> LayoutTests/TestExpectations:-4602
>> -webkit.org/b/214463 imported/w3c/web-platform-tests/css/css-sizing/aspect-ratio/block-aspect-ratio-045.html [ ImageOnlyFailure ]
> 
> Possibly it fixes select-element-001.html (on Mac).

Yes, removed.

>> Source/WebCore/rendering/RenderBox.cpp:679
>> +        return;
> 
> Should this be an ASSERT too?

Done

>> Source/WebCore/rendering/RenderBox.cpp:693
>> +    auto shouldCheckTransferredSizes = [&](const Length& dependedLength, const Length& determinedLength) {
> 
> dependend*

Done, thanks

>> Source/WebCore/rendering/RenderBox.h:726
>> +    enum class ConstrainWidthOrHeight { Width, Height };
> 
> This seems a bit superfluous. Something like ConstrainDimension { Width, Height }; ?

Done, thanks!

[cathiechen]

Created attachment 461336 [details]
WIP-patch

[Rob Buis]

Comment on attachment 461335 [details]
WIP-patch

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

> Source/WebCore/rendering/RenderBox.cpp:744
> +    return std::clamp(logicalWidth, computedMinWidth, computedMaxWidth);

Here we should take care to optimize the non preferred aspect ratio case, which should be the more common one.

> Source/WebCore/rendering/RenderBox.cpp:766
> +    LayoutUnit MaxH = computedLogicalMaxHeight ? computedLogicalMaxHeight.value() : LayoutUnit::max();

Variable names should not start with capitals.

> Source/WebCore/rendering/RenderBox.cpp:767
> +    LayoutUnit MinH = computedLogicalMinHeight ? computedLogicalMinHeight.value() : LayoutUnit();

Ditto.

> Source/WebCore/rendering/RenderBox.cpp:770
> +    return std::clamp(logicalHeight, MinH, MaxH);

See above.

[cathiechen]

Pull request: https://github.com/WebKit/WebKit/pull/2952

[Radar WebKit Bug Importer]

<rdar://problem/98152982>

[EWS]

Committed 253262@main (94a7be7787d3): <https://commits.webkit.org/253262@main>

Reviewed commits have been landed. Closing PR #2952 and removing active labels.