mozilla-central: changeset 527540:8e727e285adbc143fe0aae366ef688954921ce74

author	Anders Hartvoll Ruud <andruud@chromium.org>
	Tue, 28 Apr 2020 11:34:46 +0000
changeset 527540	8e727e285adbc143fe0aae366ef688954921ce74
parent 527539	7c786601421dabc1082e7dae5f3411ef95144b53
child 527541	6a7be38cb8f33b5ac5eb4490b57c7cf657f9b242
push id	37368
push user	btara@mozilla.com
push date	Fri, 01 May 2020 21:45:51 +0000
treeherder	mozilla-central@0f9c5a59e45d [default view] [failures only]
perfherder	[talos] [build metrics] [platform microbench] (compared to previous push)
reviewers	testonly
bugs	1630745, 23032, 1062217, 552085, 2152449, 761034
milestone	77.0a1
first release with	nightly linux32 0f9c5a59e45d / 77.0a1 / 20200501214551 / files nightly linux64 0f9c5a59e45d / 77.0a1 / 20200501214551 / files nightly mac 0f9c5a59e45d / 77.0a1 / 20200501214551 / files nightly win32 0f9c5a59e45d / 77.0a1 / 20200501214551 / files nightly win64 0f9c5a59e45d / 77.0a1 / 20200501214551 / files
last release without	nightly linux32 29e888a9270c / 77.0a1 / 20200501094247 / files nightly linux64 29e888a9270c / 77.0a1 / 20200501094247 / files nightly mac 29e888a9270c / 77.0a1 / 20200501094247 / files nightly win32 29e888a9270c / 77.0a1 / 20200501094247 / files nightly win64 29e888a9270c / 77.0a1 / 20200501094247 / files

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

--- 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>