Support reverting to/from css-logical properties

The properties defined by css-logical (and more generally surrogates)
pose a hard problem for 'revert' in the StyleCascade, because
declarations such as 'margin-bottom:revert' may actually need to revert
to a different property in the previous origin (e.g. margin-block-end).

An example is the h1 element, which is styled by html.css using
css-logical properties. When the author then reverts using a
corresponding physical property, we can't simply look up the cascaded
value for that same physical property in the UA origin: we must be
aware that the css-logical property won the cascade _at that level_.

So when we we're applying an author declaration 'margin-bottom:revert',
we have to find the *target property* for the revert, which is either
margin-bottom (if that property won the cascade at the target origin),
or the corresponding css-logical property. However, to find the target
property, we need to know during the application of margin-bottom that
a corresponding logical property (surrogate) needs to be taken into
account. AutoSurrogateScope was created for this purpose, which sets
(and resets) a "current surrogate" field on CascadeResolver. This
current surrogate is used when resolving reverts, in order to find
the correct target property.

Also, we don't control the order in which physical/logical properties
are applied. This means we could be applying the physical property
first, which is unaware of any logical properties that should have been
taken into account for revert. In other words, if we apply
'margin-bottom:revert' without the context of a "current surrogate" set,
we'll fall back to unconditionally using margin-bottom as the target
property, which may be incorrect if there also exists a logical
property we'll get to later. This is why the force-reapply code exists:
when both the physical and logical property exists in the cascade,
the physical property must be applied (or re-applied) during
AutoSurrogateScope, otherwise revert will not behave correctly.

Bug: 579788
Change-Id: I864a4c9afdd94f130c42635d28d45c843d5fb617
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2130850
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756419}

third_party/blink/renderer/core/css/resolver/cascade_resolver.cc

@@ -69,4 +69,10 @@ CascadeResolver::AutoLock::~AutoLock() {
     resolver_.cycle_depth_ = kNotFound;
 }
 
+CascadeResolver::AutoSurrogateScope::AutoSurrogateScope(
+    const CSSProperty& surrogate,
+    CascadeResolver& resolver)
+    : base::AutoReset<const CSSProperty*>(&resolver.current_surrogate_,
+                                          &surrogate) {}
+
 }  // namespace blink

third_party/blink/renderer/core/css/resolver/cascade_resolver.h

@@ -5,6 +5,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_RESOLVER_CASCADE_RESOLVER_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_RESOLVER_CASCADE_RESOLVER_H_
 
+#include "base/auto_reset.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/css/css_property_name.h"
 #include "third_party/blink/renderer/core/css/properties/css_property.h"
@@ -16,6 +17,7 @@ namespace blink {
 class CascadePriority;
 class CSSProperty;
 class CSSVariableData;
+class CSSProperty;
 
 namespace cssvalue {
 
@@ -69,8 +71,22 @@ class CORE_EXPORT CascadeResolver {
     CascadeResolver& resolver_;
   };
 
+  // Automatically sets (and resets) the current surrogate.
+  //
+  // We need to know the current surrogate in order to make 'revert' work for
+  // e.g. css-logical properties. See TargetPropertyForRevert for more
+  // information.
+  class CORE_EXPORT AutoSurrogateScope
+      : private base::AutoReset<const CSSProperty*> {
+    STACK_ALLOCATED();
+
+   public:
+    AutoSurrogateScope(const CSSProperty& surrogate, CascadeResolver&);
+  };
+
  private:
   friend class AutoLock;
+  friend class AutoSurrogateScope;
   friend class StyleCascade;
   friend class TestCascadeResolver;
 
@@ -96,6 +112,7 @@ class CORE_EXPORT CascadeResolver {
   wtf_size_t cycle_depth_ = kNotFound;
   CascadeFilter filter_;
   const uint8_t generation_ = 0;
+  const CSSProperty* current_surrogate_ = nullptr;
 
   // A very simple cache for CSSPendingSubstitutionValues. We cache only the
   // most recently parsed CSSPendingSubstitutionValue, such that consecutive

third_party/blink/renderer/core/css/resolver/style_cascade.cc

@@ -112,6 +112,35 @@ CascadeOrigin TargetOriginForRevert(CascadeOrigin origin) {
   }
 }
 
+// When doing 'revert' on a surrogate property (e.g. inline-size), or the
+// corresponding original property (e.g. width), we have to check which of the
+// two properties won the cascade in the target origin. The property with the
+// highest priority in the target origin is the property we need to revert to.
+//
+// For example, if the UA sheet specifies:
+//
+//  h1 { margin-block-end: 0.67em }
+//
+// And the author specifies:
+//
+//  h1 { margin-bottom: revert }
+//
+// Then the expectation is to revert to 0.67em. Without computing the correct
+// target property, we'd revert to 'margin-bottom' in the user-agent origin,
+// which isn't present, and hence it resolve to 'unset', which is not correct.
+const CSSProperty* TargetPropertyForRevert(const CSSProperty& original,
+                                           const CSSProperty* surrogate,
+                                           CascadeOrigin origin,
+                                           CascadeMap& map) {
+  if (!surrogate)
+    return &original;
+  CascadePriority surrogate_priority =
+      map.At(surrogate->GetCSSPropertyName(), origin);
+  CascadePriority original_priority =
+      map.At(original.GetCSSPropertyName(), origin);
+  return (original_priority < surrogate_priority) ? surrogate : &original;
+}
+
 }  // namespace
 
 MatchResult& StyleCascade::MutableMatchResult() {
@@ -383,12 +412,18 @@ void StyleCascade::ApplySurrogate(const CSSProperty& surrogate,
                                   CascadeResolver& resolver) {
   DCHECK(surrogate.IsSurrogate());
 
+  CascadeResolver::AutoSurrogateScope surrogate_scope(surrogate, resolver);
+
   const CSSProperty& original = SurrogateFor(surrogate);
   CascadePriority* original_priority = map_.Find(original.GetCSSPropertyName());
 
   if (original_priority) {
     if (surrogate_priority < *original_priority) {
       // The original has a higher priority, so skip the surrogate property.
+      // However, we need to force-reapply the original property, since applying
+      // with and without surrogate scope can yield different results.
+      resolver.MarkUnapplied(original_priority);
+      LookupAndApply(original, resolver);
       return;
     }
 
@@ -656,6 +691,18 @@ const CSSValue* StyleCascade::ResolveRevert(const CSSProperty& property,
                                             CascadeOrigin origin,
                                             CascadeResolver& resolver) {
   CascadeOrigin target_origin = TargetOriginForRevert(origin);
+  const CSSProperty* target_property = &property;
+
+  // A 'revert' in a surrogate property (e.g. inline-size), or a 'revert' in
+  // a corresponding original property (e.g. width) could mean we need to revert
+  // to a different property on the previous origin.
+  if (const auto* current_surrogate = resolver.current_surrogate_) {
+    const CSSProperty& surrogate_target = SurrogateFor(*current_surrogate);
+    if (&property == &surrogate_target || &property == current_surrogate) {
+      target_property = TargetPropertyForRevert(
+          surrogate_target, current_surrogate, target_origin, map_);
+    }
+  }
 
   switch (target_origin) {
     case CascadeOrigin::kTransition:
@@ -666,11 +713,12 @@ const CSSValue* StyleCascade::ResolveRevert(const CSSProperty& property,
     case CascadeOrigin::kAuthor:
     case CascadeOrigin::kAnimation: {
       CascadePriority* p =
-          map_.Find(property.GetCSSPropertyName(), target_origin);
+          map_.Find(target_property->GetCSSPropertyName(), target_origin);
       if (!p)
         return cssvalue::CSSUnsetValue::Create();
-      return Resolve(property, *ValueAt(match_result_, p->GetPosition()),
-                     target_origin, resolver);
+      return Resolve(*target_property,
+                     *ValueAt(match_result_, p->GetPosition()), target_origin,
+                     resolver);
     }
   }
 }

third_party/blink/renderer/core/css/resolver/style_cascade_test.cc

@@ -1344,6 +1344,159 @@ TEST_F(StyleCascadeTest, RevertRegisteredInheritedFallback) {
   cascade.Apply();
   EXPECT_EQ("1px", cascade.ComputedValue("--x"));
 }
+
+TEST_F(StyleCascadeTest, RevertUASurrogate) {
+  TestCascade cascade(GetDocument());
+
+  // User-agent:
+
+  // Only logical:
+  cascade.Add("inline-size:10px", CascadeOrigin::kUserAgent);
+  cascade.Add("min-inline-size:11px", CascadeOrigin::kUserAgent);
+  // Only physical:
+  cascade.Add("height:12px", CascadeOrigin::kUserAgent);
+  cascade.Add("min-height:13px", CascadeOrigin::kUserAgent);
+  // Physical first:
+  cascade.Add("margin-left:14px", CascadeOrigin::kUserAgent);
+  cascade.Add("padding-left:15px", CascadeOrigin::kUserAgent);
+  cascade.Add("margin-inline-start:16px", CascadeOrigin::kUserAgent);
+  cascade.Add("padding-inline-start:17px", CascadeOrigin::kUserAgent);
+  // Logical first:
+  cascade.Add("margin-inline-end:18px", CascadeOrigin::kUserAgent);
+  cascade.Add("padding-inline-end:19px", CascadeOrigin::kUserAgent);
+  cascade.Add("margin-right:20px", CascadeOrigin::kUserAgent);
+  cascade.Add("padding-right:21px", CascadeOrigin::kUserAgent);
+
+  // Author:
+
+  cascade.Add("width:100px", CascadeOrigin::kAuthor);
+  cascade.Add("height:101px", CascadeOrigin::kAuthor);
+  cascade.Add("margin:102px", CascadeOrigin::kAuthor);
+  cascade.Add("padding:103px", CascadeOrigin::kAuthor);
+  cascade.Add("min-width:104px", CascadeOrigin::kAuthor);
+  cascade.Add("min-height:105px", CascadeOrigin::kAuthor);
+  // Revert via physical:
+  cascade.Add("width:revert", CascadeOrigin::kAuthor);
+  cascade.Add("height:revert", CascadeOrigin::kAuthor);
+  cascade.Add("margin-left:revert", CascadeOrigin::kAuthor);
+  cascade.Add("margin-right:revert", CascadeOrigin::kAuthor);
+  // Revert via logical:
+  cascade.Add("min-inline-size:revert", CascadeOrigin::kAuthor);
+  cascade.Add("min-block-size:revert", CascadeOrigin::kAuthor);
+  cascade.Add("padding-inline-start:revert", CascadeOrigin::kAuthor);
+  cascade.Add("padding-inline-end:revert", CascadeOrigin::kAuthor);
+
+  cascade.Apply();
+
+  EXPECT_EQ("10px", cascade.ComputedValue("width"));
+  EXPECT_EQ("12px", cascade.ComputedValue("height"));
+  EXPECT_EQ("11px", cascade.ComputedValue("min-width"));
+  EXPECT_EQ("13px", cascade.ComputedValue("min-height"));
+  EXPECT_EQ("102px", cascade.ComputedValue("margin-top"));
+  EXPECT_EQ("20px", cascade.ComputedValue("margin-right"));
+  EXPECT_EQ("102px", cascade.ComputedValue("margin-bottom"));
+  EXPECT_EQ("16px", cascade.ComputedValue("margin-left"));
+  EXPECT_EQ("103px", cascade.ComputedValue("padding-top"));
+  EXPECT_EQ("21px", cascade.ComputedValue("padding-right"));
+  EXPECT_EQ("103px", cascade.ComputedValue("padding-bottom"));
+  EXPECT_EQ("17px", cascade.ComputedValue("padding-left"));
+
+  EXPECT_EQ("10px", cascade.ComputedValue("inline-size"));
+  EXPECT_EQ("12px", cascade.ComputedValue("block-size"));
+  EXPECT_EQ("11px", cascade.ComputedValue("min-inline-size"));
+  EXPECT_EQ("13px", cascade.ComputedValue("min-block-size"));
+  EXPECT_EQ("102px", cascade.ComputedValue("margin-block-start"));
+  EXPECT_EQ("20px", cascade.ComputedValue("margin-inline-end"));
+  EXPECT_EQ("102px", cascade.ComputedValue("margin-block-end"));
+  EXPECT_EQ("16px", cascade.ComputedValue("margin-inline-start"));
+  EXPECT_EQ("103px", cascade.ComputedValue("padding-block-start"));
+  EXPECT_EQ("21px", cascade.ComputedValue("padding-inline-end"));
+  EXPECT_EQ("103px", cascade.ComputedValue("padding-block-end"));
+  EXPECT_EQ("17px", cascade.ComputedValue("padding-inline-start"));
+}
+
+TEST_F(StyleCascadeTest, RevertWithImportantPhysical) {
+  TestCascade cascade(GetDocument());
+  cascade.Add("inline-size:10px", CascadeOrigin::kUserAgent);
+  cascade.Add("block-size:11px", CascadeOrigin::kUserAgent);
+
+  cascade.Add("width:100px", CascadeOrigin::kAuthor);
+  cascade.Add("height:101px", CascadeOrigin::kAuthor);
+  cascade.Add("width:revert !important", CascadeOrigin::kAuthor);
+  cascade.Add("inline-size:101px", CascadeOrigin::kAuthor);
+  cascade.Add("block-size:102px", CascadeOrigin::kAuthor);
+  cascade.Add("height:revert !important", CascadeOrigin::kAuthor);
+  cascade.Apply();
+
+  EXPECT_EQ("10px", cascade.ComputedValue("width"));
+  EXPECT_EQ("11px", cascade.ComputedValue("height"));
+  EXPECT_EQ("10px", cascade.ComputedValue("inline-size"));
+  EXPECT_EQ("11px", cascade.ComputedValue("block-size"));
+}
+
+TEST_F(StyleCascadeTest, RevertWithImportantLogical) {
+  TestCascade cascade(GetDocument());
+  cascade.Add("inline-size:10px", CascadeOrigin::kUserAgent);
+  cascade.Add("block-size:11px", CascadeOrigin::kUserAgent);
+
+  cascade.Add("inline-size:revert !important", CascadeOrigin::kAuthor);
+  cascade.Add("width:100px", CascadeOrigin::kAuthor);
+  cascade.Add("height:101px", CascadeOrigin::kAuthor);
+  cascade.Add("block-size:revert !important", CascadeOrigin::kAuthor);
+  cascade.Apply();
+
+  EXPECT_EQ("10px", cascade.ComputedValue("width"));
+  EXPECT_EQ("11px", cascade.ComputedValue("height"));
+  EXPECT_EQ("10px", cascade.ComputedValue("inline-size"));
+  EXPECT_EQ("11px", cascade.ComputedValue("block-size"));
+}
+
+TEST_F(StyleCascadeTest, RevertSurrogateChain) {
+  TestCascade cascade(GetDocument());
+
+  cascade.Add("inline-size:revert", CascadeOrigin::kUserAgent);
+  cascade.Add("block-size:10px", CascadeOrigin::kUserAgent);
+  cascade.Add("min-inline-size:11px", CascadeOrigin::kUserAgent);
+  cascade.Add("min-block-size:12px", CascadeOrigin::kUserAgent);
+  cascade.Add("margin-inline:13px", CascadeOrigin::kUserAgent);
+  cascade.Add("margin-block:14px", CascadeOrigin::kUserAgent);
+  cascade.Add("margin-top:revert", CascadeOrigin::kUserAgent);
+  cascade.Add("margin-left:15px", CascadeOrigin::kUserAgent);
+  cascade.Add("margin-bottom:16px", CascadeOrigin::kUserAgent);
+  cascade.Add("margin-block-end:17px", CascadeOrigin::kUserAgent);
+
+  cascade.Add("inline-size:101px", CascadeOrigin::kUser);
+  cascade.Add("block-size:102px", CascadeOrigin::kUser);
+  cascade.Add("width:revert", CascadeOrigin::kUser);
+  cascade.Add("height:revert", CascadeOrigin::kUser);
+  cascade.Add("min-inline-size:103px", CascadeOrigin::kUser);
+  cascade.Add("min-block-size:104px", CascadeOrigin::kUser);
+  cascade.Add("margin:105px", CascadeOrigin::kUser);
+  cascade.Add("margin-block-start:revert", CascadeOrigin::kUser);
+  cascade.Add("margin-inline-start:106px", CascadeOrigin::kUser);
+  cascade.Add("margin-block-end:revert", CascadeOrigin::kUser);
+  cascade.Add("margin-right:107px", CascadeOrigin::kUser);
+
+  cascade.Add("inline-size:revert", CascadeOrigin::kAuthor);
+  cascade.Add("block-size:revert", CascadeOrigin::kAuthor);
+  cascade.Add("min-inline-size:revert", CascadeOrigin::kAuthor);
+  cascade.Add("min-block-size:1001px", CascadeOrigin::kAuthor);
+  cascade.Add("margin:1002px", CascadeOrigin::kAuthor);
+  cascade.Add("margin-top:revert", CascadeOrigin::kAuthor);
+  cascade.Add("margin-left:1003px", CascadeOrigin::kAuthor);
+  cascade.Add("margin-bottom:1004px", CascadeOrigin::kAuthor);
+  cascade.Add("margin-right:1005px", CascadeOrigin::kAuthor);
+  cascade.Apply();
+
+  EXPECT_EQ("auto", cascade.ComputedValue("width"));
+  EXPECT_EQ("10px", cascade.ComputedValue("height"));
+  EXPECT_EQ("103px", cascade.ComputedValue("min-width"));
+  EXPECT_EQ("1001px", cascade.ComputedValue("min-height"));
+  EXPECT_EQ("0px", cascade.ComputedValue("margin-top"));
+  EXPECT_EQ("1005px", cascade.ComputedValue("margin-right"));
+  EXPECT_EQ("1004px", cascade.ComputedValue("margin-bottom"));
+  EXPECT_EQ("1003px", cascade.ComputedValue("margin-left"));
+}
 TEST_F(StyleCascadeTest, RegisteredInitial) {
   RegisterProperty(GetDocument(), "--x", "<length>", "0px", false);
 

third_party/blink/web_tests/external/wpt/css/css-cascade/revert-val-003-expected.txt

-This is a testharness.js-based test.
-FAIL revert works with transitions assert_not_equals: Margin mid transition got disallowed value "0px"
-Harness: the test ran to completion.
-

third_party/blink/web_tests/external/wpt/css/css-cascade/revert-val-005.html

+<!DOCTYPE html>
+<title>CSS Cascade: 'revert' in css-logical properties</title>
+<link rel="help" href="https://drafts.csswg.org/css-cascade/#default">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  #h1_physical {
+    margin: 0px;
+    margin: revert;
+  }
+  #h1_logical {
+    margin: 0px;
+    margin-inline-start: revert;
+    margin-inline-end: revert;
+    margin-block-start: revert;
+    margin-block-end: revert;
+  }
+</style>
+<h1 id=h1_physical></h1>
+<h1 id=h1_logical></h1>
+<h1 id=ref></h1>
+<script>
+  test(function() {
+    let actual = getComputedStyle(h1_physical).marginTop;
+    let expected = getComputedStyle(ref).marginTop;
+    // This test assumes that the UA style sheet sets a non-0px value on
+    // <h1> elements:
+    assert_not_equals(expected, '0px');
+    assert_equals(actual, expected);
+  }, 'The revert keyword works with physical properties');
+
+  test(function() {
+    let actual = getComputedStyle(h1_logical).marginTop;
+    let expected = getComputedStyle(ref).marginTop;
+    // This test assumes that the UA style sheet sets a non-0px value on
+    // <h1> elements:
+    assert_not_equals(expected, '0px');
+    assert_equals(actual, expected);
+  }, 'The revert keyword works with logical properties');
+</script>