Make HasBackground/Border flags monotonic

There's currently a bug where a transition on an element with appearance can cause these flags to disappear. This is because the cascade is applied (in full) _twice_ for style resolves where the base computed style optimization is not in use: the first apply produces the base computed style, and the second apply is the same as the first apply, except that it also includes animation effects. The problem is that the second apply can reset the flags if there's a transition (for example) for the property or properties to be set in the first apply round. For example, assume there's a regular declaration background-color:green. This causes HasAuthorBackground to be set after the first apply round. Then, in the second apply round, if there's an animation effect on background-color, then CascadeResolver::AuthorFlags will not have kBackground set, since transitions are not author declarations, and therefore the flag will be set back to false. Instead this CL makes the flags monotonic, such that we're not overwriting the flags later. This fixes a regression where artifacts are shown when transitioning UI elements. Note that it may seem weird that transition/animation declarations don't count as author declarations in this context, but this was done to match the old Blink behavior where transitions/animations also had no effect on the flags. Note also that the now-removed logic regarding Find/Reject/etc. in the if-statement is no longer needed. It was there because forced colors mode used a separate StyleCascade instance previously, which made matters a bit more complicated. However, this has since been changed, so the logic can be simplified. Bug: 1086732 Change-Id: I6e588a7e1d6805ab0a59a9a92bec0eb26e7120eb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214552Reviewed-by: Rune Lillesveen <futhark@chromium.org> Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Cr-Commit-Position: refs/heads/master@{#772169}

Make HasBackground/Border flags monotonic
There's currently a bug where a transition on an element with appearance can cause these flags to disappear. This is because the cascade is applied (in full) _twice_ for style resolves where the base computed style optimization is not in use: the first apply produces the base computed style, and the second apply is the same as the first apply, except that it also includes animation effects. The problem is that the second apply can reset the flags if there's a transition (for example) for the property or properties to be set in the first apply round. For example, assume there's a regular declaration background-color:green. This causes HasAuthorBackground to be set after the first apply round. Then, in the second apply round, if there's an animation effect on background-color, then CascadeResolver::AuthorFlags will not have kBackground set, since transitions are not author declarations, and therefore the flag will be set back to false. Instead this CL makes the flags monotonic, such that we're not overwriting the flags later. This fixes a regression where artifacts are shown when transitioning UI elements. Note that it may seem weird that transition/animation declarations don't count as author declarations in this context, but this was done to match the old Blink behavior where transitions/animations also had no effect on the flags. Note also that the now-removed logic regarding Find/Reject/etc. in the if-statement is no longer needed. It was there because forced colors mode used a separate StyleCascade instance previously, which made matters a bit more complicated. However, this has since been changed, so the logic can be simplified. Bug: 1086732 Change-Id: I6e588a7e1d6805ab0a59a9a92bec0eb26e7120eb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214552Reviewed-by: Rune Lillesveen <futhark@chromium.org> Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Cr-Commit-Position: refs/heads/master@{#772169}
1af5ee6f · Anders Hartvoll Ruud · Commit Bot · d817741d · 1af5ee6f · 1af5ee6f
Commit 1af5ee6f authored May 27, 2020 by Anders Hartvoll Ruud Committed by Commit Bot May 27, 2020
3 changed files
--- a/third_party/blink/renderer/core/css/resolver/style_cascade.cc
+++ b/third_party/blink/renderer/core/css/resolver/style_cascade.cc
@@ -146,12 +146,11 @@ void StyleCascade::Apply(CascadeFilter filter) {
  ApplyMatchResult(resolver);
  ApplyInterpolations(resolver);
-  if (map_.Find(CSSPropertyName(CSSPropertyID::kAppearance)) &&
+  if (state_.Style()->HasAppearance()) {
-      !resolver.filter_.Rejects(GetCSSPropertyAppearance()) &&
+    if (resolver.AuthorFlags() & CSSProperty::kBackground)
-      state_.Style()->HasAppearance()) {
+      state_.Style()->SetHasAuthorBackground(true);
-    CSSProperty::Flags flags = resolver.AuthorFlags();
+    if (resolver.AuthorFlags() & CSSProperty::kBorder)
-    state_.Style()->SetHasAuthorBackground(flags & CSSProperty::kBackground);
+      state_.Style()->SetHasAuthorBorder(true);
-    state_.Style()->SetHasAuthorBorder(flags & CSSProperty::kBorder);
  }
 }

--- a/third_party/blink/web_tests/external/wpt/css/css-ui/appearance-transition-ref.html
+++ b/third_party/blink/web_tests/external/wpt/css/css-ui/appearance-transition-ref.html
+<!DOCTYPE html>
+<style>
+  input { background-color: rgb(0, 100, 0); }
+</style>
+<input value="text">
+<p>PASS if the input field has a dark green background</p>
--- a/third_party/blink/web_tests/external/wpt/css/css-ui/appearance-transition.tentative.html
+++ b/third_party/blink/web_tests/external/wpt/css/css-ui/appearance-transition.tentative.html
+<!DOCTYPE html>
+<link rel="help" href="https://crbug.com/1086732">
+<link rel="match" href="appearance-transition-ref.html">
+<style>
+  input {
+    background-color: rgb(0, 0, 0);
+    transition: background-color 1e10s steps(2, start);
+  }
+  .bg200 {
+    background-color: rgb(0, 200, 0);
+  }
+</style>
+<input value="text" id=input>
+<script>
+  document.documentElement.offsetTop;
+  input.classList.toggle('bg200');
+</script>
+<p>PASS if the input field has a dark green background</p>