Changeset 192275 in webkit

Timestamp:

Nov 10, 2015 3:18:52 PM (8 years ago)

Author:

Alan Bujtas

Message:

Continuations with anonymous wrappers inside misplaces child renderers.
​https://bugs.webkit.org/show_bug.cgi?id=150908

When a child is appended to an inline container and the beforeChild is not a direct child, but
it is inside a generated subtree, we need to special case the inline split to form continuation.

RenderInline::splitInlines() assumes that beforeChild is always a direct child of the current
inline container. However when beforeChild type requires wrapper content (such as table cells), the DOM and the
render tree get out of sync. In such cases, we need to ensure that both the beforeChild and its siblings end up
in the correct generated block.

Reviewed by Darin Adler and David Hyatt.

Source/WebCore:

Test: fast/inline/continuation-with-anon-wrappers.html

• rendering/RenderInline.cpp:

(WebCore::RenderInline::splitInlines):
(WebCore::RenderInline::addChildToContinuation):

LayoutTests:

• fast/inline/continuation-with-anon-wrappers-expected.txt: Added.
• fast/inline/continuation-with-anon-wrappers.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/inline/continuation-with-anon-wrappers-expected.txt (added)
• LayoutTests/fast/inline/continuation-with-anon-wrappers.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderInline.cpp (modified) (5 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2015-11-10  Zalan Bujtas  <zalan@apple.com>
+
+        Continuations with anonymous wrappers inside misplaces child renderers.
+        https://bugs.webkit.org/show_bug.cgi?id=150908
+
+        When a child is appended to an inline container and the beforeChild is not a direct child, but 
+        it is inside a generated subtree, we need to special case the inline split to form continuation.
+
+        RenderInline::splitInlines() assumes that beforeChild is always a direct child of the current
+        inline container. However when beforeChild type requires wrapper content (such as table cells), the DOM and the
+        render tree get out of sync. In such cases, we need to ensure that both the beforeChild and its siblings end up
+        in the correct generated block.
+
+        Reviewed by Darin Adler and David Hyatt.
+
+        * fast/inline/continuation-with-anon-wrappers-expected.txt: Added.
+        * fast/inline/continuation-with-anon-wrappers.html: Added.
+
 2015-11-10  Geoffrey Garen  <ggaren@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-11-10  Zalan Bujtas  <zalan@apple.com>
+
+        Continuations with anonymous wrappers inside misplaces child renderers.
+        https://bugs.webkit.org/show_bug.cgi?id=150908
+
+        When a child is appended to an inline container and the beforeChild is not a direct child, but 
+        it is inside a generated subtree, we need to special case the inline split to form continuation.
+
+        RenderInline::splitInlines() assumes that beforeChild is always a direct child of the current
+        inline container. However when beforeChild type requires wrapper content (such as table cells), the DOM and the
+        render tree get out of sync. In such cases, we need to ensure that both the beforeChild and its siblings end up
+        in the correct generated block.
+
+        Reviewed by Darin Adler and David Hyatt.
+
+        Test: fast/inline/continuation-with-anon-wrappers.html
+
+        * rendering/RenderInline.cpp:
+        (WebCore::RenderInline::splitInlines):
+        (WebCore::RenderInline::addChildToContinuation):
+
 2015-11-10  Geoffrey Garen  <ggaren@apple.com>
 

trunk/Source/WebCore/rendering/RenderInline.cpp

@@ -413 +413 @@
     // Create a clone of this inline.
     RenderPtr<RenderInline> cloneInline = clone();
-    cloneInline->setContinuation(oldCont);
-
 #if ENABLE(FULLSCREEN_API)
     // If we're splitting the inline containing the fullscreened element,
@@ -425 +423 @@
         beforeChild = document().fullScreenRenderer();
 #endif
-
     // Now take all of the children from beforeChild to the end and remove
     // them from |this| and place them in the clone.
-    RenderObject* renderer = beforeChild;
-    while (renderer) {
-        RenderObject* tmp = renderer;
-        renderer = tmp->nextSibling();
-        removeChildInternal(*tmp, NotifyChildren);
-        cloneInline->addChildIgnoringContinuation(tmp);
-        tmp->setNeedsLayoutAndPrefWidthsRecalc();
-    }
-
+    for (RenderObject* rendererToMove = beforeChild; rendererToMove;) {
+        RenderObject* nextSibling = rendererToMove->nextSibling();
+        // When anonymous wrapper is present, we might need to move the whole subtree instead.
+        if (rendererToMove->parent() != this) {
+            auto* anonymousParent = rendererToMove->parent();
+            while (anonymousParent && anonymousParent->parent() != this) {
+                ASSERT(anonymousParent->isAnonymous());
+                anonymousParent = anonymousParent->parent();
+            }
+            if (!anonymousParent) {
+                ASSERT_NOT_REACHED();
+                break;
+            }
+            // If beforeChild is the first child in the subtree, we could just move the whole subtree.
+            if (!rendererToMove->previousSibling()) {
+                // Reparent the whole anonymous wrapper tree.
+                rendererToMove = anonymousParent;
+                // Skip to the next sibling that is not in this subtree.
+                nextSibling = anonymousParent->nextSibling();
+            } else if (!rendererToMove->nextSibling()) {
+                // This is the last renderer in the subtree. We need to jump out of the wrapper subtree, so that
+                // the siblings are getting reparented too.
+                nextSibling = anonymousParent->nextSibling();
+            }
+            // Otherwise just move the renderer to the inline clone. Should the renderer need an anon
+            // wrapper, the addChild() will generate one for it.
+            // FIXME: When the anonymous wrapper has multiple children, we end up traversing up to the topmost wrapper
+            // every time, which is a bit wasteful.
+        }
+        rendererToMove->parent()->removeChildInternal(*rendererToMove, NotifyChildren);
+        cloneInline->addChildIgnoringContinuation(rendererToMove);
+        rendererToMove->setNeedsLayoutAndPrefWidthsRecalc();
+        rendererToMove = nextSibling;
+    }
+    cloneInline->setContinuation(oldCont);
     // Hook |clone| up as the continuation of the middle block.
     middleBlock->setContinuation(cloneInline.get());
@@ -469 +492 @@
             // Now we need to take all of the children starting from the first child
             // *after* currentChild and append them all to the clone.
-            renderer = currentChild->nextSibling();
-            while (renderer) {
-                RenderObject* tmp = renderer;
-                renderer = tmp->nextSibling();
-                currentInline.removeChildInternal(*tmp, NotifyChildren);
-                cloneInline->addChildIgnoringContinuation(tmp);
-                tmp->setNeedsLayoutAndPrefWidthsRecalc();
+            for (auto* current = currentChild->nextSibling(); current;) {
+                auto* next = current->nextSibling();
+                currentInline.removeChildInternal(*current, NotifyChildren);
+                cloneInline->addChildIgnoringContinuation(current);
+                current->setNeedsLayoutAndPrefWidthsRecalc();
+                current = next;
             }
         }
@@ -493 +515 @@
     // Now take all the children after currentChild and remove them from the fromBlock
     // and put them in the toBlock.
-    renderer = currentChild->nextSibling();
-    while (renderer) {
-        RenderObject* tmp = renderer;
-        renderer = tmp->nextSibling();
-        fromBlock->removeChildInternal(*tmp, NotifyChildren);
-        toBlock->insertChildInternal(tmp, nullptr, NotifyChildren);
+    for (auto* current = currentChild->nextSibling(); current;) {
+        auto* next = current->nextSibling();
+        fromBlock->removeChildInternal(*current, NotifyChildren);
+        toBlock->insertChildInternal(current, nullptr, NotifyChildren);
+        current = next;
     }
 }
@@ -570 +591 @@
 {
     RenderBoxModelObject* flow = continuationBefore(beforeChild);
-    ASSERT(!beforeChild || is<RenderBlock>(*beforeChild->parent()) || is<RenderInline>(*beforeChild->parent()));
-    RenderBoxModelObject* beforeChildParent = nullptr;
-    if (beforeChild)
-        beforeChildParent = downcast<RenderBoxModelObject>(beforeChild->parent());
-    else {
-        if (RenderBoxModelObject* continuation = nextContinuation(flow))
-            beforeChildParent = continuation;
-        else
-            beforeChildParent = flow;
+    // It may or may not be the direct parent of the beforeChild.
+    RenderBoxModelObject* beforeChildAncestor = nullptr;
+    // In case of anonymous wrappers, the parent of the beforeChild is mostly irrelevant. What we need is
+    // the topmost wrapper.
+    if (beforeChild && !is<RenderBlock>(beforeChild->parent()) && beforeChild->parent()->isAnonymous()) {
+        RenderElement* anonymousParent = beforeChild->parent();
+        while (anonymousParent && anonymousParent->parent() && anonymousParent->parent()->isAnonymous())
+            anonymousParent = anonymousParent->parent();
+        ASSERT(anonymousParent && anonymousParent->parent());
+        beforeChildAncestor = downcast<RenderBoxModelObject>(anonymousParent->parent());
+    } else {
+        ASSERT(!beforeChild || is<RenderBlock>(*beforeChild->parent()) || is<RenderInline>(*beforeChild->parent()));
+        if (beforeChild)
+            beforeChildAncestor = downcast<RenderBoxModelObject>(beforeChild->parent());
+        else {
+            if (RenderBoxModelObject* continuation = nextContinuation(flow))
+                beforeChildAncestor = continuation;
+            else
+                beforeChildAncestor = flow;
+        }
     }
 
     if (newChild->isFloatingOrOutOfFlowPositioned())
-        return beforeChildParent->addChildIgnoringContinuation(newChild, beforeChild);
-
+        return beforeChildAncestor->addChildIgnoringContinuation(newChild, beforeChild);
+
+    if (flow == beforeChildAncestor)
+        return flow->addChildIgnoringContinuation(newChild, beforeChild);
     // A continuation always consists of two potential candidates: an inline or an anonymous
     // block box holding block children.
     bool childInline = newChildIsInline(*newChild, *this);
-    bool bcpInline = beforeChildParent->isInline();
-    bool flowInline = flow->isInline();
-
-    if (flow == beforeChildParent)
-        return flow->addChildIgnoringContinuation(newChild, beforeChild);
-    else {
-        // The goal here is to match up if we can, so that we can coalesce and create the
-        // minimal # of continuations needed for the inline.
-        if (childInline == bcpInline)
-            return beforeChildParent->addChildIgnoringContinuation(newChild, beforeChild);
-        else if (flowInline == childInline)
-            return flow->addChildIgnoringContinuation(newChild); // Just treat like an append.
-        else
-            return beforeChildParent->addChildIgnoringContinuation(newChild, beforeChild);
-    }
+    // The goal here is to match up if we can, so that we can coalesce and create the
+    // minimal # of continuations needed for the inline.
+    if (childInline == beforeChildAncestor->isInline())
+        return beforeChildAncestor->addChildIgnoringContinuation(newChild, beforeChild);
+    if (flow->isInline() == childInline)
+        return flow->addChildIgnoringContinuation(newChild); // Just treat like an append.
+    return beforeChildAncestor->addChildIgnoringContinuation(newChild, beforeChild);
 }