Skip to content

GitLab

T
tangled
Commit e9d1ed95 authored by Abhishek Arya's avatar Abhishek Arya Committed by Commit Bot
Browse files
Options

Revert "Floats and out-of-flow objects may not be adjacent to anonymous blocks." 

This reverts commit 2394d0ac.

Reason for revert: Caused ClusterFuzz regressions - 853522, 853537, 853538, 853540.

Original change's description:
> Floats and out-of-flow objects may not be adjacent to anonymous blocks.
> 
> Floats and out-of-flow objects need to be true layout siblings of the
> inlines, or rendering will be wrong. This means that such objects should
> never be siblings of anonymous blocks, but rather inside them. This
> already works correctly for initial layout tree building, and also for
> many DOM manipulations. However, code was missing to satisfy this
> requirement if we removed a regular block that was a sibling of an
> anonymous block and either a float or out-of-flow positioned object.
> 
> This even caused a crash triggered by ruby code, which ended up mixing
> inline and block children within the same container. That is not
> allowed. This happened in the MoveAllChildrenIncludingFloatsTo() call
> inside LayoutRubyBase::MoveBlockChildren(). Added a DCHECK to
> MoveAllChildrenIncludingFloatsTo() (which could fail prior to this fix);
> When moving children from one container to another, either both or none
> of the containers must have inline children.
> 
> Bug: 852640
> Change-Id: I51d3de12c73ddd07d6b4c1aa55221b4f92359ca7
> Reviewed-on: https://chromium-review.googlesource.com/1102690
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567882}

TBR=eae@chromium.org,mstensho@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 852640
Change-Id: I0af60d9eac770f21b6a0fbeccf88bb43d5efd6fb
Reviewed-on: https://chromium-review.googlesource.com/1103503Reviewed-by: default avatarAbhishek Arya <inferno@chromium.org>
Commit-Queue: Abhishek Arya <inferno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567914}
parent 1333bf97
Hide whitespace changes
Inline Side-by-side
Showing with 6 additions and 142 deletions
third_party/WebKit/LayoutTests/external/wpt/css/CSS2/abspos/remove-block-between-inline-and-abspos.html deleted 100644 → 0
View file @ 1333bf97
<!DOCTYPE html>
<title>Removing block between inline and float should put the two on the same line</title>
<link rel="author" title="Morten Stenshorne" href="mstensho@chromium.org">
<link rel="help" href="https://www.w3.org/TR/CSS22/visuren.html#absolute-positioning" title="9.6 Absolute positioning">
<link rel="match" href="../../reference/ref-filled-green-200px-square.html">
<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
<!-- This is a block with block children, so inlines need to be wrapped inside
anonymous blocks. A float and out-of-flow positioned box is neutral here,
in that it can either live among block children OR among inline
children. If it is (or becomes) sibling of an inline child, though, it
should be wrapped inside the same anonymous block as the inline, or layout
will be wrong. -->
<div style="width:200px; background:red;">
<div style="height:50px; background:green;"></div>
<div style="display:inline-block; vertical-align:top; width:100px; height:150px; background:green;"></div>
<div id="removeMe" style="height:100px;"></div>
<span style="position:absolute; width:100px; height:150px; background:green;"></span>
</div>
<script>
document.body.offsetTop; // Trigger layout.
document.getElementById("removeMe").style.display = "none";
</script>
third_party/WebKit/LayoutTests/external/wpt/css/CSS2/floats/remove-block-between-inline-and-float.html deleted 100644 → 0
View file @ 1333bf97
<!DOCTYPE html>
<title>Removing block between inline and float should put the two on the same line</title>
<link rel="author" title="Morten Stenshorne" href="mstensho@chromium.org">
<link rel="help" href="https://www.w3.org/TR/CSS22/visuren.html#floats" title="9.5 Floats">
<link rel="match" href="../../reference/ref-filled-green-200px-square.html">
<p>Test passes if there is a filled green square and <strong>no red</strong>.</p>
<!-- This is a block with block children, so inlines need to be wrapped inside
anonymous blocks. A float and out-of-flow positioned box is neutral here,
in that it can either live among block children OR among inline
children. If it is (or becomes) sibling of an inline child, though, it
should be wrapped inside the same anonymous block as the inline, or layout
will be wrong. -->
<div style="width:200px; background:red;">
<div style="height:50px; background:green;"></div>
<div style="display:inline-block; vertical-align:top; width:100px; height:150px; background:green;"></div>
<div id="removeMe" style="height:100px;"></div>
<div style="float:left; width:100px; height:150px; background:green;"></div>
</div>
<script>
document.body.offsetTop; // Trigger layout.
document.getElementById("removeMe").style.display = "none";
</script>
third_party/WebKit/LayoutTests/fast/ruby/merge-base-anonymous-blocks-crash.html deleted 100644 → 0
View file @ 1333bf97
<!DOCTYPE html>
<ruby>
<div></div>
<span id="firstInline"></span>
<rt id="rubyText" style="display:none;"></rt>
<span id="secondInline"></span>
<div id="removeMe"></div>
<div id="absPos" style="position:absolute;"></div>
<span id="addMe" style="display:none;"></span>
</ruby>
<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>
<script>
// The comments describe what happens with crbug.com/852640 present.
test(() => {
// Perform 4 style changes. Relayout before and between each
// change.
document.body.offsetTop;
// Remove a regular block. Its previous and next siblings are an inline
// and an absolutely positioned object, respectively. They should be
// placed together, i.e. the absolutely positioned object should be put
// under the anonymous block around the inline. That's what would have
// happened if the style were like this during initial layout. Getting
// to the same style situation via DOM changes should result in the
// exact same tree. With the bug present, we just leave the absolutely
// positioned box alone, and keep it as a *sibling* (instead of making
// it a child, which would have been the correct thing) of the
// anonoymous wrapper around #firstInline and #secondInline.
document.getElementById("removeMe").style.display = "none";
document.body.offsetTop;
// Add an inline after the absolutely positioned box. It finds no
// adjacent anonymous wrapper block (there's just #absPos, and it's not
// inside a wrapper), so it creates one, and puts itself and #absPos
// inside it. Now we have two sibling anonymous wrapper blocks (one with
// #firstInline and #secondInline, and one with #absPos and #addMe),
// which is weird.
document.getElementById("addMe").style.display = "inline";
document.body.offsetTop;
// Insert an RT object between #firstInline and #secondInline. This will
// split the ruby run and the ruby base in two; one run and base that
// ends with #firstChild, and one run and base that starts with
// #secondChild. We now have a first ruby base that ends with an
// anonymous block (to contain #firstInline), and a second ruby base
// that starts with an anonymous block (to contain #secondInline). This
// is fine.
document.getElementById("rubyText").style.display = "block";
document.body.offsetTop;
// Remove the RT object again. This will cause the two ruby runs and
// bases to be merged back into one. There is code that carefully makes
// sure that the anonymous block at the end of the first base is merged
// with the anonymous block at the start of the next base. This is the
// right thing to do, since we want the two inlines back on one and the
// same line. However, this will cause the first anonymous block in the
// second base to be destroyed (because its contents,
// i.e. #secondInline, is moved over to the anonymous block around
// #firstInline). This in turn will leave the second base with one
// anonymous block (around #absPos and #addMe). Our tree modification
// machinery will detect this and unwrap #absPos and #addMe from their
// anonymous block and destroy it. The second base will now be marked as
// "children inline", while the first base is still marked as having
// block children. This could be fine, but if we now just move the
// children of the second base directly into the first base, we're going
// to end up with a mix of block and inline children within the same
// block, and that's not allowed.
document.getElementById("rubyText").style.display = "none";
}, "No crash or CHECK/DCHECK failure.");
</script>
third_party/blink/renderer/core/layout/layout_block_flow.cc
View file @ e9d1ed95
...@@ -3191,41 +3191,19 @@ void LayoutBlockFlow::RemoveChild(LayoutObject* old_child) { ...@@ -3191,41 +3191,19 @@ void LayoutBlockFlow::RemoveChild(LayoutObject* old_child) {
return; return;
} }
// If this child is a block, and if our previous and next siblings are both // If this child is a block, and if our previous and next siblings are
// anonymous blocks with inline content, then we can go ahead and fold the // both anonymous blocks with inline content, then we can go ahead and
// inline content back together. If only one of the siblings is such an // fold the inline content back together.
// anonymous blocks, check if the other sibling (and any of *its* siblings)
// are floating or out-of-flow positioned. In that case, they should be moved
// into the anonymous block.
LayoutObject* prev = old_child->PreviousSibling(); LayoutObject* prev = old_child->PreviousSibling();
LayoutObject* next = old_child->NextSibling(); LayoutObject* next = old_child->NextSibling();
bool merged_anonymous_blocks = false; bool merged_anonymous_blocks = false;
if (prev && next && !old_child->IsInline() && if (prev && next && !old_child->IsInline() &&
!old_child->VirtualContinuation()) { !old_child->VirtualContinuation() && prev->IsLayoutBlockFlow() &&
if (prev->IsLayoutBlockFlow() && next->IsLayoutBlockFlow() && next->IsLayoutBlockFlow()) {
ToLayoutBlockFlow(prev)->MergeSiblingContiguousAnonymousBlock( if (ToLayoutBlockFlow(prev)->MergeSiblingContiguousAnonymousBlock(
ToLayoutBlockFlow(next))) { ToLayoutBlockFlow(next))) {
merged_anonymous_blocks = true; merged_anonymous_blocks = true;
next = nullptr; next = nullptr;
} else if (prev->IsAnonymousBlock()) {
// The previous sibling is anonymous. Scan the next siblings and reparent
// any floating or out-of-flow positioned objects into the end of the
// previous anonymous block.
while (next && next->IsFloatingOrOutOfFlowPositioned()) {
LayoutObject* sibling = next->NextSibling();
MoveChildTo(ToLayoutBlockFlow(prev), next, nullptr, false);
next = sibling;
}
} else if (next->IsAnonymousBlock()) {
// The next sibling is anonymous. Scan the previous siblings and reparent
// any floating or out-of-flow positioned objects into the start of the
// next anonymous block.
while (prev && prev->IsFloatingOrOutOfFlowPositioned()) {
LayoutObject* sibling = prev->PreviousSibling();
MoveChildTo(ToLayoutBlockFlow(next), prev,
ToLayoutBlockFlow(next)->FirstChild(), false);
prev = sibling;
}
} }
} }
...@@ -3291,9 +3269,6 @@ void LayoutBlockFlow::MoveAllChildrenIncludingFloatsTo( ...@@ -3291,9 +3269,6 @@ void LayoutBlockFlow::MoveAllChildrenIncludingFloatsTo(
bool full_remove_insert) { bool full_remove_insert) {
LayoutBlockFlow* to_block_flow = ToLayoutBlockFlow(to_block); LayoutBlockFlow* to_block_flow = ToLayoutBlockFlow(to_block);
DCHECK(full_remove_insert ||
to_block_flow->ChildrenInline() == ChildrenInline());
// When a portion of the layout tree is being detached, anonymous blocks // When a portion of the layout tree is being detached, anonymous blocks
// will be combined as their children are deleted. In this process, the // will be combined as their children are deleted. In this process, the
// anonymous block later in the tree is merged into the one preceding it. // anonymous block later in the tree is merged into the one preceding it.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment