From 107e77cef0a0b4577e26661d3be9a3326ebcb6b2 Mon Sep 17 00:00:00 2001
From: Anders Hartvoll Ruud <andruud@chromium.org>
Date: Tue, 28 Apr 2020 11:34:46 +0000
Subject: [PATCH] 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
---
 .../animation-important-001.html              | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/testing/web-platform/tests/css/css-animations/animation-important-001.html b/testing/web-platform/tests/css/css-animations/animation-important-001.html
index 5bc3ecfa7501..6b1fcfd76b60 100644
--- a/testing/web-platform/tests/css/css-animations/animation-important-001.html
+++ b/testing/web-platform/tests/css/css-animations/animation-important-001.html
@@ -3,6 +3,20 @@
 <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); }
@@ -39,12 +53,22 @@
   #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)');
@@ -69,4 +93,52 @@ test(() => {
   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>