Slotted nodes ignore transition
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Nov 2016 10:59:23 +0000 (10:59 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Nov 2016 10:59:23 +0000 (10:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=160866
<rdar://problem/29231901>

Reviewed by Sam Weinig.

Source/WebCore:

The problem is that slot (display:contents) always triggers full render tree rebuild when something
changes in the slotted subtree. This causes animation to jump to end (may be another bug).

Test: fast/shadow-dom/shadow-host-transition.html

* style/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::updateElementRenderer):
(WebCore::RenderTreeUpdater::updateBeforeOrAfterPseudoElement):
* style/StyleChange.h:

    Rearrange so the strongest ('Detach') is the highest.

* style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::resolveElement):
(WebCore::Style::TreeResolver::createAnimatedElementUpdate):

    If style was display:contents and stays that way, use 'Inherit' StyleChange which doesn't force render tree rebuild.
    Refactor more of the functionality to createAnimatedElementUpdate.

* style/StyleTreeResolver.h:

LayoutTests:

* fast/shadow-dom/shadow-host-transition-expected.html: Added.
* fast/shadow-dom/shadow-host-transition.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/shadow-dom/shadow-host-transition-expected.html [new file with mode: 0644]
LayoutTests/fast/shadow-dom/shadow-host-transition.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/style/RenderTreeUpdater.cpp
Source/WebCore/style/StyleChange.h
Source/WebCore/style/StyleTreeResolver.cpp
Source/WebCore/style/StyleTreeResolver.h

index 060b610..b5d519c 100644 (file)
@@ -1,3 +1,14 @@
+2016-11-28  Antti Koivisto  <antti@apple.com>
+
+        Slotted nodes ignore transition
+        https://bugs.webkit.org/show_bug.cgi?id=160866
+        <rdar://problem/29231901>
+
+        Reviewed by Sam Weinig.
+
+        * fast/shadow-dom/shadow-host-transition-expected.html: Added.
+        * fast/shadow-dom/shadow-host-transition.html: Added.
+
 2016-11-28  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: Debugger should have an option for showing asynchronous call stacks
diff --git a/LayoutTests/fast/shadow-dom/shadow-host-transition-expected.html b/LayoutTests/fast/shadow-dom/shadow-host-transition-expected.html
new file mode 100644 (file)
index 0000000..e704d24
--- /dev/null
@@ -0,0 +1,7 @@
+<!DOCTYPE html>
+<html>
+<body>
+    <p>Test passes if you see a single 100px by 100px green box below.</p>
+    <div style="width: 100px; height: 100px; background: green;"></div>
+</body>
+</html>
diff --git a/LayoutTests/fast/shadow-dom/shadow-host-transition.html b/LayoutTests/fast/shadow-dom/shadow-host-transition.html
new file mode 100644 (file)
index 0000000..b11e2b5
--- /dev/null
@@ -0,0 +1,111 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+.test {
+    width: 100px;
+    height: 25px;
+    background: red;
+    color: green;
+}
+
+#host1.green {
+    transition: background 0.1s;
+    background: green !important;
+}
+
+#host3 .green {
+    transition: background 0.1s;
+    background: green !important;
+    height: 100%;
+}
+
+</style>
+</head>
+<body>
+<p>Test passes if you see a single 100px by 100px green box below.</p>
+<div id="host1" class="test"><div>text</div></div>
+<div id="host2" class="test"><div>text</div></div>
+<div id="host3" class="test"><div>text</div></div>
+<div id="host4" class="test"><div>text</div></div>
+<script>
+
+if (window.testRunner)
+    testRunner.waitUntilDone();
+
+let expectedEventCount = 0;
+let eventCount = 0;
+
+function transitionEndEvent()
+{
+    ++eventCount;
+    if (eventCount == expectedEventCount) {
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }
+}
+
+{
+    const host = document.getElementById('host1');
+    host.attachShadow({mode: 'closed'}).innerHTML = `
+        <slot></slot>
+    `;
+
+    getComputedStyle(host.firstChild).backgroundColor;
+    ++expectedEventCount;
+    document.addEventListener('transitionend', transitionEndEvent);
+    host.classList.toggle('green');
+}
+
+{
+    const host = document.getElementById('host2');
+    host.attachShadow({mode: 'closed'}).innerHTML = `
+    <style>
+    :host(.green) {
+        transition: background 0.1s;
+        background: green !important;
+    }
+    </style>
+    <slot></slot>
+    `;
+
+    getComputedStyle(host.firstChild).backgroundColor;
+    ++expectedEventCount;
+    document.addEventListener('transitionend', transitionEndEvent);
+    host.classList.toggle('green');
+}
+
+{
+    const host = document.getElementById('host3');
+    host.attachShadow({mode: 'closed'}).innerHTML = `
+    <slot></slot>
+    `;
+
+    getComputedStyle(host.firstChild).backgroundColor;
+    ++expectedEventCount;
+    document.addEventListener('transitionend', transitionEndEvent);
+    host.firstChild.classList.toggle('green');
+}
+
+{
+    const host = document.getElementById('host4');
+    host.attachShadow({mode: 'closed'}).innerHTML = `
+    <style>
+    ::slotted(.green) {
+        transition: background 0.1s;
+        background: green !important;
+        height: 100%;
+    }
+    </style>
+    <slot></slot>
+    `;
+
+    getComputedStyle(host.firstChild).backgroundColor;
+    ++expectedEventCount;
+    document.addEventListener('transitionend', transitionEndEvent);
+    host.firstChild.classList.toggle('green');
+}
+
+</script>
+</body>
+</html>
index 4bc5745..fc29eab 100644 (file)
@@ -1,3 +1,32 @@
+2016-11-28  Antti Koivisto  <antti@apple.com>
+
+        Slotted nodes ignore transition
+        https://bugs.webkit.org/show_bug.cgi?id=160866
+        <rdar://problem/29231901>
+
+        Reviewed by Sam Weinig.
+
+        The problem is that slot (display:contents) always triggers full render tree rebuild when something
+        changes in the slotted subtree. This causes animation to jump to end (may be another bug).
+
+        Test: fast/shadow-dom/shadow-host-transition.html
+
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::updateElementRenderer):
+        (WebCore::RenderTreeUpdater::updateBeforeOrAfterPseudoElement):
+        * style/StyleChange.h:
+
+            Rearrange so the strongest ('Detach') is the highest.
+
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::resolveElement):
+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
+
+            If style was display:contents and stays that way, use 'Inherit' StyleChange which doesn't force render tree rebuild.
+            Refactor more of the functionality to createAnimatedElementUpdate.
+
+        * style/StyleTreeResolver.h:
+
 2016-11-28  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK] Crash in WebCore::PlatformDisplayX11::supportsXComposite when running under Wayland
index 08b63e3..6d74af7 100644 (file)
@@ -257,14 +257,14 @@ void RenderTreeUpdater::updateElementRenderer(Element& element, Style::ElementUp
     if (shouldTearDownRenderers)
         tearDownRenderers(element, TeardownType::KeepHoverAndActive);
 
-    bool hasDisplayContest = update.style && update.style->display() == CONTENTS;
-    if (hasDisplayContest != element.hasDisplayContents()) {
-        element.setHasDisplayContents(hasDisplayContest);
+    bool hasDisplayContents = update.style && update.style->display() == CONTENTS;
+    if (hasDisplayContents != element.hasDisplayContents()) {
+        element.setHasDisplayContents(hasDisplayContents);
         // Render tree position needs to be recomputed as rendering siblings may be found from the display:contents subtree.
         renderTreePosition().invalidateNextSibling();
     }
 
-    bool shouldCreateNewRenderer = !element.renderer() && update.style && !hasDisplayContest;
+    bool shouldCreateNewRenderer = !element.renderer() && update.style && !hasDisplayContents;
     if (shouldCreateNewRenderer) {
         if (element.hasCustomStyleResolveCallbacks())
             element.willAttachRenderers();
@@ -479,8 +479,7 @@ void RenderTreeUpdater::updateBeforeOrAfterPseudoElement(Element& current, Pseud
 {
     PseudoElement* pseudoElement = pseudoId == BEFORE ? current.beforePseudoElement() : current.afterPseudoElement();
 
-    auto* renderer = pseudoElement ? pseudoElement->renderer() : nullptr;
-    if (renderer)
+    if (auto* renderer = pseudoElement ? pseudoElement->renderer() : nullptr)
         renderTreePosition().invalidateNextSibling(*renderer);
 
     bool needsPseudoElement = WebCore::needsPseudoElement(current, pseudoId);
@@ -494,21 +493,25 @@ void RenderTreeUpdater::updateBeforeOrAfterPseudoElement(Element& current, Pseud
         return;
     }
 
+    RefPtr<PseudoElement> newPseudoElement;
+    if (!pseudoElement) {
+        newPseudoElement = PseudoElement::create(current, pseudoId);
+        pseudoElement = newPseudoElement.get();
+    }
+
     auto newStyle = RenderStyle::clonePtr(*current.renderer()->getCachedPseudoStyle(pseudoId, &current.renderer()->style()));
 
-    auto elementUpdate = Style::TreeResolver::createAnimatedElementUpdate(WTFMove(newStyle), renderer, m_document);
+    auto elementUpdate = Style::TreeResolver::createAnimatedElementUpdate(WTFMove(newStyle), *pseudoElement, Style::NoChange);
 
     if (elementUpdate.change == Style::NoChange)
         return;
 
-    if (!pseudoElement) {
-        auto newPseudoElement = PseudoElement::create(current, pseudoId);
-        pseudoElement = newPseudoElement.ptr();
-        InspectorInstrumentation::pseudoElementCreated(m_document.page(), newPseudoElement);
+    if (newPseudoElement) {
+        InspectorInstrumentation::pseudoElementCreated(m_document.page(), *newPseudoElement);
         if (pseudoId == BEFORE)
-            current.setBeforePseudoElement(WTFMove(newPseudoElement));
+            current.setBeforePseudoElement(newPseudoElement.releaseNonNull());
         else
-            current.setAfterPseudoElement(WTFMove(newPseudoElement));
+            current.setAfterPseudoElement(newPseudoElement.releaseNonNull());
     }
 
     updateElementRenderer(*pseudoElement, elementUpdate);
index 25424e5..de54cbb 100644 (file)
@@ -31,7 +31,7 @@ class RenderStyle;
 
 namespace Style {
 
-enum Change { NoChange, NoInherit, Inherit, Detach, Force };
+enum Change { NoChange, NoInherit, Inherit, Force, Detach };
 
 Change determineChange(const RenderStyle&, const RenderStyle&);
 
index e7dbaef..46bc1a3 100644 (file)
@@ -200,13 +200,7 @@ ElementUpdate TreeResolver::resolveElement(Element& element)
     if (!affectsRenderedSubtree(element, *newStyle))
         return { };
 
-    bool shouldReconstructRenderTree = element.styleValidity() >= Validity::SubtreeAndRenderersInvalid || parent().change == Detach;
-    auto* rendererToUpdate = shouldReconstructRenderTree ? nullptr : element.renderer();
-
-    auto update = createAnimatedElementUpdate(WTFMove(newStyle), rendererToUpdate, m_document);
-
-    if (element.styleResolutionShouldRecompositeLayer())
-        update.recompositeLayer = true;
+    auto update = createAnimatedElementUpdate(WTFMove(newStyle), element, parent().change);
 
     auto* existingStyle = element.renderStyle();
 
@@ -218,7 +212,7 @@ ElementUpdate TreeResolver::resolveElement(Element& element)
             // "rem" units are relative to the document element's font size so we need to recompute everything.
             // In practice this is rare.
             scope().styleResolver.invalidateMatchedPropertiesCache();
-            update.change = Force;
+            update.change = std::max(update.change, Force);
         }
     }
 
@@ -233,9 +227,6 @@ ElementUpdate TreeResolver::resolveElement(Element& element)
             update.change = Detach;
     }
 
-    if (update.change != Detach && (parent().change == Force || element.styleValidity() >= Validity::SubtreeInvalid))
-        update.change = Force;
-
     return update;
 }
 
@@ -253,25 +244,45 @@ const RenderStyle* TreeResolver::parentBoxStyle() const
     return nullptr;
 }
 
-ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderStyle> newStyle, RenderElement* rendererToUpdate, Document& document)
+ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderStyle> newStyle, Element& element, Change parentChange)
 {
-    ElementUpdate update;
+    auto validity = element.styleValidity();
+    bool recompositeLayer = element.styleResolutionShouldRecompositeLayer();
+
+    auto makeUpdate = [&] (std::unique_ptr<RenderStyle> style, Change change) {
+        if (validity >= Validity::SubtreeInvalid)
+            change = std::max(change, validity == Validity::SubtreeAndRenderersInvalid ? Detach : Force);
+        if (parentChange >= Force)
+            change = std::max(change, parentChange);
+        return ElementUpdate { WTFMove(style), change, recompositeLayer };
+    };
+
+    auto* renderer = element.renderer();
+
+    bool shouldReconstruct = validity >= Validity::SubtreeAndRenderersInvalid || parentChange == Detach;
+    if (shouldReconstruct)
+        return makeUpdate(WTFMove(newStyle), Detach);
+
+    if (!renderer) {
+        auto keepsDisplayContents = newStyle->display() == CONTENTS && element.hasDisplayContents();
+        // Some inherited property might have changed.
+        return makeUpdate(WTFMove(newStyle), keepsDisplayContents ? Inherit : Detach);
+    }
 
     std::unique_ptr<RenderStyle> animatedStyle;
-    if (rendererToUpdate && document.frame()->animation().updateAnimations(*rendererToUpdate, *newStyle, animatedStyle))
-        update.recompositeLayer = true;
+    if (element.document().frame()->animation().updateAnimations(*renderer, *newStyle, animatedStyle))
+        recompositeLayer = true;
 
     if (animatedStyle) {
-        update.change = determineChange(rendererToUpdate->style(), *animatedStyle);
+        auto change = determineChange(renderer->style(), *animatedStyle);
         // If animation forces render tree reconstruction pass the original style. The animation will be applied on renderer construction.
         // FIXME: We should always use the animated style here.
-        update.style = update.change == Detach ? WTFMove(newStyle) : WTFMove(animatedStyle);
-    } else {
-        update.change = rendererToUpdate ? determineChange(rendererToUpdate->style(), *newStyle) : Detach;
-        update.style = WTFMove(newStyle);
+        auto style = change == Detach ? WTFMove(newStyle) : WTFMove(animatedStyle);
+        return makeUpdate(WTFMove(style), change);
     }
 
-    return update;
+    auto change = determineChange(renderer->style(), *newStyle);
+    return makeUpdate(WTFMove(newStyle), change);
 }
 
 void TreeResolver::pushParent(Element& element, const RenderStyle& style, Change change)
index 755cf4f..4fb8be5 100644 (file)
@@ -32,6 +32,7 @@
 #include "StyleChange.h"
 #include "StyleSharingResolver.h"
 #include "StyleUpdate.h"
+#include "StyleValidity.h"
 #include <wtf/Function.h>
 #include <wtf/RefPtr.h>
 
@@ -53,7 +54,7 @@ public:
 
     std::unique_ptr<Update> resolve(Change);
 
-    static ElementUpdate createAnimatedElementUpdate(std::unique_ptr<RenderStyle>, RenderElement* existingRenderer, Document&);
+    static ElementUpdate createAnimatedElementUpdate(std::unique_ptr<RenderStyle>, Element&, Change parentChange);
 
 private:
     std::unique_ptr<RenderStyle> styleForElement(Element&, const RenderStyle& inheritedStyle);