[Web Animations] Tests using the new animation engine may crash under WebCore::FrameV...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 May 2018 18:19:30 +0000 (18:19 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 May 2018 18:19:30 +0000 (18:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=185612
<rdar://problem/39579344>

Reviewed by Dean Jackson.

Source/WebCore:

Add a new internals.pseudoElement() method to obtain a pseudo element matching a given pseudo-id. This is necessary to be able to move off
internals.pauseTransitionAtTimeOnPseudoElement() and internals.pauseAnimationAtTimeOnPseudoElement() for Web Animations testing.

* testing/Internals.cpp:
(WebCore::Internals::pseudoElement):
* testing/Internals.h:
* testing/Internals.idl:

LayoutTests:

Some tests that were opting into the new animation engine were using internals methods (pauseAnimationAtTimeOnElement, pauseTransitionAtTimeOnElement, etc.)
that enforce the creation of animations in the old animation engine. Meanwhile, the code that toggles the animation engine used based on HTML comments is run
prior to teardown of the previous test and so a test running with the new engine would run with the legacy engine during teardown. These two factors would
cause `ASSERT(!frame().animation().hasAnimations())` to fail under FrameView::didDestroyRenderTree().

We update tests that use these internals method to use the Web Animations API instead and opt into the new animation engine if they didn't already do that.

* animations/animation-hit-test-transform.html:
* animations/keyframes-dynamic-expected.txt:
* animations/keyframes-dynamic.html:
* animations/missing-from-to-expected.txt:
* animations/missing-from-to-transforms-expected.txt:
* animations/missing-from-to-transforms.html:
* animations/missing-from-to.html:
* fast/css-generated-content/pseudo-animation.html:
* transitions/transition-hit-test-transform.html:

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/animations/animation-hit-test-transform.html
LayoutTests/animations/keyframes-dynamic-expected.txt
LayoutTests/animations/keyframes-dynamic.html
LayoutTests/animations/missing-from-to-expected.txt
LayoutTests/animations/missing-from-to-transforms-expected.txt
LayoutTests/animations/missing-from-to-transforms.html
LayoutTests/animations/missing-from-to.html
LayoutTests/fast/css-generated-content/pseudo-animation.html
LayoutTests/transitions/transition-hit-test-transform.html
Source/WebCore/ChangeLog
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl

index 9f0eec1..f70692b 100644 (file)
@@ -1,5 +1,30 @@
 2018-05-14  Antoine Quint  <graouts@apple.com>
 
+        [Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
+        https://bugs.webkit.org/show_bug.cgi?id=185612
+        <rdar://problem/39579344>
+
+        Reviewed by Dean Jackson.
+
+        Some tests that were opting into the new animation engine were using internals methods (pauseAnimationAtTimeOnElement, pauseTransitionAtTimeOnElement, etc.)
+        that enforce the creation of animations in the old animation engine. Meanwhile, the code that toggles the animation engine used based on HTML comments is run
+        prior to teardown of the previous test and so a test running with the new engine would run with the legacy engine during teardown. These two factors would
+        cause `ASSERT(!frame().animation().hasAnimations())` to fail under FrameView::didDestroyRenderTree().
+
+        We update tests that use these internals method to use the Web Animations API instead and opt into the new animation engine if they didn't already do that.
+
+        * animations/animation-hit-test-transform.html:
+        * animations/keyframes-dynamic-expected.txt:
+        * animations/keyframes-dynamic.html:
+        * animations/missing-from-to-expected.txt:
+        * animations/missing-from-to-transforms-expected.txt:
+        * animations/missing-from-to-transforms.html:
+        * animations/missing-from-to.html:
+        * fast/css-generated-content/pseudo-animation.html:
+        * transitions/transition-hit-test-transform.html:
+
+2018-05-14  Antoine Quint  <graouts@apple.com>
+
         REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
         https://bugs.webkit.org/show_bug.cgi?id=185299
         <rdar://problem/39630230>
index 49f50e7..7807bc7 100644 (file)
@@ -1,5 +1,4 @@
-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
-   "http://www.w3.org/TR/html4/loose.dtd">
+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <html lang="en">
 <head>
      
         function doTest()
         {
-            if (window.testRunner) {
-                var target = document.getElementById("target");
-                if (!internals.pauseAnimationAtTimeOnElement("anim", 2.0, target)) {
-                    document.getElementById('result').innerHTML = "FAIL: Failed to pause animation"
-                    testRunner.notifyDone();
-                    return;
-                }
-        
-                checkResults();
+            var target = document.getElementById("target");
+            if (!pauseAnimationAtTimeOnElement("anim", 2.0, target)) {
+                document.getElementById('result').innerHTML = "FAIL: Failed to pause animation"
                 testRunner.notifyDone();
+                return;
             }
-            else {
-                window.setTimeout("checkResults()", 2000);
-            }
+    
+            checkResults();
+            testRunner.notifyDone();
         }
     
         function startTest()
index d209f18..cf6e10c 100644 (file)
@@ -1,7 +1,8 @@
 This test performs an animation of the left property. It should stop for 100ms at 100px and 200px We test for those values 50ms after it has stopped at each position. The animations for the three boxes are inserted by JavaScript. The first box's keyframes remain in the stylesheet. The second box's keyframes are removed after its animation starts (but it should animate). The third box's keyframes are removed before its animation starts, and it should not animate.
-PASS - box3 animation was not running
 PASS - "left" property for "box1" element at 0.3s saw something close to: 100
 PASS - "left" property for "box1" element at 0.7s saw something close to: 200
 PASS - "left" property for "box2" element at 0.3s saw something close to: 100
 PASS - "left" property for "box2" element at 0.7s saw something close to: 200
+PASS - "left" property for "box3" element at 0.3s saw something close to: 0
+PASS - "left" property for "box3" element at 0.7s saw something close to: 0
 
index 9d78f3d..cdd40c1 100644 (file)
@@ -1,5 +1,4 @@
-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
-   "http://www.w3.org/TR/html4/loose.dtd">
+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <html lang="en">
 <head>
@@ -28,6 +27,8 @@
       ["anim1", 0.7, "box1", "left", 200, 1],
       ["anim2", 0.3, "box2", "left", 100, 1],
       ["anim2", 0.7, "box2", "left", 200, 1],
+      ["anim3", 0.3, "box3", "left", 0, 0],
+      ["anim3", 0.7, "box3", "left", 0, 0],
     ];
 
     function addKeyframes() {
         style.sheet.removeRule(box3Index);
 
         runAnimationTest(expectedValues);
-
-        if (window.testRunner) {
-            if (internals.pauseAnimationAtTimeOnElement("anim3", 0.1, box3))
-                result += "FAIL - box3 animation was running<br>";
-            else
-                result += "PASS - box3 animation was not running<br>";
-        }
     }
     
     window.addEventListener('DOMContentLoaded', addKeyframes, false);
index 4ddf27d..5b3ff41 100644 (file)
@@ -1,5 +1,4 @@
 This test performs animations of the left property on five boxes over 2 seconds. Box 1 has all keyframes. Box 2 has a missing "from" keyframe. Box 3 has a missing "to" keyframe. Box 4 has both "from" and "to" keyframes missing, but other keyframes which should trigger the generation of "from" and "to". Box 5 has no keyframes, and should not animate. The test takes 3 snapshots each and expects each result to be within a specified range.
-PASS - box5 animation was not running
 PASS - "left" property for "box1" element at 0.4s saw something close to: 20
 PASS - "left" property for "box1" element at 1s saw something close to: 20
 PASS - "left" property for "box1" element at 1.6s saw something close to: 15
@@ -12,4 +11,7 @@ PASS - "left" property for "box3" element at 1.6s saw something close to: 15
 PASS - "left" property for "box4" element at 0.4s saw something close to: 20
 PASS - "left" property for "box4" element at 1s saw something close to: 25
 PASS - "left" property for "box4" element at 1.6s saw something close to: 15
+PASS - "left" property for "box5" element at 0.4s saw something close to: 10
+PASS - "left" property for "box5" element at 1s saw something close to: 10
+PASS - "left" property for "box5" element at 1.6s saw something close to: 10
 
index a40bc1d..e5da3b2 100644 (file)
@@ -1,5 +1,4 @@
 This test performs animations of the transform property on five boxes over 2 seconds. Box 1 has all keyframes. Box 2 has a missing "from" keyframe. Box 3 has a missing "to" keyframe. Box 4 has both "from" and "to" keyframes missing, but other keyframes which should trigger the generation of "from" and "to". Box 5 has no keyframes, and should not animate. The test takes 3 snapshots each and expects each result to be within a specified range.
-PASS - box5 animation was not running
 PASS - "webkitTransform.4" property for "box1" element at 0.4s saw something close to: 20
 PASS - "webkitTransform.4" property for "box1" element at 1s saw something close to: 20
 PASS - "webkitTransform.4" property for "box1" element at 1.6s saw something close to: 15
@@ -12,4 +11,7 @@ PASS - "webkitTransform.4" property for "box3" element at 1.6s saw something clo
 PASS - "webkitTransform.4" property for "box4" element at 0.4s saw something close to: 20
 PASS - "webkitTransform.4" property for "box4" element at 1s saw something close to: 25
 PASS - "webkitTransform.4" property for "box4" element at 1.6s saw something close to: 15
+PASS - "webkitTransform.4" property for "box5" element at 0.4s saw something close to: 10
+PASS - "webkitTransform.4" property for "box5" element at 1s saw something close to: 10
+PASS - "webkitTransform.4" property for "box5" element at 1.6s saw something close to: 10
 
index debeaa3..e86fef6 100644 (file)
@@ -71,7 +71,7 @@
   <script src="resources/animation-test-helpers.js" type="text/javascript" charset="utf-8"></script>
   <script type="text/javascript" charset="utf-8">
 
-    const expectedValues = [
+    runAnimationTest([
       // [animation-name, time, element-id, property, expected-value, tolerance]
       ["anim1", 0.4, "box1", "webkitTransform.4", 20, 2],
       ["anim1", 1.0, "box1", "webkitTransform.4", 20, 2],
       ["anim3", 1.6, "box3", "webkitTransform.4", 15, 2],
       ["anim4", 0.4, "box4", "webkitTransform.4", 20, 2],
       ["anim4", 1.0, "box4", "webkitTransform.4", 25, 2],
-      ["anim4", 1.6, "box4", "webkitTransform.4", 15, 2]
-    ];
-
-    runAnimationTest(expectedValues, function() {
-      if (window.testRunner) {
-          var box5Element = document.getElementById('box5');
-          if (internals.pauseAnimationAtTimeOnElement("anim5", 0.1, box5Element))
-              result += "FAIL - box5 animation was running<br>";
-          else 
-              result += "PASS - box5 animation was not running<br>";
-      }
-    });
+      ["anim4", 1.6, "box4", "webkitTransform.4", 15, 2],
+      ["anim5", 0.4, "box5", "webkitTransform.4", 10, 0],
+      ["anim5", 1.0, "box5", "webkitTransform.4", 10, 0],
+      ["anim5", 1.6, "box5", "webkitTransform.4", 10, 0]
+    ]);
     
   </script>
 </head>
index 851707f..28c1f6c 100644 (file)
@@ -1,5 +1,4 @@
-<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
-   "http://www.w3.org/TR/html4/loose.dtd">
+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <html lang="en">
 <head>
@@ -71,7 +70,7 @@
   <script src="resources/animation-test-helpers.js" type="text/javascript" charset="utf-8"></script>
   <script type="text/javascript" charset="utf-8">
 
-    const expectedValues = [
+    runAnimationTest([
       // [animation-name, time, element-id, property, expected-value, tolerance]
       ["anim1", 0.4, "box1", "left", 20, 2],
       ["anim1", 1.0, "box1", "left", 20, 2],
       ["anim3", 1.6, "box3", "left", 15, 2],
       ["anim4", 0.4, "box4", "left", 20, 2],
       ["anim4", 1.0, "box4", "left", 25, 2],
-      ["anim4", 1.6, "box4", "left", 15, 2]
-    ];
-
-    runAnimationTest(expectedValues, function() {
-      if (window.testRunner) {
-          var box5Element = document.getElementById('box5');
-          if (internals.pauseAnimationAtTimeOnElement("anim5", 0.1, box5Element))
-              result += "FAIL - box5 animation was running<br>";
-          else 
-              result += "PASS - box5 animation was not running<br>";
-      }
-    });
+      ["anim4", 1.6, "box4", "left", 15, 2],
+      ["anim5", 0.4, "box5", "left", 10, 0],
+      ["anim5", 1.0, "box5", "left", 10, 0],
+      ["anim5", 1.6, "box5", "left", 10, 0]
+    ]);
     
   </script>
 </head>
index 3a0c6c4..b05132c 100644 (file)
@@ -1,4 +1,4 @@
-<!DOCTYPE html>
+<!DOCTYPE html><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <script src="../../resources/js-test-pre.js"></script>
 
@@ -78,6 +78,25 @@ function getPseudoComputedTop(id)
 // FIXME: This test should be modified so subpixel doesn't cause off by one
 // below and it no longer needs shouldBeCloseTo.
 
+function pauseAnimationAtTimeOnPseudoElement(animationName, time, element, pseudoId)
+{
+    const pseudoElement = internals.pseudoElement(element, pseudoId);
+    if (!pseudoElement) {
+        console.log("Failed to find pseudo element");
+        return;
+    }
+
+    const animations = pseudoElement.getAnimations();
+    for (let animation of animations) {
+        if (animation instanceof CSSAnimation && animation.animationName == animationName && animation.effect.getKeyframes().length) {
+            animation.currentTime = time * 1000;
+            animation.pause();
+            return true;
+        }
+    }
+    return false;
+}
+
 function testAnimation(id)
 {
     var div = document.getElementById(id);
@@ -85,11 +104,11 @@ function testAnimation(id)
     window.div = div;
     shouldBe('div.offsetWidth', '52');
     if (window.internals) {
-        internals.pauseAnimationAtTimeOnPseudoElement('example', 1.0, div, id);
+        pauseAnimationAtTimeOnPseudoElement('example', 1.0, div, id);
         shouldBeCloseTo('div.offsetWidth', 20, 1);
         computedTop = getPseudoComputedTop(id);
         shouldBeCloseTo('computedTop', 170, 1);
-        internals.pauseAnimationAtTimeOnPseudoElement('example', 2.0, div, id);
+        pauseAnimationAtTimeOnPseudoElement('example', 2.0, div, id);
         shouldBeCloseTo('div.offsetWidth', 12, 1);
         computedTop = getPseudoComputedTop(id);
         shouldBeCloseTo('computedTop', 200, 1);
index 1c0a035..8874816 100644 (file)
@@ -1,4 +1,4 @@
-<!DOCTYPE html>
+<!DOCTYPE html><!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
 
 <html>
 <head>
@@ -49,7 +49,7 @@
         {
             if (window.testRunner) {
                 var target = document.getElementById('target');
-                if (!internals.pauseTransitionAtTimeOnElement("-webkit-transform", 2.0, target))
+                if (!pauseTransitionAtTimeOnElement("-webkit-transform", 2.0, target))
                     throw("Transition is not running");
         
                 checkResults();
index 98e520b..01d58a3 100644 (file)
@@ -1,5 +1,21 @@
 2018-05-14  Antoine Quint  <graouts@apple.com>
 
+        [Web Animations] Tests using the new animation engine may crash under WebCore::FrameView::didDestroyRenderTree when using internals methods
+        https://bugs.webkit.org/show_bug.cgi?id=185612
+        <rdar://problem/39579344>
+
+        Reviewed by Dean Jackson.
+
+        Add a new internals.pseudoElement() method to obtain a pseudo element matching a given pseudo-id. This is necessary to be able to move off
+        internals.pauseTransitionAtTimeOnPseudoElement() and internals.pauseAnimationAtTimeOnPseudoElement() for Web Animations testing.
+
+        * testing/Internals.cpp:
+        (WebCore::Internals::pseudoElement):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
+2018-05-14  Antoine Quint  <graouts@apple.com>
+
         REGRESSION (r230574): Interrupted hardware transitions don't behave correctly
         https://bugs.webkit.org/show_bug.cgi?id=185299
         <rdar://problem/39630230>
index 81a4009..09aa0a9 100644 (file)
@@ -1048,6 +1048,14 @@ ExceptionOr<bool> Internals::pauseTransitionAtTimeOnPseudoElement(const String&
     return frame()->animation().pauseTransitionAtTime(*pseudoElement, property, pauseTime);
 }
 
+ExceptionOr<RefPtr<Element>> Internals::pseudoElement(Element& element, const String& pseudoId)
+{
+    if (pseudoId != "before" && pseudoId != "after")
+        return Exception { InvalidAccessError };
+
+    return pseudoId == "before" ? element.beforePseudoElement() : element.afterPseudoElement();
+}
+
 ExceptionOr<String> Internals::elementRenderTreeAsText(Element& element)
 {
     element.document().updateStyleIfNeeded();
index 1b83bcb..0823637 100644 (file)
@@ -195,6 +195,9 @@ public:
     ExceptionOr<bool> pauseTransitionAtTimeOnElement(const String& propertyName, double pauseTime, Element&);
     ExceptionOr<bool> pauseTransitionAtTimeOnPseudoElement(const String& property, double pauseTime, Element&, const String& pseudoId);
 
+    // For animations testing, we need a way to get at pseudo elements.
+    ExceptionOr<RefPtr<Element>> pseudoElement(Element&, const String&);
+
     Node* treeScopeRootNode(Node&);
     Node* parentTreeScope(Node&);
 
index 8d9c267..cb508ca 100644 (file)
@@ -148,6 +148,9 @@ enum EventThrottlingBehavior {
     [MayThrowException] boolean pauseTransitionAtTimeOnElement(DOMString propertyName, unrestricted double pauseTime, Element element);
     [MayThrowException] boolean pauseTransitionAtTimeOnPseudoElement(DOMString property, unrestricted double pauseTime, Element element, DOMString pseudoId);
 
+    // For animations testing, we need a way to get at pseudo elements.
+    [MayThrowException] Element? pseudoElement(Element element, DOMString pseudoId);
+
     DOMString visiblePlaceholder(Element element);
     void selectColorInColorChooser(HTMLInputElement element, DOMString colorValue);
     [MayThrowException] sequence<DOMString> formControlStateOfPreviousHistoryItem();