The transferred min/max sizes should be constrained by defined sizes. https://www.w3.org/TR/css-sizing-4/#aspect-ratio-size-transfers
Created attachment 461283 [details] WIP-patch WIP Patch.
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?
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?
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
Created attachment 461333 [details] WIP-patch
Created attachment 461335 [details] WIP-patch
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 }; ?
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!
Created attachment 461336 [details] WIP-patch
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.
Pull request: https://github.com/WebKit/WebKit/pull/2952
<rdar://problem/98152982>
Committed 253262@main (94a7be7787d3): <https://commits.webkit.org/253262@main> Reviewed commits have been landed. Closing PR #2952 and removing active labels.