Bug 243209

Summary: Fix aspect-ratio/flex-aspect-ratio-031.html
Product: WebKit Reporter: Rob Buis <rbuis>
Component: CSSAssignee: Rob Buis <rbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, darin, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, ntim, pdr, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari 15   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Rob Buis
Reported 2022-07-26 05:00:39 PDT
Fix aspect-ratio/flex-aspect-ratio-031.html.
Attachments
Patch (5.64 KB, patch)
2022-07-26 05:06 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (4.83 KB, patch)
2022-07-26 08:25 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (7.67 KB, patch)
2022-07-27 08:57 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Patch (7.63 KB, patch)
2022-07-29 05:42 PDT, Rob Buis 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Rob Buis
Comment 1 2022-07-26 05:06:28 PDT
Created attachment 461220 [details] Patch
Rob Buis
Comment 2 2022-07-26 08:25:10 PDT
Created attachment 461224 [details] Patch
Rob Buis
Comment 3 2022-07-27 08:57:25 PDT
Created attachment 461247 [details] Patch
Darin Adler
Comment 4 2022-07-28 09:54:33 PDT
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
Comment 5 2022-07-29 05:42:49 PDT
Created attachment 461291 [details] Patch
Rob Buis
Comment 6 2022-07-29 05:45:07 PDT
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
Comment 7 2022-07-29 08:51:46 PDT
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
Comment 8 2022-07-29 08:52:16 PDT
<rdar://problem/97787509>
Note You need to log in before you can comment on or make changes to this bug.