Fix grid-aspect-ratio-018+19.html.
Created attachment 428816 [details] Patch
Comment on attachment 428816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428816&action=review > Source/WebCore/rendering/RenderBox.cpp:5163 > + if (isGridItem() && style().justifySelf().position() == ItemPosition::Stretch) I don't think this is correct. There are several other cases that we should consider to determine the grid item uses stretch alignment: 1- container: justify-items: stretch | normal; item: justify-self: auto | normal 2- container/item combination resolved to stretch, but width non-auto. Additionally, should we consider here the item's writing-mode, so that we check align-self property instead ? > Source/WebCore/rendering/RenderGrid.cpp:1870 > + if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().alignSelf().position() != ItemPosition::Stretch) { ditto > Source/WebCore/rendering/RenderGrid.cpp:1891 > + } else if (child.style().alignSelf().position() != ItemPosition::Stretch) { ditto
<rdar://problem/78390550>
Created attachment 432866 [details] Patch
Created attachment 433747 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 433777 [details] Patch
Created attachment 433786 [details] Patch
Created attachment 433791 [details] Patch
Comment on attachment 433791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433791&action=review > Source/WebCore/rendering/RenderBox.cpp:2791 > + auto itemPosition = stretchingMode == StretchingMode::Any ? containingBlock->selfAlignmentNormalBehavior(this) : ItemPosition::Normal; Is this variable name accurate ? Shouldn't be something like "normal behavior" ? > Source/WebCore/rendering/RenderGrid.cpp:1140 > + // interested in explicit stretch, not normal behavior. No need to wrap line in this comment. > Source/WebCore/rendering/RenderGrid.cpp:1141 > + bool hasExplicitInlineStretch = child.style().resolvedJustifySelf(&style(), ItemPosition::Normal).position() == ItemPosition::Stretch; Now that we have this StretchingMode enumg, wouldn't be better to use it in the alignSelfForChild to determine the proper normal-behavior ? > Source/WebCore/rendering/RenderGrid.cpp:1142 > + bool hasExplicitBlockStretch = child.style().resolvedAlignSelf(&style(), ItemPosition::Normal).position() == ItemPosition::Stretch; ditto > Source/WebCore/rendering/RenderGrid.cpp:1144 > + std::swap(hasExplicitInlineStretch, hasExplicitBlockStretch); Perhaps we could avoid this swatch if we use the blockFlowIsColumnAxis before, so that inline is associated to justify or align, and viceversa. > Source/WebCore/rendering/RenderGrid.cpp:1145 > + if (hasExplicitInlineStretch && hasExplicitBlockStretch) Couldn't these 3 clause be simplified like this ? return hasExplicitInlineStretch ? hasExplicitBlockStretc : !hasExplicitBlockStretch; > Source/WebCore/rendering/RenderGrid.cpp:1887 > + if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().alignSelf().position() != ItemPosition::Stretch) { Why we don't need to consider align-self: auto here ? > Source/WebCore/rendering/RenderGrid.cpp:1891 > + } else if (child.style().justifySelf().position() != ItemPosition::Stretch) { ditto > Source/WebCore/rendering/RenderGrid.cpp:1903 > + if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().justifySelf().position() != ItemPosition::Stretch) { ditto > Source/WebCore/rendering/RenderGrid.cpp:1908 > + } else if (child.style().alignSelf().position() != ItemPosition::Stretch) { ditto
Created attachment 433855 [details] Patch
Comment on attachment 433791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433791&action=review >> Source/WebCore/rendering/RenderBox.cpp:2791 >> + auto itemPosition = stretchingMode == StretchingMode::Any ? containingBlock->selfAlignmentNormalBehavior(this) : ItemPosition::Normal; > > Is this variable name accurate ? Shouldn't be something like "normal behavior" ? Yes, I had to improve it too in my chromium patch, done. >> Source/WebCore/rendering/RenderGrid.cpp:1140 >> + // interested in explicit stretch, not normal behavior. > > No need to wrap line in this comment. I think we can remove this comment, but point taken (this happens when applying chromium patches :)) >> Source/WebCore/rendering/RenderGrid.cpp:1141 >> + bool hasExplicitInlineStretch = child.style().resolvedJustifySelf(&style(), ItemPosition::Normal).position() == ItemPosition::Stretch; > > Now that we have this StretchingMode enumg, wouldn't be better to use it in the alignSelfForChild to determine the proper normal-behavior ? Done. >> Source/WebCore/rendering/RenderGrid.cpp:1142 >> + bool hasExplicitBlockStretch = child.style().resolvedAlignSelf(&style(), ItemPosition::Normal).position() == ItemPosition::Stretch; > > ditto Done. >> Source/WebCore/rendering/RenderGrid.cpp:1144 >> + std::swap(hasExplicitInlineStretch, hasExplicitBlockStretch); > > Perhaps we could avoid this swatch if we use the blockFlowIsColumnAxis before, so that inline is associated to justify or align, and viceversa. I think row/columns and writing-mode needs more tests which could result in removing the swap, but I prefer to do that in a future patch. >> Source/WebCore/rendering/RenderGrid.cpp:1145 >> + if (hasExplicitInlineStretch && hasExplicitBlockStretch) > > Couldn't these 3 clause be simplified like this ? > return hasExplicitInlineStretch ? hasExplicitBlockStretc : !hasExplicitBlockStretch; Looks like it can be simplified way more, now that I wrote down the combinations. >> Source/WebCore/rendering/RenderGrid.cpp:1887 >> + if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().alignSelf().position() != ItemPosition::Stretch) { > > Why we don't need to consider align-self: auto here ? I think this one boils down to more testing needed. I left a FIXME to indicate we should work on this more. >> Source/WebCore/rendering/RenderGrid.cpp:1891 >> + } else if (child.style().justifySelf().position() != ItemPosition::Stretch) { > > ditto Done. >> Source/WebCore/rendering/RenderGrid.cpp:1903 >> + if (isHorizontalWritingMode() == child.isHorizontalWritingMode() && child.style().justifySelf().position() != ItemPosition::Stretch) { > > ditto Done. >> Source/WebCore/rendering/RenderGrid.cpp:1908 >> + } else if (child.style().alignSelf().position() != ItemPosition::Stretch) { > > ditto Done.
Comment on attachment 433855 [details] Patch r=me
Committed r280075 (239801@main): <https://commits.webkit.org/239801@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433855 [details].
*** Bug 227293 has been marked as a duplicate of this bug. ***