Summary: | [CSS-grid] Ignore the aspect-ratio of a replaced element if stretch alignments are applied to both axes | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zsun | ||||||||||||
Component: | CSS | Assignee: | zsun | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | changseok, esprehn+autocc, ews-watchlist, glenn, jfernandez, kondapallykalyan, obrufau, pdr, rbuis, rego, svillar, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
zsun
2021-07-01 01:16:19 PDT
Created attachment 432671 [details]
Patch
Informal LGTM. Comment on attachment 432671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432671&action=review > Source/WebCore/ChangeLog:12 > + This CL only fixed the case when both axes have stretch alignments applied and there is no auto margin(s) Perhaps it'd be better to add our own description of the change. > Source/WebCore/rendering/RenderBox.cpp:2635 > + } else if (shouldComputeLogicalWidthFromAspectRatio() && style().logicalWidth().isAuto() && (!isGridItem() || !hasStretchedLogicalWidth() || !hasStretchedLogicalHeight())) { I'm not sure this is the right approach. I know in Chromium there are these clauses as well to added the case of grid items with stretched size in any size to the ones that should ignore aspect ratio. However, in Chromium we also tried to do it in shouldComputeLogicalWidthFromAspectRatio, which is called in other functions of this class and seems a more appropriated candidate for this logic. Had you consider that approach already ? Created attachment 432925 [details]
Patch
Created attachment 432930 [details]
Patch
Comment on attachment 432930 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432930&action=review > Source/WebCore/rendering/RenderBox.cpp:2609 > + treatAsReplaced = treatAsReplaced && (!isGridItem() || (!hasStretchedLogicalWidth() && !shouldComputeLogicalWidthFromAspectRatio())); I still have doubts about this change; sorry, because I suggested this approach in my last review, but I think we need to study it a bit more before we can land it. First of all, aren't hasStretchedLogicalWidth and shouldComputeLogicalWidthFromAspectRatio clauses redundant ? I mean, inside the second one, we are already checking out whether we are using 'stretch' as the value for the justify-self property. As a matter of fact, the current implementation of shouldComputeLogicalWidthFromAspectRatio regarding how we are checking the justify-self property for 'stretch' is not totally correct. We should also consider the case of justify-items: stretch | normal in the containing block, when this RenderBox instance's justify-self property has 'auto' for its value. This is precisely why hasStretchedLogicalWidth() uses style.resolvedJustifySelf to determine whether the size is being stretched or not. Finally, and this is why I asked sorry in the first place, I think we are mixing now the concepts of 'treatAsReplaced' vs 'compute-size-from-aspect-ratio'. I assumed in my last review that they were equivalent, but looking a the code (some lines after this) it seems we have different branches for these 2 concepts. > Source/WebCore/rendering/RenderGrid.cpp:1870 > + if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().alignSelf().position() != ItemPosition::Stretch) { I think we should use resolveAlingSelf instead (as I commented before) > Source/WebCore/rendering/RenderGrid.cpp:1874 > + } else if (child.style().justifySelf().position() != ItemPosition::Stretch) { I think we should use resolveJustifySelf instead. > Source/WebCore/rendering/RenderGrid.cpp:1886 > + if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().justifySelf().position() != ItemPosition::Stretch) { ditto > Source/WebCore/rendering/RenderGrid.cpp:1891 > + } else if (child.style().alignSelf().position() != ItemPosition::Stretch) { ditto Created attachment 433129 [details]
Patch
Created attachment 433576 [details]
Patch
Comment on attachment 433576 [details]
Patch
r=me
Committed r280022 (239764@main): <https://commits.webkit.org/239764@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433576 [details]. |