Skip to content

GitLab

T
tangled
Commit 2394d0ac authored by Morten Stenshorne's avatar Morten Stenshorne Committed by Commit Bot
Browse files
Options

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/1102690Reviewed-by: default avatarEmil A Eklund <eae@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567882}
parent 28f48ea0
Hide whitespace changes
Inline Side-by-side
Showing with 142 additions and 6 deletions
third_party/WebKit/LayoutTests/external/wpt/css/CSS2/abspos/remove-block-between-inline-and-abspos.html 0 → 100644
View file @ 2394d0ac
<!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 0 → 100644
View file @ 2394d0ac
<!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 0 → 100644
View file @ 2394d0ac
<!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 @ 2394d0ac
...@@ -3191,19 +3191,41 @@ void LayoutBlockFlow::RemoveChild(LayoutObject* old_child) { ...@@ -3191,19 +3191,41 @@ void LayoutBlockFlow::RemoveChild(LayoutObject* old_child) {
return; return;
} }
// If this child is a block, and if our previous and next siblings are // If this child is a block, and if our previous and next siblings are both
// both anonymous blocks with inline content, then we can go ahead and // anonymous blocks with inline content, then we can go ahead and fold the
// fold the inline content back together. // inline content back together. If only one of the siblings is such an
// 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() && prev->IsLayoutBlockFlow() && !old_child->VirtualContinuation()) {
next->IsLayoutBlockFlow()) { if (prev->IsLayoutBlockFlow() && next->IsLayoutBlockFlow() &&
if (ToLayoutBlockFlow(prev)->MergeSiblingContiguousAnonymousBlock( 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;
}
} }
} }
...@@ -3269,6 +3291,9 @@ void LayoutBlockFlow::MoveAllChildrenIncludingFloatsTo( ...@@ -3269,6 +3291,9 @@ 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