Changeset 287881 in webkit

Timestamp:

Jan 11, 2022, 8:20:06 AM (5 years ago)

Author:

graouts@webkit.org

Message:

css/css-transitions/KeyframeEffect-setKeyframes.tentative.html is a failure
​https://bugs.webkit.org/show_bug.cgi?id=235062

Reviewed by Dean Jackson.

LayoutTests/imported/w3c:

Mark WPT progressions.

• web-platform-tests/css/css-transitions/KeyframeEffect-setKeyframes.tentative-expected.txt:

Source/WebCore:

The WPT at css/css-transitions/KeyframeEffect-setKeyframes.tentative.html, which checks
the behavior of programmatically changing keyframes for a CSS Transition, highlighted
three failures:

1. we did not flush pending style changes as setKeyframes() was called for an animation created fromm CSS (CSS Transition or CSS Animation),
2. we did not use the style from the last style update to computed the before-change style if available, but rather the computed style generated _after_ the last style update,
3. we did not apply _all_ animations as we compute the before-change style to consider new transitions to run, but only animations matching the current property.

We've corrected all of those errors and this test now passes save for one failure, which
seems to be a different type of problem and will be looked at in a future patch.

• animation/KeyframeEffect.cpp:

(WebCore::KeyframeEffect::setBindingsKeyframes):

• style/Styleable.cpp:

(WebCore::updateCSSTransitionsForStyleableAndProperty):

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/KeyframeEffect-setKeyframes.tentative-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffect.cpp (modified) (1 diff)
• Source/WebCore/style/Styleable.cpp (modified) (1 diff)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2022-01-11  Antoine Quint  <graouts@webkit.org>
+
+        css/css-transitions/KeyframeEffect-setKeyframes.tentative.html is a failure
+        https://bugs.webkit.org/show_bug.cgi?id=235062
+
+        Reviewed by Dean Jackson.
+
+        Mark WPT progressions.
+
+        * web-platform-tests/css/css-transitions/KeyframeEffect-setKeyframes.tentative-expected.txt:
+
 2022-01-11  Tim Nguyen  <ntim@apple.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/KeyframeEffect-setKeyframes.tentative-expected.txt

@@ -4 +4 @@
 PASS A transition with no keyframes still returns the original transitionProperty
 PASS A transition with replaced keyframes animating the same property still exhibits normal reversing behavior.
-FAIL A transition with replaced keyframes animating a different property still exhibits normal reversing behavior  (reversing from the base value). undefined is not an object (evaluating 'reversedTransition.effect')
-FAIL A transition with replaced keyframes animating nothing still exhibits normal reversing behavior (reversing from the base value). undefined is not an object (evaluating 'reversedTransition.effect')
+PASS A transition with replaced keyframes animating a different property still exhibits normal reversing behavior  (reversing from the base value).
+PASS A transition with replaced keyframes animating nothing still exhibits normal reversing behavior (reversing from the base value).
 FAIL A transition with replaced keyframes animating nothing on a property being controlled by another modified transition exhibits normal reversing behavior and reverses from the other transition's current value. assert_equals: The reversed transition gets its start value from the other transition controlling left expected "280px" but got "200px"
-FAIL A transition with replaced kefyrames and composite 'add' exhibits normal reversing behavior, and the effect is not double counted when calculating the before change style assert_equals: The start value matches the 'before change' value expected "175px" but got "150px"
+PASS A transition with replaced kefyrames and composite 'add' exhibits normal reversing behavior, and the effect is not double counted when calculating the before change style
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-01-11  Antoine Quint  <graouts@webkit.org>
+
+        css/css-transitions/KeyframeEffect-setKeyframes.tentative.html is a failure
+        https://bugs.webkit.org/show_bug.cgi?id=235062
+
+        Reviewed by Dean Jackson.
+
+        The WPT at css/css-transitions/KeyframeEffect-setKeyframes.tentative.html, which checks
+        the behavior of programmatically changing keyframes for a CSS Transition, highlighted
+        three failures:
+
+        1. we did not flush pending style changes as setKeyframes() was called for an animation
+           created fromm CSS (CSS Transition or CSS Animation),
+        2. we did not use the style from the last style update to computed the before-change style
+           if available, but rather the computed style generated _after_ the last style update,
+        3. we did not apply _all_ animations as we compute the before-change style to consider
+           new transitions to run, but only animations matching the current property.
+
+        We've corrected all of those errors and this test now passes save for one failure, which
+        seems to be a different type of problem and will be looked at in a future patch.
+
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::setBindingsKeyframes):
+        * style/Styleable.cpp:
+        (WebCore::updateCSSTransitionsForStyleableAndProperty):
+
 2022-01-11  Andres Gonzalez  <andresg_22@apple.com>
 

trunk/Source/WebCore/animation/KeyframeEffect.cpp

@@ -820 +820 @@
 ExceptionOr<void> KeyframeEffect::setBindingsKeyframes(JSGlobalObject& lexicalGlobalObject, Strong<JSObject>&& keyframesInput)
 {
+    if (is<DeclarativeAnimation>(animation()))
+        downcast<DeclarativeAnimation>(*animation()).flushPendingStyleChanges();
     auto retVal = setKeyframes(lexicalGlobalObject, WTFMove(keyframesInput));
     if (!retVal.hasException() && is<CSSAnimation>(animation()))

trunk/Source/WebCore/style/Styleable.cpp

@@ -411 +411 @@
     // any styles derived from declarative animations such as CSS Transitions, CSS Animations, and SMIL Animations updated to the current time.
     auto beforeChangeStyle = [&]() -> const RenderStyle {
-        if (animation && animation->isRelevant()) {
-            auto animatedStyle = RenderStyle::clone(currentStyle);
-            // If a transition has not yet started or started when animations were last updated, use the timeline time at its creation
-            // as its start time to ensure that it will produce a style with progress > 0.
-            bool shouldUseTimelineTimeAtCreation = is<CSSTransition>(animation) && (!animation->startTime() || *animation->startTime() == styleable.element.document().timeline().currentTime());
-            animation->resolve(animatedStyle, { nullptr }, shouldUseTimelineTimeAtCreation ? downcast<CSSTransition>(*animation).timelineTimeAtCreation() : std::nullopt);
-            return animatedStyle;
-        }
-
-        // If it exists, use the recorded RenderStyle for this element during a previous call to Style::TreeResolver::createAnimatedElementUpdate().
-        if (auto* lastStyleChangeEventStyle = styleable.lastStyleChangeEventStyle())
-            return RenderStyle::clone(*lastStyleChangeEventStyle);
-
-        // If we haven't computed styles from animations for this element, the before-change style is the previously resolved style for this element.
+        if (auto* lastStyleChangeEventStyle = styleable.lastStyleChangeEventStyle()) {
+            auto style = RenderStyle::clone(*lastStyleChangeEventStyle);
+            if (auto* keyframeEffectStack = styleable.keyframeEffectStack()) {
+                for (const auto& effect : keyframeEffectStack->sortedEffects()) {
+                    auto* effectAnimation = effect->animation();
+                    bool shouldUseTimelineTimeAtCreation = is<CSSTransition>(effectAnimation) && (!effectAnimation->startTime() || *effectAnimation->startTime() == styleable.element.document().timeline().currentTime());
+                    effectAnimation->resolve(style, { nullptr }, shouldUseTimelineTimeAtCreation ? downcast<CSSTransition>(*effectAnimation).timelineTimeAtCreation() : std::nullopt);
+                }
+            }
+            return style;
+        }
         return RenderStyle::clone(currentStyle);
     }();