display:contents should work with dynamic table mutations
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Nov 2017 22:38:35 +0000 (22:38 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Nov 2017 22:38:35 +0000 (22:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=179179

Reviewed by Ryosuke Niwa.

Source/WebCore:

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::addChildIgnoringContinuation):

    RenderText with inline text wrapper as beforeChild is now resolved in RenderTreePosition, covering all cases.
    Verify this with assert.

* rendering/RenderElement.cpp:
(WebCore::RenderElement::insertChildInternal):

    Add assertion.

* rendering/RenderTableSection.cpp:
(WebCore::RenderTableSection::addChild):

    Fix cases where we did unchecked downcasts for anonymous beforeChild.

* style/RenderTreePosition.cpp:
(WebCore::RenderTreePosition::insert):

    When inserting before a text rendeder with an display:contents inline wrapper, use the wrapper as beforeChild.

* style/RenderTreePosition.h:
(WebCore::RenderTreePosition::insert): Deleted.
* style/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::updateRenderTree):
(WebCore::RenderTreeUpdater::renderingParent):

    Add separate helper to get parent frame for the closest rendered (non display:contents) ancestor.

(WebCore::RenderTreeUpdater::renderTreePosition):
(WebCore::RenderTreeUpdater::updateElementRenderer):
(WebCore::RenderTreeUpdater::textRendererIsNeeded):
(WebCore::RenderTreeUpdater::updateTextRenderer):
(WebCore::RenderTreeUpdater::storePreviousRenderer):

    Use it for tracking state related to render tree siblings. With this we compute whitespace nodes
    correctly for display:contents. The test cases end up depending on that.

* style/RenderTreeUpdater.h:

LayoutTests:

* TestExpectations:

These now pass:

imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-table-001-none.html
imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-table-002-none.html

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@224360 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBlock.cpp
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderTableSection.cpp
Source/WebCore/style/RenderTreePosition.cpp
Source/WebCore/style/RenderTreePosition.h
Source/WebCore/style/RenderTreeUpdater.cpp
Source/WebCore/style/RenderTreeUpdater.h

index f151f38..c9d5e02 100644 (file)
@@ -1,3 +1,17 @@
+2017-11-02  Antti Koivisto  <antti@apple.com>
+
+        display:contents should work with dynamic table mutations
+        https://bugs.webkit.org/show_bug.cgi?id=179179
+
+        Reviewed by Ryosuke Niwa.
+
+        * TestExpectations:
+
+        These now pass:
+
+        imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-table-001-none.html
+        imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-table-002-none.html
+
 2017-11-02  Joseph Pecoraro  <pecoraro@apple.com>
 
         Inspector should display service worker served responses properly
index 1b1095c..3d7f25a 100644 (file)
@@ -1340,14 +1340,12 @@ imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-eleme
 ########################################
 ### START OF display: contents failures
 
-webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-table-002-none.html [ ImageOnlyFailure ]
 webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-flex-003.html [ ImageOnlyFailure ]
 webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-table-001-inline.html [ ImageOnlyFailure ]
 webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-flex-002-none.html [ ImageOnlyFailure ]
 webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-flex-002.html [ ImageOnlyFailure ]
 webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-flow-root-001.html [ ImageOnlyFailure ]
 webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-flex-003-none.html [ ImageOnlyFailure ]
-webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-table-001-none.html [ ImageOnlyFailure ]
 webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-flex-002-inline.html [ ImageOnlyFailure ]
 webkit.org/b/157477 imported/w3c/web-platform-tests/css/css-display-3/display-contents-dynamic-flex-003-inline.html [ ImageOnlyFailure ]
 
index 6becf5b..59ece3c 100644 (file)
@@ -1,3 +1,50 @@
+2017-11-02  Antti Koivisto  <antti@apple.com>
+
+        display:contents should work with dynamic table mutations
+        https://bugs.webkit.org/show_bug.cgi?id=179179
+
+        Reviewed by Ryosuke Niwa.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::addChildIgnoringContinuation):
+
+            RenderText with inline text wrapper as beforeChild is now resolved in RenderTreePosition, covering all cases.
+            Verify this with assert.
+
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::insertChildInternal):
+
+            Add assertion.
+
+        * rendering/RenderTableSection.cpp:
+        (WebCore::RenderTableSection::addChild):
+
+            Fix cases where we did unchecked downcasts for anonymous beforeChild.
+
+        * style/RenderTreePosition.cpp:
+        (WebCore::RenderTreePosition::insert):
+
+            When inserting before a text rendeder with an display:contents inline wrapper, use the wrapper as beforeChild.
+
+        * style/RenderTreePosition.h:
+        (WebCore::RenderTreePosition::insert): Deleted.
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::updateRenderTree):
+        (WebCore::RenderTreeUpdater::renderingParent):
+
+            Add separate helper to get parent frame for the closest rendered (non display:contents) ancestor.
+
+        (WebCore::RenderTreeUpdater::renderTreePosition):
+        (WebCore::RenderTreeUpdater::updateElementRenderer):
+        (WebCore::RenderTreeUpdater::textRendererIsNeeded):
+        (WebCore::RenderTreeUpdater::updateTextRenderer):
+        (WebCore::RenderTreeUpdater::storePreviousRenderer):
+
+            Use it for tracking state related to render tree siblings. With this we compute whitespace nodes
+            correctly for display:contents. The test cases end up depending on that.
+
+        * style/RenderTreeUpdater.h:
+
 2017-11-02  Tim Horton  <timothy_horton@apple.com>
 
         Bump the size of SameAsRenderElement after r224324
index 68635c3..d962d01 100644 (file)
@@ -550,11 +550,8 @@ void RenderBlock::addChildIgnoringContinuation(RenderPtr<RenderObject> newChild,
         ASSERT(beforeChildContainer);
 
         if (beforeChildContainer->isAnonymous()) {
-            if (beforeChildContainer->isInline()) {
-                ASSERT(RenderText::findByDisplayContentsInlineWrapperCandidate(*beforeChildContainer) == beforeChild);
-                addChild(WTFMove(newChild), beforeChildContainer);
-                return;
-            }
+            RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!beforeChildContainer->isInline());
+
             // If the requested beforeChild is not one of our children, then this is because
             // there is an anonymous container within this object that contains the beforeChild.
             RenderElement* beforeChildAnonymousContainer = beforeChildContainer;
@@ -574,6 +571,7 @@ void RenderBlock::addChildIgnoringContinuation(RenderPtr<RenderObject> newChild,
             }
 
             ASSERT(beforeChildAnonymousContainer->isTable());
+
             if (newChild->isTablePart()) {
                 // Insert into the anonymous table.
                 beforeChildAnonymousContainer->addChild(WTFMove(newChild), beforeChild);
@@ -582,12 +580,7 @@ void RenderBlock::addChildIgnoringContinuation(RenderPtr<RenderObject> newChild,
 
             beforeChild = splitAnonymousBoxesAroundChild(beforeChild);
 
-            ASSERT(beforeChild->parent() == this);
-            if (beforeChild->parent() != this) {
-                // We should never reach here. If we do, we need to use the
-                // safe fallback to use the topmost beforeChild container.
-                beforeChild = beforeChildContainer;
-            }
+            RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(beforeChild->parent() == this);
         }
     }
 
index 03b83d0..2c6acbf 100644 (file)
@@ -538,6 +538,7 @@ void RenderElement::insertChildInternal(RenderPtr<RenderObject> newChildPtr, Ren
         beforeChild = beforeChild->parent();
 
     ASSERT(!beforeChild || beforeChild->parent() == this);
+    ASSERT(!is<RenderText>(beforeChild) || !downcast<RenderText>(*beforeChild).inlineWrapperForDisplayContents());
 
     // Take the ownership.
     auto* newChild = newChildPtr.release();
index 008f54b..abde213 100644 (file)
@@ -122,7 +122,7 @@ void RenderTableSection::addChild(RenderPtr<RenderObject> child, RenderObject* b
         RenderObject* last = beforeChild;
         if (!last)
             last = lastRow();
-        if (last && last->isAnonymous() && !last->isBeforeOrAfterContent()) {
+        if (is<RenderTableRow>(last) && last->isAnonymous() && !last->isBeforeOrAfterContent()) {
             RenderTableRow& row = downcast<RenderTableRow>(*last);
             if (beforeChild == &row)
                 beforeChild = row.firstCell();
@@ -143,7 +143,7 @@ void RenderTableSection::addChild(RenderPtr<RenderObject> child, RenderObject* b
         RenderObject* lastBox = last;
         while (lastBox && lastBox->parent()->isAnonymous() && !is<RenderTableRow>(*lastBox))
             lastBox = lastBox->parent();
-        if (lastBox && lastBox->isAnonymous() && !lastBox->isBeforeOrAfterContent()) {
+        if (is<RenderTableRow>(lastBox) && lastBox->isAnonymous() && !lastBox->isBeforeOrAfterContent()) {
             downcast<RenderTableRow>(*lastBox).addChild(WTFMove(child), beforeChild);
             return;
         }
index f17d42e..067a763 100644 (file)
 
 #include "ComposedTreeIterator.h"
 #include "PseudoElement.h"
+#include "RenderInline.h"
 #include "RenderObject.h"
 #include "ShadowRoot.h"
 
 namespace WebCore {
 
+void RenderTreePosition::insert(RenderPtr<RenderObject> renderer)
+{
+    ASSERT(m_hasValidNextSibling);
+    auto* insertBefore = m_nextSibling;
+    if (is<RenderText>(insertBefore)) {
+        if (auto* wrapperInline = downcast<RenderText>(*insertBefore).inlineWrapperForDisplayContents())
+            insertBefore = wrapperInline;
+    }
+    m_parent.addChild(WTFMove(renderer), insertBefore);
+}
+
 void RenderTreePosition::computeNextSibling(const Node& node)
 {
     ASSERT(!node.renderer());
index c10da76..8d89fae 100644 (file)
@@ -78,10 +78,4 @@ inline bool RenderTreePosition::canInsert(RenderText& renderer) const
     return m_parent.isChildAllowed(renderer, m_parent.style());
 }
 
-inline void RenderTreePosition::insert(RenderPtr<RenderObject> renderer)
-{
-    ASSERT(m_hasValidNextSibling);
-    m_parent.addChild(WTFMove(renderer), m_nextSibling);
-}
-
 } // namespace WebCore
index 3de720f..b99c828 100644 (file)
@@ -175,7 +175,7 @@ void RenderTreeUpdater::updateRenderTree(ContainerNode& root)
             auto& text = downcast<Text>(node);
             auto* textUpdate = m_styleUpdate->textUpdate(text);
             bool didCreateParent = parent().updates && parent().updates->update.change == Style::Detach;
-            bool mayNeedUpdateWhitespaceOnlyRenderer = parent().didCreateOrDestroyChildRenderer && text.containsOnlyWhitespace();
+            bool mayNeedUpdateWhitespaceOnlyRenderer = renderingParent().didCreateOrDestroyChildRenderer && text.containsOnlyWhitespace();
             if (didCreateParent || textUpdate || mayNeedUpdateWhitespaceOnlyRenderer)
                 updateTextRenderer(text, textUpdate);
 
@@ -215,14 +215,19 @@ void RenderTreeUpdater::updateRenderTree(ContainerNode& root)
     popParentsToDepth(0);
 }
 
-RenderTreePosition& RenderTreeUpdater::renderTreePosition()
+auto RenderTreeUpdater::renderingParent() -> Parent&
 {
-    for (unsigned i = m_parentStack.size(); i; --i) {
-        if (auto& position = m_parentStack[i - 1].renderTreePosition)
-            return *position;
+    for (unsigned i = m_parentStack.size(); i--;) {
+        if (m_parentStack[i].renderTreePosition)
+            return m_parentStack[i];
     }
     ASSERT_NOT_REACHED();
-    return *m_parentStack.last().renderTreePosition;
+    return m_parentStack.last();
+}
+
+RenderTreePosition& RenderTreeUpdater::renderTreePosition()
+{
+    return *renderingParent().renderTreePosition;
 }
 
 void RenderTreeUpdater::pushParent(Element& element, const Style::ElementUpdates* updates)
@@ -314,7 +319,7 @@ void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::Ele
         auto teardownType = update.style->display() == NONE ? TeardownType::RendererUpdateCancelingAnimations : TeardownType::RendererUpdate;
         tearDownRenderers(element, teardownType);
 
-        parent().didCreateOrDestroyChildRenderer = true;
+        renderingParent().didCreateOrDestroyChildRenderer = true;
     }
 
     bool hasDisplayContents = update.style->display() == CONTENTS;
@@ -329,7 +334,7 @@ void RenderTreeUpdater::updateElementRenderer(Element& element, const Style::Ele
             element.willAttachRenderers();
         createRenderer(element, RenderStyle::clone(*update.style));
 
-        parent().didCreateOrDestroyChildRenderer = true;
+        renderingParent().didCreateOrDestroyChildRenderer = true;
         return;
     }
 
@@ -410,7 +415,7 @@ bool RenderTreeUpdater::textRendererIsNeeded(const Text& textNode)
     if (parentRenderer.style().preserveNewline()) // pre/pre-wrap/pre-line always make renderers.
         return true;
 
-    auto* previousRenderer = parent().previousChildRenderer;
+    auto* previousRenderer = renderingParent().previousChildRenderer;
     if (previousRenderer && previousRenderer->isBR()) // <span><br/> <br/></span>
         return false;
 
@@ -483,13 +488,13 @@ void RenderTreeUpdater::updateTextRenderer(Text& text, const Style::TextUpdate*
             return;
         }
         tearDownRenderer(text);
-        parent().didCreateOrDestroyChildRenderer = true;
+        renderingParent().didCreateOrDestroyChildRenderer = true;
         return;
     }
     if (!needsRenderer)
         return;
     createTextRenderer(text, renderTreePosition(), textUpdate);
-    parent().didCreateOrDestroyChildRenderer = true;
+    renderingParent().didCreateOrDestroyChildRenderer = true;
 }
 
 void RenderTreeUpdater::storePreviousRenderer(Node& node)
@@ -497,8 +502,8 @@ void RenderTreeUpdater::storePreviousRenderer(Node& node)
     auto* renderer = node.renderer();
     if (!renderer)
         return;
-    ASSERT(parent().previousChildRenderer != renderer);
-    parent().previousChildRenderer = renderer;
+    ASSERT(renderingParent().previousChildRenderer != renderer);
+    renderingParent().previousChildRenderer = renderer;
 }
 
 void RenderTreeUpdater::tearDownRenderers(Element& root)
index 026355f..ca4000b 100644 (file)
@@ -78,6 +78,7 @@ private:
         Parent(Element&, const Style::ElementUpdates*);
     };
     Parent& parent() { return m_parentStack.last(); }
+    Parent& renderingParent();
     RenderTreePosition& renderTreePosition();
 
     GeneratedContent& generatedContent() { return *m_generatedContent; }