[@property] Support @property in user stylesheets

It's currently a bit tricky to add support for this, because
StyleEngine can't currently gracefully support the scenario of
user/author rules sharing a single storage location. (We have a
similar issue with the font cache).

This CL handles @property rules roughly the same way as
@font-face rules are handled:

 - When adding user @property rules, clear all rules (including
   author), then add user rules.
 - MarkDocumentDirty + SetNeedsAppendAllSheets, to ensure that
   we ApplyRuleSetChanges for author rules.
 - When adding author @property rules, clear all rules (including
   user), then add all user rules, then author rules.

This should cover all the different cases where only user sheets are
present/modified, only author sheets present/modified, or when both
types of sheets are present/modified.

It's a bit of extra clear/add churn, but storing author/user rules
separately will add much more complexity (e.g. to PropertyRegistry::
Iterator).

Note: I seem to have declared ClearPropertyRules in a previous CL
without defining it.

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

third_party/blink/renderer/core/css/property_registration.cc

@@ -233,8 +233,17 @@ void PropertyRegistration::registerProperty(
 }
 
 void PropertyRegistration::RemoveDeclaredProperties(Document& document) {
-  document.EnsurePropertyRegistry().RemoveDeclaredProperties();
-  document.GetStyleEngine().PropertyRegistryChanged();
+  if (!document.GetPropertyRegistry())
+    return;
+
+  PropertyRegistry& registry = document.EnsurePropertyRegistry();
+
+  size_t version_before = registry.Version();
+  registry.RemoveDeclaredProperties();
+  size_t version_after = registry.Version();
+
+  if (version_before != version_after)
+    document.GetStyleEngine().PropertyRegistryChanged();
 }
 
 }  // namespace blink

third_party/blink/renderer/core/css/property_registry.cc

@@ -21,6 +21,8 @@ void PropertyRegistry::DeclareProperty(const AtomicString& name,
 }
 
 void PropertyRegistry::RemoveDeclaredProperties() {
+  if (declared_properties_.IsEmpty())
+    return;
   declared_properties_.clear();
   version_++;
 }

third_party/blink/renderer/core/css/property_registry_test.cc

@@ -230,6 +230,9 @@ TEST_F(PropertyRegistryTest, Version) {
 
   Registry().RemoveDeclaredProperties();
   EXPECT_EQ(6u, Registry().Version());
+
+  Registry().RemoveDeclaredProperties();
+  EXPECT_EQ(6u, Registry().Version());
 }
 
 TEST_F(PropertyRegistryTest, RemoveDeclaredProperties) {

third_party/blink/renderer/core/css/style_engine.cc

@@ -1599,6 +1599,19 @@ void StyleEngine::ApplyUserRuleSetChanges(
     ScopedStyleResolver::KeyframesRulesAdded(GetDocument());
   }
 
+  if (changed_rule_flags & kPropertyRules) {
+    ClearPropertyRules();
+    AddPropertyRulesFromSheets(new_style_sheets);
+
+    // We just cleared all the rules, which includes any author rules. They
+    // must be forcibly re-added.
+    if (ScopedStyleResolver* scoped_resolver =
+            GetDocument().GetScopedStyleResolver()) {
+      scoped_resolver->SetNeedsAppendAllSheets();
+      MarkDocumentDirty();
+    }
+  }
+
   if ((changed_rule_flags & kFontFaceRules) || has_rebuilt_font_face_cache) {
     GetFontSelector()->FontFaceInvalidated(
         FontInvalidationReason::kGeneralInvalidation);
@@ -1624,9 +1637,11 @@ void StyleEngine::ApplyRuleSetChanges(
   bool rebuild_font_face_cache = change == kActiveSheetsChanged &&
                                  (changed_rule_flags & kFontFaceRules) &&
                                  tree_scope.RootNode().IsDocumentNode();
+  bool rebuild_at_property_registry = false;
   ScopedStyleResolver* scoped_resolver = tree_scope.GetScopedStyleResolver();
   if (scoped_resolver && scoped_resolver->NeedsAppendAllSheets()) {
     rebuild_font_face_cache = true;
+    rebuild_at_property_registry = true;
     change = kActiveSheetsChanged;
   }
 
@@ -1639,16 +1654,13 @@ void StyleEngine::ApplyRuleSetChanges(
   if (changed_rule_flags & kKeyframesRules)
     ScopedStyleResolver::KeyframesRulesAdded(tree_scope);
 
-  if (changed_rule_flags & kPropertyRules) {
+  if ((changed_rule_flags & kPropertyRules) || rebuild_at_property_registry) {
     // @property rules are (for now) ignored in shadow trees, per spec.
     // https://drafts.css-houdini.org/css-properties-values-api-1/#at-property-rule
     if (tree_scope.RootNode().IsDocumentNode()) {
-      PropertyRegistration::RemoveDeclaredProperties(GetDocument());
-
-      for (const ActiveStyleSheet& active_sheet : new_style_sheets) {
-        if (RuleSet* rule_set = active_sheet.second)
-          AddPropertyRules(*rule_set);
-      }
+      ClearPropertyRules();
+      AddPropertyRulesFromSheets(active_user_style_sheets_);
+      AddPropertyRulesFromSheets(new_style_sheets);
     }
   }
 
@@ -1859,6 +1871,18 @@ void StyleEngine::CollectMatchingUserRules(
   }
 }
 
+void StyleEngine::ClearPropertyRules() {
+  PropertyRegistration::RemoveDeclaredProperties(GetDocument());
+}
+
+void StyleEngine::AddPropertyRulesFromSheets(
+    const ActiveStyleSheetVector& sheets) {
+  for (const ActiveStyleSheet& active_sheet : sheets) {
+    if (RuleSet* rule_set = active_sheet.second)
+      AddPropertyRules(*rule_set);
+  }
+}
+
 bool StyleEngine::AddUserFontFaceRules(const RuleSet& rule_set) {
   if (!font_selector_)
     return false;

third_party/blink/renderer/core/css/style_engine.h

@@ -486,6 +486,8 @@ class CORE_EXPORT StyleEngine final : public GarbageCollected<StyleEngine>,
   void ClearKeyframeRules() { keyframes_rule_map_.clear(); }
   void ClearPropertyRules();
 
+  void AddPropertyRulesFromSheets(const ActiveStyleSheetVector&);
+
   // Returns true if any @font-face rules are added.
   bool AddUserFontFaceRules(const RuleSet&);
   void AddUserKeyframeRules(const RuleSet&);

third_party/blink/renderer/core/css/style_engine_test.cc

@@ -21,6 +21,7 @@
 #include "third_party/blink/renderer/core/css/css_style_sheet.h"
 #include "third_party/blink/renderer/core/css/css_test_helpers.h"
 #include "third_party/blink/renderer/core/css/parser/css_parser_context.h"
+#include "third_party/blink/renderer/core/css/properties/css_property_ref.h"
 #include "third_party/blink/renderer/core/css/style_sheet_contents.h"
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/first_letter_pseudo_element.h"
@@ -73,6 +74,22 @@ class StyleEngineTest : public testing::Test {
     return GetStyleEngine().style_recalc_root_.GetRootNode();
   }
 
+  const CSSValue* ComputedValue(Element* element, String property_name) {
+    CSSPropertyRef ref(property_name, GetDocument());
+    DCHECK(ref.IsValid());
+    return ref.GetProperty().CSSValueFromComputedStyle(
+        element->ComputedStyleRef(),
+        /* layout_object */ nullptr,
+        /* allow_visited_style */ false);
+  }
+
+  void InjectSheet(String key, WebDocument::CSSOrigin origin, String text) {
+    auto* context = MakeGarbageCollected<CSSParserContext>(GetDocument());
+    auto* sheet = MakeGarbageCollected<StyleSheetContents>(context);
+    sheet->ParseString(text);
+    GetStyleEngine().InjectSheet(StyleSheetKey(key), sheet, origin);
+  }
+
  private:
   std::unique_ptr<DummyPageHolder> dummy_page_holder_;
 };
@@ -2739,6 +2756,54 @@ TEST_F(StyleEngineTest, MediaQueryAffectedByViewportSanityCheck) {
   EXPECT_FALSE(GetStyleEngine().MediaQueryAffectedByViewportChange());
 }
 
+TEST_F(StyleEngineTest, RemoveDeclaredPropertiesEmptyRegistry) {
+  ScopedCSSVariables2AtPropertyForTest scoped_feature(true);
+
+  EXPECT_FALSE(GetDocument().GetPropertyRegistry());
+  PropertyRegistration::RemoveDeclaredProperties(GetDocument());
+  EXPECT_FALSE(GetDocument().GetPropertyRegistry());
+}
+
+TEST_F(StyleEngineTest, AtPropertyInUserOrigin) {
+  // @property in the user origin:
+  InjectSheet("user1", WebDocument::kUserOrigin, R"CSS(
+    @property --x {
+      syntax: "<length>";
+      inherits: false;
+      initial-value: 10px;
+    }
+  )CSS");
+  UpdateAllLifecyclePhases();
+  ASSERT_TRUE(ComputedValue(GetDocument().body(), "--x"));
+  EXPECT_EQ("10px", ComputedValue(GetDocument().body(), "--x")->CssText());
+
+  // @property in the author origin (should win over user origin)
+  InjectSheet("author", WebDocument::kAuthorOrigin, R"CSS(
+    @property --x {
+      syntax: "<length>";
+      inherits: false;
+      initial-value: 20px;
+    }
+  )CSS");
+  UpdateAllLifecyclePhases();
+  ASSERT_TRUE(ComputedValue(GetDocument().body(), "--x"));
+  EXPECT_EQ("20px", ComputedValue(GetDocument().body(), "--x")->CssText());
+
+  // An additional @property in the user origin:
+  InjectSheet("user2", WebDocument::kUserOrigin, R"CSS(
+    @property --y {
+      syntax: "<length>";
+      inherits: false;
+      initial-value: 30px;
+    }
+  )CSS");
+  UpdateAllLifecyclePhases();
+  ASSERT_TRUE(ComputedValue(GetDocument().body(), "--x"));
+  ASSERT_TRUE(ComputedValue(GetDocument().body(), "--y"));
+  EXPECT_EQ("20px", ComputedValue(GetDocument().body(), "--x")->CssText());
+  EXPECT_EQ("30px", ComputedValue(GetDocument().body(), "--y")->CssText());
+}
+
 class ParameterizedStyleEngineTest
     : public testing::WithParamInterface<bool>,
       private ScopedCSSReducedFontLoadingInvalidationsForTest,