REGRESSION (r252724): Unable to tap on play button on google video 'See the top searc...
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Jan 2020 16:44:32 +0000 (16:44 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Jan 2020 16:44:32 +0000 (16:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=205694
<rdar://problem/58062987>

Reviewed by Zalan Bujtas.

Source/WebCore:

After r252724, which separated 'used' from 'specified' z-index in style, we need to copy
the specified to the used z-index in animated styles, while preserving the existing 'forceStackingContext'
behavior which set the used z-index to 0.

Do so by creating Adjuster::adjustAnimatedStyle(), which is called from TreeResolver::createAnimatedElementUpdate()
if any animations could have affected the style. We need to pass back information about whether the animation should
force stacking context.

Test: animations/z-index-in-keyframe.html

* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::apply):
* animation/KeyframeEffect.h:
(WebCore::KeyframeEffect::triggersStackingContext const):
* dom/Element.cpp:
(WebCore::Element::applyKeyframeEffects):
* dom/Element.h:
* page/animation/CSSAnimationController.h:
(): Deleted.
* page/animation/CompositeAnimation.cpp:
(WebCore::CompositeAnimation::animate):
* style/StyleAdjuster.cpp:
(WebCore::Style::Adjuster::adjustAnimatedStyle):
* style/StyleAdjuster.h:
* style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::createAnimatedElementUpdate):

LayoutTests:

* animations/z-index-in-keyframe-expected.html: Added.
* animations/z-index-in-keyframe.html: Added.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/animations/z-index-in-keyframe-expected.html [new file with mode: 0644]
LayoutTests/animations/z-index-in-keyframe.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/KeyframeEffect.cpp
Source/WebCore/animation/KeyframeEffect.h
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/page/animation/AnimationBase.h
Source/WebCore/page/animation/CSSAnimationController.h
Source/WebCore/page/animation/CompositeAnimation.cpp
Source/WebCore/style/StyleAdjuster.cpp
Source/WebCore/style/StyleAdjuster.h
Source/WebCore/style/StyleTreeResolver.cpp

index 86fba26..b4f1db9 100644 (file)
@@ -1,3 +1,14 @@
+2020-01-05  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r252724): Unable to tap on play button on google video 'See the top search trends of 2019'
+        https://bugs.webkit.org/show_bug.cgi?id=205694
+        <rdar://problem/58062987>
+
+        Reviewed by Zalan Bujtas.
+
+        * animations/z-index-in-keyframe-expected.html: Added.
+        * animations/z-index-in-keyframe.html: Added.
+
 2020-01-06  Chris Dumez  <cdumez@apple.com>
 
         Regression r254029: imported/w3c/web-platform-tests/html/dom/idlharness.https.html is failing
diff --git a/LayoutTests/animations/z-index-in-keyframe-expected.html b/LayoutTests/animations/z-index-in-keyframe-expected.html
new file mode 100644 (file)
index 0000000..5330a45
--- /dev/null
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        .container {
+            position: absolute;
+            width: 300px;
+            height: 300px;
+            border: 1px solid black;
+        }
+        
+        .box {
+            position: absolute;
+            width: 200px;
+            height: 200px;
+        }
+        
+        .bottom {
+            left: 0px;
+            top: 0px;
+            background-color: orange;
+        }
+
+        .top {
+            left: 100px;
+            top: 100px;
+            background-color: green;
+            z-index: -1;
+            opacity: 0.8;
+        }
+    </style>
+</head>
+<body>
+    <div class="container">
+        <div class="bottom box"></div>
+        <div class="top box"></div>
+    </div>
+</body>
+</html>
diff --git a/LayoutTests/animations/z-index-in-keyframe.html b/LayoutTests/animations/z-index-in-keyframe.html
new file mode 100644 (file)
index 0000000..337e77c
--- /dev/null
@@ -0,0 +1,59 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        .container {
+            position: absolute;
+            width: 300px;
+            height: 300px;
+            border: 1px solid black;
+        }
+        
+        .box {
+            position: absolute;
+            width: 200px;
+            height: 200px;
+        }
+        
+        .bottom {
+            left: 0px;
+            top: 0px;
+            background-color: orange;
+        }
+
+        .top {
+            left: 100px;
+            top: 100px;
+            background-color: green;
+            animation: fade-out 30ms forwards;
+        }
+        
+        @keyframes fade-out {
+            0% {
+                opacity: 1;
+            }
+            100% {
+                z-index: -1;
+                opacity: 0.8;
+            }
+        }
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        window.addEventListener('load', () => {
+            document.getElementById('tester').addEventListener('animationend', () => {
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            }, false);
+        }, false);
+    </script>
+</head>
+<body>
+    <div class="container">
+        <div class="bottom box"></div>
+        <div id="tester" class="top box"></div>
+    </div>
+</body>
+</html>
index 1f8ab10..3ca5f5a 100644 (file)
@@ -1,3 +1,38 @@
+2020-01-05  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r252724): Unable to tap on play button on google video 'See the top search trends of 2019'
+        https://bugs.webkit.org/show_bug.cgi?id=205694
+        <rdar://problem/58062987>
+
+        Reviewed by Zalan Bujtas.
+
+        After r252724, which separated 'used' from 'specified' z-index in style, we need to copy
+        the specified to the used z-index in animated styles, while preserving the existing 'forceStackingContext'
+        behavior which set the used z-index to 0.
+
+        Do so by creating Adjuster::adjustAnimatedStyle(), which is called from TreeResolver::createAnimatedElementUpdate()
+        if any animations could have affected the style. We need to pass back information about whether the animation should
+        force stacking context.
+
+        Test: animations/z-index-in-keyframe.html
+
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::apply):
+        * animation/KeyframeEffect.h:
+        (WebCore::KeyframeEffect::triggersStackingContext const):
+        * dom/Element.cpp:
+        (WebCore::Element::applyKeyframeEffects):
+        * dom/Element.h:
+        * page/animation/CSSAnimationController.h:
+        (): Deleted.
+        * page/animation/CompositeAnimation.cpp:
+        (WebCore::CompositeAnimation::animate):
+        * style/StyleAdjuster.cpp:
+        (WebCore::Style::Adjuster::adjustAnimatedStyle):
+        * style/StyleAdjuster.h:
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
+
 2020-01-06  Per Arne Vollan  <pvollan@apple.com>
 
         [iOS] Issue mach lookup extension to launch services daemon for Mail
index f994307..9583aca 100644 (file)
@@ -50,6 +50,7 @@
 #include "RenderElement.h"
 #include "RenderStyle.h"
 #include "RuntimeEnabledFeatures.h"
+#include "StyleAdjuster.h"
 #include "StylePendingResources.h"
 #include "StyleResolver.h"
 #include "TimingFunction.h"
@@ -1090,12 +1091,6 @@ void KeyframeEffect::apply(RenderStyle& targetStyle)
         return;
 
     setAnimatedPropertiesInStyle(targetStyle, computedTiming.progress.value());
-
-    // https://w3c.github.io/web-animations/#side-effects-section
-    // For every property targeted by at least one animation effect that is current or in effect, the user agent
-    // must act as if the will-change property ([css-will-change-1]) on the target element includes the property.
-    if (m_triggersStackingContext && targetStyle.hasAutoUsedZIndex())
-        targetStyle.setUsedZIndex(0);
 }
 
 void KeyframeEffect::invalidate()
index 049337d..8560981 100644 (file)
@@ -126,6 +126,7 @@ public:
     RenderElement* renderer() const override;
     const RenderStyle& currentStyle() const override;
     bool isAccelerated() const { return m_shouldRunAccelerated; }
+    bool triggersStackingContext() const { return m_triggersStackingContext; }
     bool filterFunctionListsMatch() const override { return m_filterFunctionListsMatch; }
     bool transformFunctionListsMatch() const override { return m_transformFunctionListsMatch; }
 #if ENABLE(FILTERS_LEVEL_2)
index 537ed2b..79aa2b5 100644 (file)
@@ -3724,19 +3724,22 @@ bool Element::hasKeyframeEffects() const
     return keyframeEffectStack && keyframeEffectStack->hasEffects();
 }
 
-bool Element::applyKeyframeEffects(RenderStyle& targetStyle)
+OptionSet<AnimationImpact> Element::applyKeyframeEffects(RenderStyle& targetStyle)
 {
-    bool hasNonAcceleratedAnimationProperty = false;
+    OptionSet<AnimationImpact> impact;
 
     for (const auto& effect : ensureKeyframeEffectStack().sortedEffects()) {
         ASSERT(effect->animation());
         effect->animation()->resolve(targetStyle);
 
-        if (!hasNonAcceleratedAnimationProperty && !effect->isAccelerated())
-            hasNonAcceleratedAnimationProperty = true;
+        if (effect->isAccelerated())
+            impact.add(AnimationImpact::RequiresRecomposite);
+
+        if (effect->triggersStackingContext())
+            impact.add(AnimationImpact::ForcesStackingContext);
     }
 
-    return !hasNonAcceleratedAnimationProperty;
+    return impact;
 }
 
 #if ENABLE(RESIZE_OBSERVER)
index 844ae22..a85d049 100644 (file)
@@ -73,6 +73,8 @@ namespace Style {
 struct ElementStyle;
 }
 
+enum class AnimationImpact;
+
 class Element : public ContainerNode {
     WTF_MAKE_ISO_ALLOCATED(Element);
 public:
@@ -480,7 +482,7 @@ public:
 
     KeyframeEffectStack& ensureKeyframeEffectStack();
     bool hasKeyframeEffects() const;
-    bool applyKeyframeEffects(RenderStyle&);
+    OptionSet<AnimationImpact> applyKeyframeEffects(RenderStyle&);
 
 #if ENABLE(FULLSCREEN_API)
     WEBCORE_EXPORT bool containsFullScreenElement() const;
index fe87020..7bc4204 100644 (file)
@@ -49,6 +49,11 @@ enum class AnimateChange {
     RunningStateChange      = 1 << 2, // Animation "running or paused" changed.
 };
 
+enum class AnimationImpact {
+    RequiresRecomposite     = 1 << 0,
+    ForcesStackingContext   = 1 << 1
+};
+
 class AnimationBase : public RefCounted<AnimationBase>
     , public CSSPropertyBlendingClient {
     friend class CompositeAnimation;
index ec7b9c7..ecd6058 100644 (file)
@@ -30,6 +30,7 @@
 
 #include "AnimationBase.h"
 #include "CSSPropertyNames.h"
+#include "Element.h"
 #include "RenderStyle.h"
 #include <wtf/Forward.h>
 
@@ -37,14 +38,14 @@ namespace WebCore {
 
 class CSSAnimationControllerPrivate;
 class Document;
-class Element;
 class Frame;
 class LayoutRect;
 class RenderElement;
 
 struct AnimationUpdate {
     std::unique_ptr<RenderStyle> style;
-    bool animationChangeRequiresRecomposite { false };
+    OptionSet<AnimationImpact> impact;
+
 };
 
 class CSSAnimationController {
index 0449470..ebd4927 100644 (file)
@@ -37,6 +37,7 @@
 #include "Logging.h"
 #include "RenderElement.h"
 #include "RenderStyle.h"
+#include "StyleAdjuster.h"
 #include <wtf/NeverDestroyed.h>
 #include <wtf/text/CString.h>
 
@@ -286,8 +287,7 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
     updateKeyframeAnimations(element, currentStyle, targetStyle);
     m_keyframeAnimations.checkConsistency();
 
-    bool animationChangeRequiresRecomposite = false;
-    bool forceStackingContext = false;
+    OptionSet<AnimationImpact> imapct;
 
     std::unique_ptr<RenderStyle> animatedStyle;
 
@@ -300,7 +300,8 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
             if (changes.contains(AnimateChange::StyleBlended))
                 checkForStackingContext |= WillChangeData::propertyCreatesStackingContext(transition->animatingProperty());
 
-            animationChangeRequiresRecomposite = changes.contains(AnimateChange::RunningStateChange) && transition->affectsAcceleratedProperty();
+            if (changes.contains(AnimateChange::RunningStateChange) && transition->affectsAcceleratedProperty())
+                imapct.add(AnimationImpact::RequiresRecomposite);
         }
 
         if (animatedStyle && checkForStackingContext) {
@@ -316,7 +317,7 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
                 || animatedStyle->hasBackdropFilter()
 #endif
                 )
-            forceStackingContext = true;
+            imapct.add(AnimationImpact::ForcesStackingContext);
         }
     }
 
@@ -326,22 +327,17 @@ AnimationUpdate CompositeAnimation::animate(Element& element, const RenderStyle*
         RefPtr<KeyframeAnimation> keyframeAnim = m_keyframeAnimations.get(name);
         if (keyframeAnim) {
             auto changes = keyframeAnim->animate(*this, targetStyle, animatedStyle);
-            animationChangeRequiresRecomposite = changes.contains(AnimateChange::RunningStateChange) && keyframeAnim->affectsAcceleratedProperty();
-            forceStackingContext |= changes.contains(AnimateChange::StyleBlended) && keyframeAnim->triggersStackingContext();
+            if (changes.contains(AnimateChange::RunningStateChange) && keyframeAnim->affectsAcceleratedProperty())
+                imapct.add(AnimationImpact::RequiresRecomposite);
+
+            if (changes.contains(AnimateChange::StyleBlended) && keyframeAnim->triggersStackingContext())
+                imapct.add(AnimationImpact::ForcesStackingContext);
+
             m_hasAnimationThatDependsOnLayout |= keyframeAnim->dependsOnLayout();
         }
     }
 
-    // https://drafts.csswg.org/css-animations-1/
-    // While an animation is applied but has not finished, or has finished but has an animation-fill-mode of forwards or both,
-    // the user agent must act as if the will-change property ([css-will-change-1]) on the element additionally
-    // includes all the properties animated by the animation.
-    if (forceStackingContext && animatedStyle) {
-        if (animatedStyle->hasAutoUsedZIndex())
-            animatedStyle->setUsedZIndex(0);
-    }
-
-    return { WTFMove(animatedStyle), animationChangeRequiresRecomposite };
+    return { WTFMove(animatedStyle), imapct };
 }
 
 std::unique_ptr<RenderStyle> CompositeAnimation::getAnimatedStyle() const
index 8daca1e..eee48fe 100644 (file)
@@ -30,6 +30,7 @@
 #include "config.h"
 #include "StyleAdjuster.h"
 
+#include "AnimationBase.h"
 #include "CSSFontSelector.h"
 #include "Element.h"
 #include "FrameView.h"
@@ -42,6 +43,7 @@
 #include "MathMLElement.h"
 #include "Page.h"
 #include "Quirks.h"
+#include "RenderBox.h"
 #include "RenderStyle.h"
 #include "RenderTheme.h"
 #include "RuntimeEnabledFeatures.h"
@@ -524,6 +526,22 @@ void Adjuster::adjustSVGElementStyle(RenderStyle& style, const SVGElement& svgEl
         style.setDisplay(DisplayType::Block);
 }
 
+void Adjuster::adjustAnimatedStyle(RenderStyle& style, const RenderStyle* parentBoxStyle, OptionSet<AnimationImpact> impact)
+{
+    // Set an explicit used z-index in two cases:
+    // 1. When the element respects z-index, and the style has an explicit z-index set (for example, the animation
+    //    itself may animate z-index).
+    // 2. When we want the stacking context side-effets of explicit z-index, via forceStackingContext.
+    // It's important to not clobber an existing used z-index, since an earlier animation may have set it, but we
+    // may still need to update the used z-index value from the specified value.
+    bool elementRespectsZIndex = style.position() != PositionType::Static || (parentBoxStyle && parentBoxStyle->isDisplayFlexibleOrGridBox());
+
+    if (elementRespectsZIndex && !style.hasAutoSpecifiedZIndex())
+        style.setUsedZIndex(style.specifiedZIndex());
+    else if (impact.contains(AnimationImpact::ForcesStackingContext))
+        style.setUsedZIndex(0);
+}
+
 void Adjuster::adjustForSiteSpecificQuirks(RenderStyle& style) const
 {
     if (m_document.quirks().needsGMailOverflowScrollQuirk() && m_element) {
index d37e2d6..e3ac23b 100644 (file)
@@ -42,6 +42,8 @@ public:
     void adjust(RenderStyle&, const RenderStyle* userAgentAppearanceStyle) const;
 
     static void adjustSVGElementStyle(RenderStyle&, const SVGElement&);
+    static void adjustAnimatedStyle(RenderStyle&, const RenderStyle* parentBoxStyle, OptionSet<AnimationImpact>);
+
 #if ENABLE(TEXT_AUTOSIZING)
     static bool adjustForTextAutosizing(RenderStyle&, const Element&);
 #endif
index 0902f39..a697655 100644 (file)
@@ -49,6 +49,7 @@
 #include "RuntimeEnabledFeatures.h"
 #include "Settings.h"
 #include "ShadowRoot.h"
+#include "StyleAdjuster.h"
 #include "StyleFontSizeFunctions.h"
 #include "StyleResolver.h"
 #include "StyleScope.h"
@@ -302,8 +303,8 @@ const RenderStyle* TreeResolver::parentBoxStyleForPseudo(const ElementUpdate& el
 ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderStyle> newStyle, Element& element, Change parentChange)
 {
     auto* oldStyle = element.renderOrDisplayContentsStyle();
-
-    bool shouldRecompositeLayer = false;
+    
+    OptionSet<AnimationImpact> animationImpact;
 
     // New code path for CSS Animations and CSS Transitions.
     if (RuntimeEnabledFeatures::sharedFeatures().webAnimationsCSSIntegrationEnabled()) {
@@ -323,7 +324,7 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
     // as animations created via the JS API.
     if (element.hasKeyframeEffects()) {
         auto animatedStyle = RenderStyle::clonePtr(*newStyle);
-        shouldRecompositeLayer = element.applyKeyframeEffects(*animatedStyle);
+        animationImpact = element.applyKeyframeEffects(*animatedStyle);
         newStyle = WTFMove(animatedStyle);
     }
 
@@ -332,20 +333,22 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
         auto& animationController = m_document.frame()->animation();
 
         auto animationUpdate = animationController.updateAnimations(element, *newStyle, oldStyle);
-        shouldRecompositeLayer = animationUpdate.animationChangeRequiresRecomposite;
+        animationImpact.add(animationUpdate.impact);
 
         if (animationUpdate.style)
             newStyle = WTFMove(animationUpdate.style);
     }
 
+    if (animationImpact)
+        Adjuster::adjustAnimatedStyle(*newStyle, parentBoxStyle(), animationImpact);
+
     auto change = oldStyle ? determineChange(*oldStyle, *newStyle) : Detach;
 
     auto validity = element.styleValidity();
     if (validity >= Validity::SubtreeAndRenderersInvalid || parentChange == Detach)
         change = Detach;
 
-    shouldRecompositeLayer |= element.styleResolutionShouldRecompositeLayer();
-
+    bool shouldRecompositeLayer = animationImpact.contains(AnimationImpact::RequiresRecomposite) || element.styleResolutionShouldRecompositeLayer();
     return { WTFMove(newStyle), change, shouldRecompositeLayer };
 }