Skip to content
Permalink
Browse files
REGRESSION (r292043): [ Mac ] fast/block/positioning/fixed-container-…
…with-relative-parent.html is a flaky image failure

https://bugs.webkit.org/show_bug.cgi?id=239101
<rdar://problem/91603539>

Reviewed by Antti Koivisto.

Source/WebCore:

1. Out of flow boxes are laid out independently from each other as the last step of their containing block layout.
2. However their static positions are computed during regular in-flow layout (as if their positions were static).

In order to do #1, we maintain a ListHashSet for the out-of-flow boxes and insert them at #2 (and we also have
a corresponding HashMap<ContainingBlock, ListHasSet>).

Normally this is a very simple list of descendant positioned boxes and since out-of-flow boxes don't interact with each
other, their position in the list is not important.
  e.g.
    <div id=A style="position: relative">
      <div>
        <div id=B style="position: absolute"></div>
        <div id=C style="position: absolute"></div>
      </div>
    </div>

At in-flow layout (#2), we insert B and C to "ListHashSet of A" as we come across them in DOM order and compute their static positions.
Later in the layout flow when we get to the "let's layout the out-of-flow boxes" phase (#1) we simply walk
the ListHashSet and lay out B and C (but "C and B" order would also work just fine).

However the ICB (RenderView) is a special containing block as it can hold different types of out-of-flow boxes (absolute and fixed)
and those out-of-flow boxes may have layout dependencies.
e.g.
    <body><div id=A class=absolute><div id=B class=fixed></div></div></body>

ICB's ListHasSet has both A and B, but in this case there's (static)layout dependency between these boxes.
In order to figure out the static position of B, we have to have A laid out first. In order to lay out A before B,
B has to be preceded by A in ICB's ListHasSet.

Now full layout always guarantees the correct order.
However in case of partial layout since we don't run a full #2, the ListHasSet may end up having an unexpected order.
  e.g.
   <body><div id=A class=absolute><div id=B><div id=C class=fixed></div></div></div></body>

 1. The initial (full) layout produces the following (correct) order for the ICB's ListHasSet -> AC.
 2. A subsequent partial layout (e.g. triggered by A's position change) runs an in-flow layout on the <body> which
 (re-)appends A to the ListHasSet (CA <- incorrect order). Now at this point we assume that the in-flow layout picks up B
 which eventually (re-)appends C to the ListHashSet (AC <- correct order). However since B does not need layout, we just
 stop at <body> which leaves us with an unexpected ListHashSet.
 3. As part of the ICB's out-of-flow layout, we pick C as the first box to lay out followed by A. However since C's static
 position depends on A's position, we end up using stale geometry when computing C's static position.

This patch fixes this issue by ensuring the absolute positioned boxes always come first in the ICB's ListHasSet (note
that their order is not really important -see above. What's important is that a potential (as-if-static) containing block always
comes before the fixed boxes).

Test: fast/block/fixed-inside-absolute-positioned.html

* rendering/RenderBlock.cpp:
(WebCore::PositionedDescendantsMap::addDescendant):
(WebCore::RenderBlock::insertPositionedObject):

LayoutTests:

* fast/block/fixed-inside-absolute-positioned-expected.html: Added.
* fast/block/fixed-inside-absolute-positioned.html: Added.
* platform/mac-wk1/TestExpectations:

Canonical link: https://commits.webkit.org/249597@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@292817 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
alanbaradlay committed Apr 13, 2022
1 parent 38c27d7 commit a7fc97b0b2c20d744c420e5b7d5e4eea06c2a495
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 10 deletions.
@@ -1,3 +1,15 @@
2022-04-13 Alan Bujtas <zalan@apple.com>

REGRESSION (r292043): [ Mac ] fast/block/positioning/fixed-container-with-relative-parent.html is a flaky image failure
https://bugs.webkit.org/show_bug.cgi?id=239101
<rdar://problem/91603539>

Reviewed by Antti Koivisto.

* fast/block/fixed-inside-absolute-positioned-expected.html: Added.
* fast/block/fixed-inside-absolute-positioned.html: Added.
* platform/mac-wk1/TestExpectations:

2022-04-13 Simon Fraser <simon.fraser@apple.com>

[css-scroll-snap] scrollIntoView fails with scroll-snap-type on :root
@@ -0,0 +1,13 @@
<style>
body {
margin: 0px;
}
div {
width: 100px;
height: 100px;
background-color: green;
margin-top: 8px;
margin-left: 100px;
}
</style>
<div></div>
@@ -0,0 +1,21 @@
<style>
.outer {
position: absolute;
width: 100px;
height: 100px;
background-color: red;
}

.inner {
position: fixed;
width: 100px;
height: 100px;
background-color: green;
}
</style>
<!-- Pass if no red -->
<div id=moveThis class=outer><div><div class=inner></div></div></div>
<script>
document.body.offsetHeight;
moveThis.style.left = "100px";
</script>
@@ -1046,8 +1046,6 @@ imported/w3c/web-platform-tests/credential-management/ [ Skip ]

webkit.org/b/182554 transitions/transition-display-property.html [ Pass ImageOnlyFailure ]

webkit.org/b/181834 [ Debug ] fast/block/positioning/fixed-container-with-relative-parent.html [ Pass ImageOnlyFailure ]

imported/w3c/web-platform-tests/css/selectors/selector-placeholder-shown-type-change-002.html [ ImageOnlyFailure ]

webkit.org/b/184456 imported/w3c/web-platform-tests/html/semantics/embedded-content/the-embed-element/embed-in-object-fallback.html [ Pass Failure ]
@@ -2294,5 +2294,3 @@ http/tests/webgpu [ Pass ]

# fractional pixel diff between modern and legacy line layout.
imported/blink/fast/multicol/vertical-lr/float-content-break.html [ ImageOnlyFailure ]

webkit.org/b/239101 fast/block/positioning/fixed-container-with-relative-parent.html [ Pass ImageOnlyFailure ]
@@ -1,3 +1,63 @@
2022-04-13 Alan Bujtas <zalan@apple.com>

REGRESSION (r292043): [ Mac ] fast/block/positioning/fixed-container-with-relative-parent.html is a flaky image failure
https://bugs.webkit.org/show_bug.cgi?id=239101
<rdar://problem/91603539>

Reviewed by Antti Koivisto.

1. Out of flow boxes are laid out independently from each other as the last step of their containing block layout.
2. However their static positions are computed during regular in-flow layout (as if their positions were static).

In order to do #1, we maintain a ListHashSet for the out-of-flow boxes and insert them at #2 (and we also have
a corresponding HashMap<ContainingBlock, ListHasSet>).

Normally this is a very simple list of descendant positioned boxes and since out-of-flow boxes don't interact with each
other, their position in the list is not important.
e.g.
<div id=A style="position: relative">
<div>
<div id=B style="position: absolute"></div>
<div id=C style="position: absolute"></div>
</div>
</div>

At in-flow layout (#2), we insert B and C to "ListHashSet of A" as we come across them in DOM order and compute their static positions.
Later in the layout flow when we get to the "let's layout the out-of-flow boxes" phase (#1) we simply walk
the ListHashSet and lay out B and C (but "C and B" order would also work just fine).

However the ICB (RenderView) is a special containing block as it can hold different types of out-of-flow boxes (absolute and fixed)
and those out-of-flow boxes may have layout dependencies.
e.g.
<body><div id=A class=absolute><div id=B class=fixed></div></div></body>

ICB's ListHasSet has both A and B, but in this case there's (static)layout dependency between these boxes.
In order to figure out the static position of B, we have to have A laid out first. In order to lay out A before B,
B has to be preceded by A in ICB's ListHasSet.

Now full layout always guarantees the correct order.
However in case of partial layout since we don't run a full #2, the ListHasSet may end up having an unexpected order.
e.g.
<body><div id=A class=absolute><div id=B><div id=C class=fixed></div></div></div></body>

1. The initial (full) layout produces the following (correct) order for the ICB's ListHasSet -> AC.
2. A subsequent partial layout (e.g. triggered by A's position change) runs an in-flow layout on the <body> which
(re-)appends A to the ListHasSet (CA <- incorrect order). Now at this point we assume that the in-flow layout picks up B
which eventually (re-)appends C to the ListHashSet (AC <- correct order). However since B does not need layout, we just
stop at <body> which leaves us with an unexpected ListHashSet.
3. As part of the ICB's out-of-flow layout, we pick C as the first box to lay out followed by A. However since C's static
position depends on A's position, we end up using stale geometry when computing C's static position.

This patch fixes this issue by ensuring the absolute positioned boxes always come first in the ICB's ListHasSet (note
that their order is not really important -see above. What's important is that a potential (as-if-static) containing block always
comes before the fixed boxes).

Test: fast/block/fixed-inside-absolute-positioned.html

* rendering/RenderBlock.cpp:
(WebCore::PositionedDescendantsMap::addDescendant):
(WebCore::RenderBlock::insertPositionedObject):

2022-04-13 Antti Koivisto <antti@apple.com>

[CSS Container Queries] Limit query range syntax
@@ -153,8 +153,7 @@ static void removeFromTrackedRendererMaps(RenderBox& descendant)

class PositionedDescendantsMap {
public:
enum class MoveDescendantToEnd { No, Yes };
void addDescendant(const RenderBlock& containingBlock, RenderBox& positionedDescendant, MoveDescendantToEnd moveDescendantToEnd)
void addDescendant(const RenderBlock& containingBlock, RenderBox& positionedDescendant)
{
// Protect against double insert where a descendant would end up with multiple containing blocks.
auto* previousContainingBlock = m_containerMap.get(&positionedDescendant);
@@ -167,8 +166,28 @@ class PositionedDescendantsMap {
return makeUnique<TrackedRendererListHashSet>();
}).iterator->value;

bool isNewEntry = moveDescendantToEnd == MoveDescendantToEnd::Yes ? descendants->appendOrMoveToLast(&positionedDescendant).isNewEntry
: descendants->add(&positionedDescendant).isNewEntry;
auto isNewEntry = false;
if (!is<RenderView>(containingBlock) || descendants->isEmpty())
isNewEntry = descendants->add(&positionedDescendant).isNewEntry;
else if (positionedDescendant.isFixedPositioned())
isNewEntry = descendants->appendOrMoveToLast(&positionedDescendant).isNewEntry;
else {
auto ensureLayoutDepentBoxPosition = [&] {
// RenderView is a special containing block as it may hold both absolute and fixed positioned containing blocks.
// When a fixed positioned box is also a descendant of an absolute positioned box anchored to the RenderView,
// we have to make sure that the absolute positioned box is inserted before the fixed box to follow
// block layout dependency.
for (auto it = descendants->begin(); it != descendants->end(); ++it) {
if ((*it)->isFixedPositioned()) {
isNewEntry = descendants->insertBefore(it, &positionedDescendant).isNewEntry;
return;
}
}
isNewEntry = descendants->appendOrMoveToLast(&positionedDescendant).isNewEntry;
};
ensureLayoutDepentBoxPosition();
}

if (!isNewEntry) {
ASSERT(m_containerMap.contains(&positionedDescendant));
return;
@@ -1791,8 +1810,7 @@ void RenderBlock::insertPositionedObject(RenderBox& positioned)
ASSERT(posChildNeedsLayout() || view().frameView().layoutContext().isInLayout());
setPosChildNeedsLayoutBit(true);
}
positionedDescendantsMap().addDescendant(*this, positioned, isRenderView() ? PositionedDescendantsMap::MoveDescendantToEnd::Yes
: PositionedDescendantsMap::MoveDescendantToEnd::No);
positionedDescendantsMap().addDescendant(*this, positioned);
}

void RenderBlock::removePositionedObject(const RenderBox& rendererToRemove)

0 comments on commit a7fc97b

Please sign in to comment.