REGRESSION (r207372) Visibility property is not inherited when used in an animation
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 May 2017 22:23:11 +0000 (22:23 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 May 2017 22:23:11 +0000 (22:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=171883
<rdar://problem/32086550>

Reviewed by Simon Fraser.

Source/WebCore:

The problem here is that our animation code is tied to renderers. We don't have renderers during
the initial style resolution so animations are not applied yet. When constructing renderers we set
their style to the initial animated style but this step can't implement inheritance.

Normally this is invisible as the first animation frame will immediately inherit the style correctly.
However in this case the animation is discrete and the first frame is the same as the initial state.
With r207372 we optimize the descendant style change away.

This patch fixes the problem by tracking that the renderer has initial animated style and inheriting
it to descendants during next style resolution even if it doesn't change.

Test: animations/animation-initial-inheritance.html

* rendering/RenderElement.cpp:
(WebCore::RenderElement::RenderElement):
* rendering/RenderElement.h:
(WebCore::RenderElement::hasInitialAnimatedStyle):
(WebCore::RenderElement::setHasInitialAnimatedStyle):
* style/RenderTreeUpdater.cpp:
(WebCore::RenderTreeUpdater::createRenderer):

    Set a bit on renderer indicating it has initial animated style.

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

    Return at least 'Inherit' for style change when updating renderer with initial animated style.

LayoutTests:

* animations/animation-initial-inheritance-expected.html: Added.
* animations/animation-initial-inheritance.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/animations/animation-initial-inheritance-expected.html [new file with mode: 0644]
LayoutTests/animations/animation-initial-inheritance.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderElement.h
Source/WebCore/style/RenderTreeUpdater.cpp
Source/WebCore/style/StyleTreeResolver.cpp

index dca2c15..5dede9e 100644 (file)
@@ -1,3 +1,14 @@
+2017-05-10  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r207372) Visibility property is not inherited when used in an animation
+        https://bugs.webkit.org/show_bug.cgi?id=171883
+        <rdar://problem/32086550>
+
+        Reviewed by Simon Fraser.
+
+        * animations/animation-initial-inheritance-expected.html: Added.
+        * animations/animation-initial-inheritance.html: Added.
+
 2017-05-10  Matt Lewis  <jlewis3@apple.com>
 
         Marked transitions/extra-transition.html as flaky.
diff --git a/LayoutTests/animations/animation-initial-inheritance-expected.html b/LayoutTests/animations/animation-initial-inheritance-expected.html
new file mode 100644 (file)
index 0000000..e52daa7
--- /dev/null
@@ -0,0 +1,23 @@
+<!doctype html>
+<html>
+<head>
+<style>
+#test1 {
+    visibility: hidden;
+}
+#test2 {
+    color: green;
+}
+</style>
+</head>
+<body>
+<div id=test1>
+This shouldn't be initially visible
+<div>This shouldn't be initially visible</div>
+</div>
+<div id=test2>
+This should be initially green
+<div>This should be initially green</div>
+</div>
+</body>
+</html>
diff --git a/LayoutTests/animations/animation-initial-inheritance.html b/LayoutTests/animations/animation-initial-inheritance.html
new file mode 100644 (file)
index 0000000..be88599
--- /dev/null
@@ -0,0 +1,39 @@
+<!doctype html>
+<html>
+<head>
+<style>
+#test1 {
+    animation:test1 3s steps(1,end) 0s 1 normal both
+}
+#test2 {
+    animation:test2 3s steps(1,end) 0s 1 normal both
+}
+@keyframes test1 {
+    from {visibility: hidden}
+    to {visibility: visible}
+}
+@keyframes test2 {
+    from {color: green}
+    to {color: red}
+}
+</style>
+<script>
+if (window.testRunner) {
+    testRunner.waitUntilDone();
+    requestAnimationFrame(() => {
+        testRunner.notifyDone();
+    });
+}
+</script>
+</head>
+<body>
+<div id=test1>
+This shouldn't be initially visible
+<div>This shouldn't be initially visible</div>
+</div>
+<div id=test2>
+This should be initially green
+<div>This should be initially green</div>
+</div>
+</body>
+</html>
index e523f67..3c8fe66 100644 (file)
@@ -1,3 +1,39 @@
+2017-05-10  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION (r207372) Visibility property is not inherited when used in an animation
+        https://bugs.webkit.org/show_bug.cgi?id=171883
+        <rdar://problem/32086550>
+
+        Reviewed by Simon Fraser.
+
+        The problem here is that our animation code is tied to renderers. We don't have renderers during
+        the initial style resolution so animations are not applied yet. When constructing renderers we set
+        their style to the initial animated style but this step can't implement inheritance.
+
+        Normally this is invisible as the first animation frame will immediately inherit the style correctly.
+        However in this case the animation is discrete and the first frame is the same as the initial state.
+        With r207372 we optimize the descendant style change away.
+
+        This patch fixes the problem by tracking that the renderer has initial animated style and inheriting
+        it to descendants during next style resolution even if it doesn't change.
+
+        Test: animations/animation-initial-inheritance.html
+
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::RenderElement):
+        * rendering/RenderElement.h:
+        (WebCore::RenderElement::hasInitialAnimatedStyle):
+        (WebCore::RenderElement::setHasInitialAnimatedStyle):
+        * style/RenderTreeUpdater.cpp:
+        (WebCore::RenderTreeUpdater::createRenderer):
+
+            Set a bit on renderer indicating it has initial animated style.
+
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
+
+            Return at least 'Inherit' for style change when updating renderer with initial animated style.
+
 2017-05-10  Jer Noble  <jer.noble@apple.com>
 
         RELEASE_ASSERT at WebAudioSourceProviderAVFObjC::provideInput()
index dbde69e..87bd5e1 100644 (file)
@@ -81,9 +81,7 @@
 namespace WebCore {
 
 struct SameSizeAsRenderElement : public RenderObject {
-    uint8_t bitfields0;
-    uint8_t bitfields1;
-    uint8_t bitfields2;
+    uint32_t bitfields;
     void* firstChild;
     void* lastChild;
     RenderStyle style;
@@ -102,6 +100,7 @@ inline RenderElement::RenderElement(ContainerNode& elementOrDocument, RenderStyl
     , m_baseTypeFlags(baseTypeFlags)
     , m_ancestorLineBoxDirty(false)
     , m_hasInitializedStyle(false)
+    , m_hasInitialAnimatedStyle(false)
     , m_renderInlineAlwaysCreatesLineBoxes(false)
     , m_renderBoxNeedsLazyRepaint(false)
     , m_hasPausedImageAnimations(false)
index f9a736c..2940a46 100644 (file)
@@ -133,6 +133,9 @@ public:
     // and so only should be called when the style is known not to have changed (or from setStyle).
     void setStyleInternal(RenderStyle&& style) { m_style = WTFMove(style); }
 
+    bool hasInitialAnimatedStyle() const { return m_hasInitialAnimatedStyle; }
+    void setHasInitialAnimatedStyle(bool b) { m_hasInitialAnimatedStyle = b; }
+
     // Repaint only if our old bounds and new bounds are different. The caller may pass in newBounds and newOutlineBox if they are known.
     bool repaintAfterLayoutIfNeeded(const RenderLayerModelObject* repaintContainer, const LayoutRect& oldBounds, const LayoutRect& oldOutlineBox, const LayoutRect* newBoundsPtr = nullptr, const LayoutRect* newOutlineBoxPtr = nullptr);
 
@@ -329,6 +332,7 @@ private:
     unsigned m_baseTypeFlags : 6;
     unsigned m_ancestorLineBoxDirty : 1;
     unsigned m_hasInitializedStyle : 1;
+    unsigned m_hasInitialAnimatedStyle : 1;
 
     unsigned m_renderInlineAlwaysCreatesLineBoxes : 1;
     unsigned m_renderBoxNeedsLazyRepaint : 1;
index 61ea61c..df4b801 100644 (file)
@@ -353,8 +353,10 @@ void RenderTreeUpdater::createRenderer(Element& element, RenderStyle&& style)
     auto& initialStyle = newRenderer->style();
     std::unique_ptr<RenderStyle> animatedStyle;
     newRenderer->animation().updateAnimations(*newRenderer, initialStyle, animatedStyle);
-    if (animatedStyle)
+    if (animatedStyle) {
         newRenderer->setStyleInternal(WTFMove(*animatedStyle));
+        newRenderer->setHasInitialAnimatedStyle(true);
+    }
 
     newRenderer->initializeStyle();
 
index 8612192..6150385 100644 (file)
@@ -258,6 +258,14 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
 
     if (animatedStyle) {
         auto change = determineChange(renderer->style(), *animatedStyle);
+        if (renderer->hasInitialAnimatedStyle()) {
+            renderer->setHasInitialAnimatedStyle(false);
+            // When we initialize a newly created renderer with initial animated style we don't inherit it to descendants.
+            // The first animation frame needs to correct this.
+            // FIXME: We should compute animated style correctly during initial style resolution when we don't have renderers yet.
+            //        https://bugs.webkit.org/show_bug.cgi?id=171926
+            change = std::max(change, Inherit);
+        }
         // 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.
         auto style = change == Detach ? WTFMove(newStyle) : WTFMove(animatedStyle);