Bug 1630745 [wpt PR 23032] - Animations modified with setKeyframes must not mask !important, a=testonly
authorAnders Hartvoll Ruud <andruud@chromium.org>
Tue, 28 Apr 2020 11:34:46 +0000
changeset 527540 8e727e285adbc143fe0aae366ef688954921ce74
parent 527539 7c786601421dabc1082e7dae5f3411ef95144b53
child 527541 6a7be38cb8f33b5ac5eb4490b57c7cf657f9b242
push id37368
push userbtara@mozilla.com
push dateFri, 01 May 2020 21:45:51 +0000
treeherdermozilla-central@0f9c5a59e45d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstestonly
bugs1630745, 23032, 1062217, 552085, 2152449, 761034
milestone77.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1630745 [wpt PR 23032] - Animations modified with setKeyframes must not mask !important, a=testonly Automatic update from web-platform-tests Animations modified with setKeyframes must not mask !important CSS declarations that are !important have higher priority in the cascade than animation effects. Unfortunately the information about which declarations were and weren't important is lost once the StyleCascade disappears. Specifically, it's not stored on the ComputedStyle. This causes a problem (once again) for the base computed style optimization, since we can't naively add animation effects on top of the base anymore. We might be overwriting something in the base that was important. The previous attempt at fixing this (flag on ElementAnimations) doesn't work properly. For example, it fails to detect the case where an animation initially doesn't conflict with important declarations, but then suddenly does via setKeyframes. To solve this, this CL stores a bitset containing information about important declarations alongside the base ComputedStyle on ElementAnimations. When we're considering whether the base can be used or not, we then check if there's any animation matching the set of important declarations. Persisting that many bits is slightly uncomfortable, but the only viable alternative I see is disabling the optimization when *any* important declaration exists in the base, which is probably a worse option. Sidenote: Initially I tried to always use the base, even when there were conflicts with important declarations. The bitset of important declarations is effectively a set of animations to be skipped, so we should still be able to use the base if we just don't apply the properties present in the set. However, it unfortunately didn't work due to visited/unvisited colors being animated together (crbug.com/1062217). Bug: 552085 Change-Id: I39e2879af8a858ce1bd97eaa2ceb6e222591df79 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152449 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Cr-Commit-Position: refs/heads/master@{#761034} -- wpt-commits: 9f16de83abdee864d6dafebbacc298b08964750d wpt-pr: 23032
testing/web-platform/tests/css/css-animations/animation-important-001.html
--- a/testing/web-platform/tests/css/css-animations/animation-important-001.html
+++ b/testing/web-platform/tests/css/css-animations/animation-important-001.html
@@ -1,13 +1,27 @@
 <!DOCTYPE html>
 <title>Test !important declarations vs. animation effects</title>
 <link rel="help" href="https://drafts.csswg.org/css-cascade/#cascade-origin">
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
+<script>
+  CSS.registerProperty({
+    name: "--length-1",
+    syntax: "<length>",
+    initialValue: "0px",
+    inherits: false
+  });
+  CSS.registerProperty({
+    name: "--length-2",
+    syntax: "<length>",
+    initialValue: "0px",
+    inherits: false
+  });
+</script>
 <style>
   @keyframes bgcolor_animation {
     from { background-color: rgb(10, 10, 10); }
     to { background-color: rgb(20, 20, 20); }
   }
   @keyframes color_and_bg_animation {
     from { background-color: rgb(10, 10, 10); color: rgb(10, 10, 10); }
     to { background-color: rgb(20, 20, 20); color: rgb(20, 20, 20); }
@@ -34,22 +48,32 @@
     content: " ";
   }
   #target5 {
     animation-name: color_and_bg_animation;
   }
   #target5:visited {
     color: rgb(0, 128, 0) !important;
   }
+  #target6 {
+    background-color: white;
+    color: white !important;
+  }
+  #target7 {
+    --length-1: 10px;
+    --length-2: 10px !important;
+  }
 </style>
 <div id="target1"></div>
 <div id="target2"></div>
 <div id="target3"></div>
 <div id="target4"></div>
 <a href="" id="target5"></a>
+<span id="target6"></span>
+<span id="target7"></span>
 <script>
 test(() => {
   assert_equals(getComputedStyle(target1).backgroundColor, 'rgb(15, 15, 15)');
 }, 'Interpolated value is observable');
 
 test(() => {
   assert_equals(getComputedStyle(target2).backgroundColor, 'rgb(0, 128, 0)');
 }, 'Important rules override animations');
@@ -64,9 +88,57 @@ test(() => {
   assert_equals(getComputedStyle(target4, '::before').backgroundColor, 'rgb(15, 15, 15)');
 }, 'Important rules override animations (::before)');
 
 test(() => {
   assert_equals(getComputedStyle(target5).color, 'rgb(15, 15, 15)');
   assert_equals(getComputedStyle(target5).backgroundColor, 'rgb(15, 15, 15)');
 }, 'Important rules do not override animations on :visited as seen from JS');
 
+test(() => {
+  getComputedStyle(target6).backgroundColor;
+
+  let animation = target6.animate([
+    { backgroundColor: 'rgb(10, 10, 10)' },
+    { backgroundColor: 'rgb(20, 20, 20)' },
+  ], {
+    duration: 1000000,
+    delay: -500000,
+    easing: 'steps(2, end)'
+  });
+
+  assert_equals(getComputedStyle(target6).backgroundColor, 'rgb(15, 15, 15)');
+  assert_equals(getComputedStyle(target6).color, 'rgb(255, 255, 255)');
+
+  animation.effect.setKeyframes([
+    { color: 'rgb(10, 10, 10)' },
+    { color: 'rgb(20, 20, 20)' },
+  ]);
+
+  assert_equals(getComputedStyle(target6).backgroundColor, 'rgb(255, 255, 255)');
+  assert_equals(getComputedStyle(target6).color, 'rgb(255, 255, 255)');
+}, 'Standard property animations appearing via setKeyframes do not override important declarations');
+
+test(() => {
+  getComputedStyle(target7).getPropertyValue('--length-1');
+
+  let animation = target7.animate([
+    { '--length-1': '100px' },
+    { '--length-1': '200px' },
+  ], {
+    duration: 1000000,
+    delay: -500000,
+    easing: 'steps(2, end)'
+  });
+
+  assert_equals(getComputedStyle(target7).getPropertyValue('--length-1'), '150px');
+  assert_equals(getComputedStyle(target7).getPropertyValue('--length-2'), '10px');
+
+  animation.effect.setKeyframes([
+    { '--length-2': '100px' },
+    { '--length-2': '200px' },
+  ]);
+
+  assert_equals(getComputedStyle(target7).getPropertyValue('--length-1'), '10px');
+  assert_equals(getComputedStyle(target7).getPropertyValue('--length-2'), '10px');
+}, 'Custom property animations appearing via setKeyframes do not override important declarations');
+
 </script>