Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[margin-trim][block layout] Trimmed block-end margins should be refle… 
…cted in computed style in a horizontal writing-mode BFC.

https://bugs.webkit.org/show_bug.cgi?id=253610
rdar://106454992

Reviewed by Alan Baradlay.

We currently perform margin trimming by doing some backtracking after a
block container with block-end margin trim has completed layout (261750@main).
In order to make sure that these margins are properly shown in their
computed style value, we need a way for ComputedStyleExtractor to know
that the block-end margin for the renderer has been trimmed. This can be
done by setting the margin-trim rare data bit for that renderer.

Whenever we trim a margin in RenderBlockFlow::trimBlockEndChildrenMargins,
we can replace the calls to setMarginAfterForChild with setTrimmedMarginForChild
which will both trim the margin and set the rare data bit for the renderer.

Later on when ComputedStyleExtractor tries to determine the value of
a bottom margin for a renderer it can first use hasTrimmedMargin on the
renderer in order to determine if that specific margin is trimmed.

While creating this patch and testing the logic, I found that certain
margins are not being trimmed correctly. It seems like particularly that
self collapsing children that has other nested self collapsing content
do not have the nested content trimmed properly. Whenever we have a
self collapsing child, we need to also go inside and trim the margins
of any other children (and perhaps perform this logic for any other
children that are nested inside that). For example

<container>
    <item style="margin-bottom: 10px"></item>
    <item style="margin-bottom: 10px; height: 0px;">
        <item style="margin-bottom: 10px; height: 0px;">
            <item style="margin-bottom: 10px; height: 0px;">
                <item style="margin-bottom: 10px; height: 0px;"></item>
                <item style="margin-bottom: 10px; height: 0px;"></item>
            </item>
        </item>
    </item>
</container>

In this scenario the content itself looks fine as if margin-trimming
occurred correctly, however when looking at the computed margin-bottom
values of the nested self collapsing children we can see that their
margins were not trimmed. Currently we fail the tests in these scenarios
that are introduced in this patch because of this. I plan on addressing
this in a separate patch following this one.

* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/block-container-block-end-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/block-container-block-end-nested-child-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/block-container-block-end-nested-child.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/block-container-block-end-with-self-collapsing-children-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/block-container-block-end-with-self-collapsing-children.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-box/margin-trim/computed-margin-values/block-container-block-end.html: Added.
* Source/WebCore/css/ComputedStyleExtractor.cpp:
(WebCore::rendererCanHaveTrimmedMargin):
* Source/WebCore/rendering/RenderBlockFlow.cpp:
(WebCore::RenderBlockFlow::trimBlockEndChildrenMargins):
* Source/WebCore/rendering/RenderBox.cpp:
(WebCore::RenderBox::physicalToFlowRelativeDirectionMapping const):
(WebCore::RenderBox::hasTrimmedMargin const):
(WebCore::RenderBox::hasTrimmedMargin const):
* Source/WebCore/rendering/RenderBox.h:
(WebCore::RenderBox::isBlockLevelBox const):

Canonical link: https://commits.webkit.org/263398@main
  • Loading branch information
@sgill26
sgill26 authored and Sammy Gill committed Apr 26, 2023
1 parent ce99b4d commit e44bd7f
Show file tree
Hide file tree
Showing 10 changed files with 304 additions and 16 deletions.
3 changes: 3 additions & 0 deletions ...sts/css/css-box/margin-trim/computed-margin-values/block-container-block-end-expected.txt
View file
@@ -0,0 +1,3 @@

PASS item 1

46 changes: 46 additions & 0 deletions ...ox/margin-trim/computed-margin-values/block-container-block-end-nested-child-expected.txt
View file
@@ -0,0 +1,46 @@

PASS item 1
FAIL item 2 assert_equals:
<item data-expected-margin-bottom="0" style="block-size: auto;">
<div><item data-expected-margin-bottom="10"></item></div>
<div>
<item data-expected-margin-bottom="0" style="block-size: auto;">
<div><item data-expected-margin-bottom="0"></item></div>
</item>
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
</div>
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
</item>
margin-bottom expected "0" but got "10"
PASS item 3
PASS item 4
PASS item 5
FAIL item 6 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 7 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
FAIL item 8 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 9 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
FAIL item 10 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 11 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"

55 changes: 55 additions & 0 deletions ...ss/css-box/margin-trim/computed-margin-values/block-container-block-end-nested-child.html
View file
@@ -0,0 +1,55 @@
<!DOCTYPE html>
<html>
<head>
<link rel="author" href="mailto:sammy.gill@apple.com">
<link rel="help" href="https://w3c.github.io/csswg-drafts/css-box-4/#margin-trim-block">
<meta name="assert" content="Items, including self-collapsing ones, at the block-end of the container should have their margins trimmed">
<style>
container {
display: block;
inline-size: min-content;
margin-trim: block-end;
}
item {
display: block;
background-color: green;
inline-size: 50px;
block-size: 10px;
margin-block-end: 10px;
}
.border {
border: 1px solid black;
}
.collapsed {
block-size: 0px;
}
</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('item')">
<div id="target">
<container>
<item data-expected-margin-bottom="10"></item>
<item data-expected-margin-bottom="0" style="block-size: auto;">
<div><item data-expected-margin-bottom="10"></item></div>
<div>
<item data-expected-margin-bottom="0" style="block-size: auto;">
<div><item data-expected-margin-bottom="0"></item></div>
</item>
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
</div>
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
</item>
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
</container>
</div>
</body>
</html>
93 changes: 93 additions & 0 deletions ...mputed-margin-values/block-container-block-end-with-self-collapsing-children-expected.txt
View file
@@ -0,0 +1,93 @@

PASS item 1
PASS item 2
FAIL item 3 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
</item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 4 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 5 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 6 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
FAIL item 7 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
FAIL item 8 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
FAIL item 9 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
</item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 10 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 11 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
FAIL item 12 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 13 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
FAIL item 14 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
FAIL item 15 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 16 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
margin-bottom expected "0" but got "10"
FAIL item 17 assert_equals:
<item class="collapsed" data-expected-margin-bottom="0"></item>
margin-bottom expected "0" but got "10"
PASS item 18

60 changes: 60 additions & 0 deletions ...-trim/computed-margin-values/block-container-block-end-with-self-collapsing-children.html
View file
@@ -0,0 +1,60 @@
<!DOCTYPE html>
<html>
<head>
<link rel="author" href="mailto:sammy.gill@apple.com">
<link rel="help" href="https://w3c.github.io/csswg-drafts/css-box-4/#margin-trim-block">
<meta name="assert" content="Second item and all self-collapsing children at block-end should have margins trimmed">
<style>
container {
display: block;
inline-size: min-content;
margin-trim: block-end;
}
item {
display: block;
background-color: green;
inline-size: 50px;
block-size: 50px;
margin-block-end: 10px;
}
.collapsed {
block-size: 0px;
}
</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('item')">
<div id="target">
<container>
<item data-expected-margin-bottom="10"></item>
<item data-expected-margin-bottom="0"></item>
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
</item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item>
<item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0">
<div><item class="collapsed" data-expected-margin-bottom="0"></item></div>
</item></div>
</item></div>
</item>
<item class="collapsed" data-expected-margin-bottom="0"></item>
</container>
</div>
</body>
</html>
32 changes: 32 additions & 0 deletions ...tform-tests/css/css-box/margin-trim/computed-margin-values/block-container-block-end.html
View file
@@ -0,0 +1,32 @@
<!DOCTYPE html>
<html>
<head>
<link rel="author" href="mailto:sammy.gill@apple.com">
<link rel="help" href="https://w3c.github.io/csswg-drafts/css-box-4/#margin-trim-block">
<meta name="assert" content="block-end margin of item should be trimmed">
<style>
container {
display: block;
inline-size: min-content;
margin-trim: block-end;
}
item {
display: block;
background-color: green;
inline-size: 50px;
block-size: 50px;
margin-block-end: 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('item')">
<div id="target">
<container>
<item data-expected-margin-bottom="0"></item>
</container>
</div>
</body>
</html>
16 changes: 10 additions & 6 deletions Source/WebCore/css/ComputedStyleExtractor.cpp
View file
Expand Up @@ -2295,13 +2295,17 @@ static bool rendererCanHaveTrimmedMargin(const RenderBox& renderer, MarginTrimTy
if (!containingBlock || containingBlock->isRenderView())
return false;

// 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->isBlockContainer())
return false;

if (containingBlock->isFlexibleBox() || containingBlock->isRenderGrid())
return containingBlock->style().marginTrim().contains(marginTrimType);
// Even though margin-trim is not inherited, it is possible for nested block level boxes
// to get placed at the block-start of an containing block ancestor which does have margin-trim.
// In this case it is not enough to simply check the immediate containing block of the child. It is
// also probably too expensive to perform an arbitrary walk up the tree to check for the existence
// of an ancestor containing block with the property, so we will just return true and let
// the rest of the logic in RenderBox::hasTrimmedMargin to determine if the rare data bit
// were set at some point during layout
if (containingBlock->isBlockContainer() && containingBlock->isHorizontalWritingMode() && renderer.isBlockLevelBox() && marginTrimType == MarginTrimType::BlockEnd)
return true;
return false;
}

Expand Down Expand Up @@ -2478,7 +2482,7 @@ static bool isLayoutDependent(CSSPropertyID propertyID, const RenderStyle* style
return isLayoutDependent(toPaddingOrMarginPropertyID(FlowRelativeDirection::InlineEnd, *renderBox, PropertyType::Margin), style, renderBox);
return false;
case CSSPropertyMarginTop:
return paddingOrMarginIsRendererDependent<&RenderStyle::marginTop>(style, renderer) || (is<RenderBox>(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) || ((is<RenderBox>(renderer)) && rendererCanHaveTrimmedMargin(downcast<RenderBox>(*renderer), MarginTrimType::InlineEnd));
case CSSPropertyMarginBottom:
Expand Down
4 changes: 2 additions & 2 deletions Source/WebCore/rendering/RenderBlockFlow.cpp
View file
Expand Up @@ -872,9 +872,9 @@ void RenderBlockFlow::trimBlockEndChildrenMargins()
}

auto* childContainingBlock = child->containingBlock();
childContainingBlock->setMarginAfterForChild(*child, 0_lu);
setTrimmedMarginForChild(*child, MarginTrimType::BlockEnd);
if (child->isSelfCollapsingBlock()) {
childContainingBlock->setMarginBeforeForChild(*child, 0_lu);
setTrimmedMarginForChild(*child, MarginTrimType::BlockStart);
childContainingBlock->setLogicalTopForChild(*child, childContainingBlock->logicalHeight());
child = child->previousSiblingBox();
} else if (auto* nestedBlock = dynamicDowncast<RenderBlockFlow>(child); nestedBlock && nestedBlock->isBlockContainer() && !nestedBlock->childrenInline() && !nestedBlock->style().marginTrim().contains(MarginTrimType::BlockEnd)) {
Expand Down
10 changes: 2 additions & 8 deletions Source/WebCore/rendering/RenderBox.cpp
View file
Expand Up @@ -1408,14 +1408,8 @@ bool RenderBox::hasTrimmedMargin(std::optional<MarginTrimType> marginTrimType) c
// We should assert here if this function is called with a layout system and
// MarginTrimType combination that is not supported yet (i.e. the layout system
// does not set the margin trim rare data bit for that margin)

// 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()) {
ASSERT_NOT_IMPLEMENTED_YET();
return false;
}
if (!isFlexItem() && !isGridItem() && isBlockLevelBox())
ASSERT(!marginTrimType || marginTrimType.value() == MarginTrimType::BlockEnd);
#endif
if (!hasRareData())
return false;
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/rendering/RenderBox.h
View file
Expand Up @@ -686,6 +686,7 @@ class RenderBox : public RenderBoxModelObject {

bool isGridItem() const { return parent() && parent()->isRenderGrid() && !isExcludedFromNormalLayout(); }
bool isFlexItem() const { return parent() && parent()->isFlexibleBox() && !isExcludedFromNormalLayout(); }
bool isBlockLevelBox() const { return style().isDisplayBlockLevel(); }

virtual void adjustBorderBoxRectForPainting(LayoutRect&) { };

Expand Down

0 comments on commit e44bd7f

Please sign in to comment.