REGRESSION (r198943): Transitions don't work if they animate display property
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 May 2016 18:00:04 +0000 (18:00 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 May 2016 18:00:04 +0000 (18:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157244
<rdar://problem/26042189>

Reviewed by Simon Fraser.

Source/WebCore:

Test: transitions/transition-display-property.html

* style/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::updateBeforeOrAfterPseudoElement):

    Call the common function for ::before/::after updates.

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

    If animation forces render tree reconstruction use the original rather than animated style for update.
    Because animations are tied to renderers we start them during renderer construction in this case.

    Factor to a function.

(WebCore::Style::elementImplicitVisibility):
* style/StyleTreeResolver.h:

LayoutTests:

* transitions/transition-display-property-expected.html: Added.
* transitions/transition-display-property.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/transitions/transition-display-property-expected.html [new file with mode: 0644]
LayoutTests/transitions/transition-display-property.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/style/RenderTreeUpdater.cpp
Source/WebCore/style/StyleTreeResolver.cpp
Source/WebCore/style/StyleTreeResolver.h

index 45fbab5..4f93dee 100644 (file)
@@ -1,3 +1,14 @@
+2016-05-02  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r198943): Transitions don't work if they animate display property
+        https://bugs.webkit.org/show_bug.cgi?id=157244
+        <rdar://problem/26042189>
+
+        Reviewed by Simon Fraser.
+
+        * transitions/transition-display-property-expected.html: Added.
+        * transitions/transition-display-property.html: Added.
+
 2016-05-03  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, drop outdated layout test after r200375.
diff --git a/LayoutTests/transitions/transition-display-property-expected.html b/LayoutTests/transitions/transition-display-property-expected.html
new file mode 100644 (file)
index 0000000..8c793ec
--- /dev/null
@@ -0,0 +1,4 @@
+<style>
+test { display:inline-block; height:10px; width:10px; border: 2px solid green; }
+</style>
+<test></test>
diff --git a/LayoutTests/transitions/transition-display-property.html b/LayoutTests/transitions/transition-display-property.html
new file mode 100644 (file)
index 0000000..d89ff3b
--- /dev/null
@@ -0,0 +1,23 @@
+<style>
+test { transition:0.1s; display:block; height:100px; border: 2px solid green; }
+.animate { display:inline-block; height:10px; width:10px; }
+</style>
+<test></test>
+<script>
+var test = document.querySelector("test");
+var count = 0;
+function testUntilComplete()
+{
+    if (test.offsetWidth == 10 || ++count == 10)
+        testRunner.notifyDone();
+    else
+        setTimeout(testUntilComplete, 0.1);
+}
+
+if (window.testRunner) {
+    testRunner.waitUntilDone();
+    testUntilComplete();
+}
+test.offsetWidth;
+test.className = "animate";
+</script>
index 7621c2f..7a6697d 100644 (file)
@@ -1,3 +1,30 @@
+2016-05-02  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r198943): Transitions don't work if they animate display property
+        https://bugs.webkit.org/show_bug.cgi?id=157244
+        <rdar://problem/26042189>
+
+        Reviewed by Simon Fraser.
+
+        Test: transitions/transition-display-property.html
+
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::updateBeforeOrAfterPseudoElement):
+
+            Call the common function for ::before/::after updates.
+
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::resolveElement):
+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
+
+            If animation forces render tree reconstruction use the original rather than animated style for update.
+            Because animations are tied to renderers we start them during renderer construction in this case.
+
+            Factor to a function.
+
+        (WebCore::Style::elementImplicitVisibility):
+        * style/StyleTreeResolver.h:
+
 2016-05-03  Pranjal Jumde  <pjumde@apple.com>
 
         WorkerGlobalScope's self, location and navigator attributes should not be replaceable
index bb34a72..cd9009e 100644 (file)
@@ -480,16 +480,9 @@ void RenderTreeUpdater::updateBeforeOrAfterPseudoElement(Element& current, Pseud
         return;
     }
 
-    Style::ElementUpdate elementUpdate;
-
     auto newStyle = RenderStyle::clonePtr(*current.renderer()->getCachedPseudoStyle(pseudoId, &current.renderer()->style()));
 
-    std::unique_ptr<RenderStyle> animatedStyle;
-    if (renderer && m_document.frame()->animation().updateAnimations(*renderer, *newStyle, animatedStyle))
-        elementUpdate.isSynthetic = true;
-
-    elementUpdate.style = animatedStyle ? WTFMove(animatedStyle) : WTFMove(newStyle);
-    elementUpdate.change = renderer ? Style::determineChange(renderer->style(), *elementUpdate.style) : Style::Detach;
+    auto elementUpdate = Style::TreeResolver::createAnimatedElementUpdate(WTFMove(newStyle), renderer, m_document);
 
     if (elementUpdate.change == Style::NoChange)
         return;
index a1c0d4d..686a099 100644 (file)
@@ -185,21 +185,13 @@ ElementUpdate TreeResolver::resolveElement(Element& element)
 {
     auto newStyle = styleForElement(element, parent().style);
 
-    auto* renderer = element.renderer();
-
     if (!affectsRenderedSubtree(element, *newStyle))
         return { };
 
-    ElementUpdate update;
-
-    bool needsNewRenderer = !renderer || element.styleChangeType() == ReconstructRenderTree || parent().change == Detach;
+    bool shouldReconstructRenderTree = element.styleChangeType() == ReconstructRenderTree || parent().change == Detach;
+    auto* rendererToUpdate = shouldReconstructRenderTree ? nullptr : element.renderer();
 
-    std::unique_ptr<RenderStyle> animatedStyle;
-    if (!needsNewRenderer && m_document.frame()->animation().updateAnimations(*renderer, *newStyle, animatedStyle))
-        update.isSynthetic = true;
-
-    update.style = animatedStyle ? WTFMove(animatedStyle) : WTFMove(newStyle);
-    update.change = needsNewRenderer ? Detach : determineChange(renderer->style(), *update.style);
+    auto update = createAnimatedElementUpdate(WTFMove(newStyle), rendererToUpdate, m_document);
 
     if (element.styleChangeType() == SyntheticStyleChange)
         update.isSynthetic = true;
@@ -210,7 +202,7 @@ ElementUpdate TreeResolver::resolveElement(Element& element)
 
         // If "rem" units are used anywhere in the document, and if the document element's font size changes, then force font updating
         // all the way down the tree. This is simpler than having to maintain a cache of objects (and such font size changes should be rare anyway).
-        if (m_document.authorStyleSheets().usesRemUnits() && update.change != NoChange && renderer && renderer->style().fontSize() != update.style->fontSize()) {
+        if (m_document.authorStyleSheets().usesRemUnits() && update.change != NoChange && element.renderer() && element.renderer()->style().fontSize() != update.style->fontSize()) {
             // Cached RenderStyles may depend on the rem units.
             scope().styleResolver.invalidateMatchedPropertiesCache();
             update.change = Force;
@@ -228,6 +220,27 @@ ElementUpdate TreeResolver::resolveElement(Element& element)
     return update;
 }
 
+ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderStyle> newStyle, RenderElement* rendererToUpdate, Document& document)
+{
+    ElementUpdate update;
+
+    std::unique_ptr<RenderStyle> animatedStyle;
+    if (rendererToUpdate && document.frame()->animation().updateAnimations(*rendererToUpdate, *newStyle, animatedStyle))
+        update.isSynthetic = true;
+
+    if (animatedStyle) {
+        update.change = determineChange(rendererToUpdate->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);
+    }
+
+    return update;
+}
+
 #if PLATFORM(IOS)
 static EVisibility elementImplicitVisibility(const Element* element)
 {
index 548c76e..fa70799 100644 (file)
@@ -60,6 +60,8 @@ public:
 
     std::unique_ptr<Update> resolve(Change);
 
+    static ElementUpdate createAnimatedElementUpdate(std::unique_ptr<RenderStyle>, RenderElement* existingRenderer, Document&);
+
 private:
     std::unique_ptr<RenderStyle> styleForElement(Element&, const RenderStyle& inheritedStyle);