| Summary: | Fix aspect-ratio/flex-aspect-ratio-031.html | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rob Buis <rbuis> | ||||||||||
| Component: | CSS | Assignee: | 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
Rob Buis
2022-07-26 05:00:39 PDT
Created attachment 461220 [details]
Patch
Created attachment 461224 [details]
Patch
Created attachment 461247 [details]
Patch
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(). Created attachment 461291 [details]
Patch
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. 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]. |