[margin-trim] block-start margins for grid items in horizontal writin…

…g-mode should be reflected in computed style. https://bugs.webkit.org/show_bug.cgi?id=253716 rdar://106559103 Reviewed by Alan Baradlay. When the block-start margins for grid items that are adjacent to the block-start edge of the grid, We need to set the rare data bits for the item to indicate that the margin has been trimmed. We can then check if this bit is set within ComputedStyleExtractor to get the trimmed value for this margin by directly querying the renderer's m_marginBox. In a horizontal writing mode this bit would correspond to the "top," margin value. For block-start margins, the trimming occurs when a renderer goes through layout and calls RenderBox::constrainBlockMarginInAvailableSpaceOrTrim, which is called from RenderBox::computeBlockDirectionMargins. This function consults the layout system, which is grid in this case, via shouldTrimChildMargin to see if it should trim its margin. If this returns true, then the renderer will not compute the margin and will return 0_lu. We can use this opportunity to set the rare data for the renderer to indicate that this margin has been trimmed. Inside of ComputedStyleExtractor, we need to update rendererCanHaveTrimmedMargin so that this returns true for grid items. This will allow us to do 2 things: 1.) Have isLayoutDependent return true for grid items with margin-trim set on the grid so that layout is forced when checking the "top," margin for grid items 2.) Allow us to call box->hasTrimmedMargin when computing the margin value (since that call is guarded by a call to rendererCanHaveTrimmedMargin) Similarly, there are a couple of other extra pieces of code in RenderBox (particularly in physicalToFlowRelativeDirectionMapping and hasTrimmedMargin) that should now be used with grid items. * LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/grid-block-start-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/grid-block-start.html: Added. * Source/WebCore/css/ComputedStyleExtractor.cpp: (WebCore::rendererCanHaveTrimmedMargin): (WebCore::isLayoutDependent): * Source/WebCore/rendering/RenderBox.cpp: (WebCore::RenderBox::physicalToFlowRelativeDirectionMapping const): (WebCore::RenderBox::hasTrimmedMargin const): (WebCore::RenderBox::constrainBlockMarginInAvailableSpaceOrTrim const): Canonical link: https://commits.webkit.org/262967@main
WebKit · Apr 14, 2023 · c87ee92 · c87ee92
1 parent b2ee03f
commit c87ee92
Show file tree

Hide file tree

Showing 4 changed files with 76 additions and 5 deletions.
diff --git a/...atform-tests/css/css-box/margin-trim/computed-margin-values/grid-block-start-expected.txt b/...atform-tests/css/css-box/margin-trim/computed-margin-values/grid-block-start-expected.txt
@@ -0,0 +1,6 @@
+
+PASS grid > item 1
+PASS grid > item 2
+PASS grid > item 3
+PASS grid > item 4
+
diff --git a/...c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/grid-block-start.html b/...c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/grid-block-start.html
@@ -0,0 +1,52 @@
+<!DOCTYPE html>
+<html>
+<head>
+<link rel="author" title="Sammy Gill" href="mailto:sammy.gill@apple.com">
+<link rel="help" href="https://drafts.csswg.org/css-box-4/#margin-trim-grid">
+<meta name="assert" content="Grid items with trimmed block start margins should be reflected in computed style">
+<style>
+grid {
+    display: grid;
+    width: min-content;
+    border: 1px solid black;
+    grid-template-columns: auto auto;
+    margin-trim: block-start;
+}
+item {
+    display: block;
+    width: 50px;
+    height: 50px;
+    background-color: green;
+}
+.locked-location {
+    grid-row: 1;
+    grid-column: 2;
+}
+item:nth-child(1) {
+    margin-block-start: 50%;
+}
+item:nth-child(2) {
+    margin-block-start: -30px;
+}
+item:nth-child(3) {
+    margin-block-start: 10px;
+}
+item:nth-child(4) {
+    margin-block-start: 10px;
+}
+</style>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/check-layout-th.js"></script>
+</head>
+<body onload="checkLayout('grid > item')">
+    <div id="target">
+    <grid>
+        <item class="locked-location" data-expected-margin-top="0"></item>
+        <item data-expected-margin-top="0"></item>
+        <item data-expected-margin-top="10"></item>
+        <item data-expected-margin-top="10"></item>
+    </grid>
+    </div>
+</body>
+</html>
diff --git a/Source/WebCore/css/ComputedStyleExtractor.cpp b/Source/WebCore/css/ComputedStyleExtractor.cpp
@@ -2304,14 +2304,16 @@ static bool rendererCanHaveTrimmedMargin(const RenderBox& renderer, std::optiona
 
     // containingBlock->isBlockContainer() can return true even if the item is in a RenderFlexibleBox
     // (e.g. buttons) so we should explicitly check that the item is not a flex item to catch block containers here
-    if (!renderer.isFlexItem() && (containingBlock->isRenderGrid() || containingBlock->isBlockContainer()))
+    if (!renderer.isFlexItem() && containingBlock->isBlockContainer())
         return false;
 
     if (containingBlock->isFlexibleBox()) {
         if (!marginTrimType)
             return !containingBlock->style().marginTrim().isEmpty();
         return containingBlock->style().marginTrim().contains(marginTrimType.value());
     }
+    if (containingBlock->isRenderGrid() && (!marginTrimType || marginTrimType.value() == MarginTrimType::BlockStart))
+        return containingBlock->style().marginTrim().contains(marginTrimType.value());
     return false;
 }
 
@@ -2348,7 +2350,7 @@ static bool isLayoutDependent(CSSPropertyID propertyID, const RenderStyle* style
             || (rendererCanHaveTrimmedMargin(downcast<RenderBox>(*renderer), { }));
     }
     case CSSPropertyMarginTop:
-        return paddingOrMarginIsRendererDependent<&RenderStyle::marginTop>(style, renderer) || (isFlexItem(renderer) && rendererCanHaveTrimmedMargin(downcast<RenderBox>(*renderer), MarginTrimType::BlockStart));
+        return paddingOrMarginIsRendererDependent<&RenderStyle::marginTop>(style, renderer) || (is<RenderBox>(renderer) && (rendererCanHaveTrimmedMargin(downcast<RenderBox>(*renderer), MarginTrimType::BlockStart)));
     case CSSPropertyMarginRight:
         return paddingOrMarginIsRendererDependent<&RenderStyle::marginRight>(style, renderer) || (isFlexItem(renderer) && rendererCanHaveTrimmedMargin(downcast<RenderBox>(*renderer), MarginTrimType::InlineEnd));
     case CSSPropertyMarginBottom:

diff --git a/Source/WebCore/rendering/RenderBox.cpp b/Source/WebCore/rendering/RenderBox.cpp
@@ -1392,7 +1392,7 @@ void RenderBox::clearOverridingLogicalWidthLength()
 FlowRelativeDirection RenderBox::physicalToFlowRelativeDirectionMapping(PhysicalDirection direction) const
 {
     auto determineFormattingContextRootStyle = [&]() -> const RenderStyle& {
-        if (isFlexItem())
+        if (isFlexItem() || isGridItem())
             return parent()->style();
         ASSERT_NOT_IMPLEMENTED_YET();
         return style();
@@ -1469,10 +1469,12 @@ bool RenderBox::hasTrimmedMargin(std::optional<MarginTrimType> marginTrimType) c
     // containingBlock->isBlockContainer() can return true even if the item is in a RenderFlexibleBox
     // (e.g. buttons) so we should explicitly check that the item is not a flex item to catch block containers here
     auto* containingBlock = this->containingBlock(); 
-    if (containingBlock && !containingBlock->isFlexibleBox() && (containingBlock->isBlockContainer() || containingBlock->isRenderGrid())) {
+    if (containingBlock && !containingBlock->isFlexibleBox() && containingBlock->isBlockContainer()) {
         ASSERT_NOT_IMPLEMENTED_YET();
         return false;
     }
+    if (containingBlock && containingBlock->isRenderGrid())
+        ASSERT(!marginTrimType || marginTrimType.value() == MarginTrimType::BlockStart);
 #endif
     if (!hasRareData())
         return false;
@@ -3814,9 +3816,18 @@ void RenderBox::computeAndSetBlockDirectionMargins(const RenderBlock& containing
 
 LayoutUnit RenderBox::constrainBlockMarginInAvailableSpaceOrTrim(const RenderBox& containingBlock, LayoutUnit availableSpace, MarginTrimType marginSide) const
 {
+
     ASSERT(marginSide == MarginTrimType::BlockStart || marginSide == MarginTrimType::BlockEnd);
-    if (containingBlock.shouldTrimChildMargin(marginSide, *this))
+    if (containingBlock.shouldTrimChildMargin(marginSide, *this)) {
+        // FIXME(255434): This should be set when the margin is being trimmed
+        // within the context of its layout system (block, flex, grid) and should not 
+        // be done at this level within RenderBox. We should be able to leave the 
+        // trimming responsibility to each of those contexts and not need to
+        // do any of it here (trimming the margin and setting the rare data bit)
+        if (isGridItem() && marginSide == MarginTrimType::BlockStart)
+            const_cast<RenderBox&>(*this).markMarginAsTrimmed(MarginTrimType::BlockStart);
         return 0_lu;
+    }
 
     return marginSide == MarginTrimType::BlockStart ? minimumValueForLength(style().marginBeforeUsing(&containingBlock.style()), availableSpace) : minimumValueForLength(style().marginAfterUsing(&containingBlock.style()), availableSpace);
 }