Change content:attr() invalidation to work for custom properties.

This simplifies invalidation of attribute changes affecting attr()
content from always invalidating for attributes used in attr() on all
elements, to always invalidating with kLocalStyleChange for attr changes
on elements which have ::before/::after content generated from attr().

This means we don't need to block lazy parsing for ::before/::after and
we will respond correctly to attr() used indirectly via var().

Bug: 791525
Change-Id: I13878a56ecf11eb3bd9da356128ac0f32dd5b62d
Reviewed-on: https://chromium-review.googlesource.com/c/1288274
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600796}

third_party/WebKit/LayoutTests/TestExpectations

@@ -4226,7 +4226,6 @@ crbug.com/715718 [ Win ] external/wpt/css/css-flexbox/flex-minimum-width-flex-it
 crbug.com/715718 [ Win ] external/wpt/css/css-flexbox/flexbox_flex-natural-mixed-basis-auto.html [ Failure Pass ]
 
 # New failure when importing css-variables:
-crbug.com/791525 external/wpt/css/css-variables/variable-generated-content-dynamic-001.html [ Failure ]
 crbug.com/791529 external/wpt/css/css-variables/variable-transitions-from-no-value.html [ Skip ]
 crbug.com/791529 external/wpt/css/css-variables/variable-transitions-to-no-value.html [ Skip ]
 crbug.com/791529 external/wpt/css/css-variables/variable-transitions-transition-property-variable-before-value.html [ Skip ]

third_party/WebKit/LayoutTests/fast/css/invalidation/content-attr-expected.txt

@@ -3,9 +3,10 @@ No subtree recalc when changing attribute used in generated content.
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS internals.updateStyleAndReturnAffectedElementCount() is 0
 PASS internals.updateStyleAndReturnAffectedElementCount() is 2
 PASS internals.updateStyleAndReturnAffectedElementCount() is 2
+PASS internals.updateStyleAndReturnAffectedElementCount() is 2
+PASS internals.updateStyleAndReturnAffectedElementCount() is 0
 PASS successfullyParsed is true
 
 TEST COMPLETE

third_party/WebKit/LayoutTests/fast/css/invalidation/content-attr.html

@@ -2,6 +2,7 @@
 <script src="../../../resources/js-test.js"></script>
 <style>
 #before::before, #after::after { content: attr(my-value); }
+#before2::before { content: "before"; }
 </style>
 <div id="before">
     <div></div>
@@ -15,6 +16,12 @@
         <div></div>
     </div>
 </div>
+<div id="before2">
+    <div></div>
+    <div>
+        <div></div>
+    </div>
+</div>
 <script>
 description("No subtree recalc when changing attribute used in generated content.");
 
@@ -25,7 +32,7 @@ document.body.offsetTop; // force layout
 
 before.setAttribute("attr-unused", "unused");
 if (window.internals)
-    shouldBe("internals.updateStyleAndReturnAffectedElementCount()", "0");
+    shouldBe("internals.updateStyleAndReturnAffectedElementCount()", "2");
 
 document.body.offsetTop; // force layout
 
@@ -38,4 +45,10 @@ document.body.offsetTop; // force layout
 after.setAttribute("my-value", "after");
 if (window.internals)
     shouldBe("internals.updateStyleAndReturnAffectedElementCount()", "2");
+
+document.body.offsetTop; // force layout
+
+before2.setAttribute("my-value", "before");
+if (window.internals)
+    shouldBe("internals.updateStyleAndReturnAffectedElementCount()", "0");
 </script>

third_party/blink/renderer/core/css/parser/css_lazy_parsing_state.cc

@@ -35,27 +35,6 @@ const CSSParserContext* CSSLazyParsingState::Context() {
   return context_;
 }
 
-bool CSSLazyParsingState::ShouldLazilyParseProperties(
-    const CSSSelectorList& selectors) const {
-  //  Disallow lazy parsing for blocks which have before/after in their selector
-  //  list. This ensures we don't cause a collectFeatures() when we trigger
-  //  parsing for attr() functions which would trigger expensive invalidation
-  //  propagation.
-  for (const auto* s = selectors.FirstForCSSOM(); s;
-       s = CSSSelectorList::Next(*s)) {
-    for (const CSSSelector* current = s; current;
-         current = current->TagHistory()) {
-      const CSSSelector::PseudoType type(current->GetPseudoType());
-      if (type == CSSSelector::kPseudoBefore ||
-          type == CSSSelector::kPseudoAfter)
-        return false;
-      if (current->Relation() != CSSSelector::kSubSelector)
-        break;
-    }
-  }
-  return true;
-}
-
 void CSSLazyParsingState::Trace(blink::Visitor* visitor) {
   visitor->Trace(owning_contents_);
   visitor->Trace(document_);

third_party/blink/renderer/core/css/parser/css_lazy_parsing_state.h

@@ -30,7 +30,6 @@ class CSSLazyParsingState
 
   const CSSParserContext* Context();
   const String& SheetText() const { return sheet_text_; }
-  bool ShouldLazilyParseProperties(const CSSSelectorList&) const;
 
   void Trace(blink::Visitor*);
 

third_party/blink/renderer/core/css/parser/css_lazy_parsing_test.cc

@@ -45,9 +45,7 @@ TEST_F(CSSLazyParsingTest, Simple) {
   EXPECT_TRUE(HasParsedProperties(rule));
 }
 
-// Avoid parsing rules with ::before or ::after to avoid causing
-// collectFeatures() when we trigger parsing for attr();
-TEST_F(CSSLazyParsingTest, DontLazyParseBeforeAfter) {
+TEST_F(CSSLazyParsingTest, LazyParseBeforeAfter) {
   CSSParserContext* context = CSSParserContext::Create(
       kHTMLStandardMode, SecureContextMode::kInsecureContext);
   StyleSheetContents* style_sheet = StyleSheetContents::Create(context);
@@ -57,8 +55,8 @@ TEST_F(CSSLazyParsingTest, DontLazyParseBeforeAfter) {
   CSSParser::ParseSheet(context, style_sheet, sheet_text,
                         CSSDeferPropertyParsing::kYes);
 
-  EXPECT_TRUE(HasParsedProperties(RuleAt(style_sheet, 0)));
-  EXPECT_TRUE(HasParsedProperties(RuleAt(style_sheet, 1)));
+  EXPECT_FALSE(HasParsedProperties(RuleAt(style_sheet, 0)));
+  EXPECT_FALSE(HasParsedProperties(RuleAt(style_sheet, 1)));
 }
 
 // Test for crbug.com/664115 where |shouldConsiderForMatchingRules| would flip
@@ -88,8 +86,8 @@ TEST_F(CSSLazyParsingTest, ShouldConsiderForMatchingRulesDoesntChange1) {
       rule->ShouldConsiderForMatchingRules(false /* includeEmptyRules */));
 }
 
-// Test the same thing as above, with a property that does not get lazy parsed,
-// to ensure that we perform the optimization where possible.
+// Test the same thing as above with lazy parsing off to ensure that we perform
+// the optimization where possible.
 TEST_F(CSSLazyParsingTest, ShouldConsiderForMatchingRulesSimple) {
   CSSParserContext* context = CSSParserContext::Create(
       kHTMLStandardMode, SecureContextMode::kInsecureContext);
@@ -97,7 +95,7 @@ TEST_F(CSSLazyParsingTest, ShouldConsiderForMatchingRulesSimple) {
 
   String sheet_text = "p::before { ,badness, } ";
   CSSParser::ParseSheet(context, style_sheet, sheet_text,
-                        CSSDeferPropertyParsing::kYes);
+                        CSSDeferPropertyParsing::kNo);
 
   StyleRule* rule = RuleAt(style_sheet, 0);
   EXPECT_TRUE(HasParsedProperties(rule));

third_party/blink/renderer/core/css/parser/css_parser_impl.cc

@@ -824,8 +824,7 @@ StyleRule* CSSParserImpl::ConsumeStyleRule(CSSParserTokenStream& stream) {
     return nullptr;  // Parse error, invalid selector list
 
   // TODO(csharrison): How should we lazily parse css that needs the observer?
-  if (!observer_ && lazy_state_ &&
-      lazy_state_->ShouldLazilyParseProperties(selector_list)) {
+  if (!observer_ && lazy_state_) {
     DCHECK(style_sheet_);
     return StyleRule::CreateLazy(
         std::move(selector_list),

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

@@ -415,10 +415,6 @@ void RuleFeatureSet::ExtractInvalidationSetFeaturesFromSimpleSelector(
     case CSSSelector::kPseudoBlinkInternalElement:
       features.invalidation_flags.SetInvalidateCustomPseudo(true);
       return;
-    case CSSSelector::kPseudoBefore:
-    case CSSSelector::kPseudoAfter:
-      features.has_before_or_after = true;
-      return;
     case CSSSelector::kPseudoSlotted:
       features.invalidation_flags.SetInvalidatesSlotted(true);
       return;
@@ -526,8 +522,6 @@ void RuleFeatureSet::UpdateInvalidationSets(const RuleData* rule_data) {
     features.invalidation_flags.SetWholeSubtreeInvalid(true);
   if (features.has_nth_pseudo)
     AddFeaturesToInvalidationSet(EnsureNthInvalidationSet(), features);
-  if (features.has_before_or_after)
-    UpdateInvalidationSetsForContentAttribute(rule_data);
 
   const CSSSelector* next_compound = last_in_compound
                                          ? last_in_compound->TagHistory()
@@ -567,39 +561,6 @@ void RuleFeatureSet::UpdateRuleSetInvalidation(
     type_rule_invalidation_set_->AddTagName(tag_name);
 }
 
-void RuleFeatureSet::UpdateInvalidationSetsForContentAttribute(
-    const RuleData* rule_data) {
-  // If any ::before and ::after rules specify 'content: attr(...)', we
-  // need to create invalidation sets for those attributes to have content
-  // changes applied through style recalc.
-
-  const CSSPropertyValueSet& property_set = rule_data->Rule()->Properties();
-
-  int property_index = property_set.FindPropertyIndex(CSSPropertyContent);
-
-  if (property_index == -1)
-    return;
-
-  CSSPropertyValueSet::PropertyReference content_property =
-      property_set.PropertyAt(property_index);
-  const CSSValue& content_value = content_property.Value();
-
-  if (!content_value.IsValueList())
-    return;
-
-  for (auto& item : ToCSSValueList(content_value)) {
-    if (!item->IsFunctionValue())
-      continue;
-    const CSSFunctionValue* function_value = ToCSSFunctionValue(item.Get());
-    if (function_value->FunctionType() != CSSValueAttr)
-      continue;
-    EnsureAttributeInvalidationSet(
-        AtomicString(ToCSSCustomIdentValue(function_value->Item(0)).Value()),
-        kInvalidateDescendants, kSubject)
-        .SetInvalidatesSelf();
-  }
-}
-
 RuleFeatureSet::FeatureInvalidationType
 RuleFeatureSet::ExtractInvalidationSetFeaturesFromSelectorList(
     const CSSSelector& simple_selector,
@@ -1260,7 +1221,6 @@ void RuleFeatureSet::InvalidationSetFeatures::Add(
   max_direct_adjacent_selectors = std::max(max_direct_adjacent_selectors,
                                            other.max_direct_adjacent_selectors);
   invalidation_flags.Merge(other.invalidation_flags);
-  has_before_or_after |= other.has_before_or_after;
   content_pseudo_crossing |= other.content_pseudo_crossing;
   has_nth_pseudo |= other.has_nth_pseudo;
 }

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

@@ -194,7 +194,6 @@ class CORE_EXPORT RuleFeatureSet {
   DescendantInvalidationSet& EnsurePartInvalidationSet();
 
   void UpdateInvalidationSets(const RuleData*);
-  void UpdateInvalidationSetsForContentAttribute(const RuleData*);
 
   struct InvalidationSetFeatures {
     DISALLOW_NEW();
@@ -209,7 +208,6 @@ class CORE_EXPORT RuleFeatureSet {
     Vector<AtomicString> tag_names;
     unsigned max_direct_adjacent_selectors = 0;
     InvalidationFlags invalidation_flags;
-    bool has_before_or_after = false;
     bool content_pseudo_crossing = false;
     bool has_nth_pseudo = false;
     bool has_features_for_rule_set_invalidation = false;

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

@@ -47,6 +47,7 @@
 #include "third_party/blink/renderer/core/dom/element.h"
 #include "third_party/blink/renderer/core/dom/element_traversal.h"
 #include "third_party/blink/renderer/core/dom/layout_tree_builder_traversal.h"
+#include "third_party/blink/renderer/core/dom/node_computed_style.h"
 #include "third_party/blink/renderer/core/dom/processing_instruction.h"
 #include "third_party/blink/renderer/core/dom/shadow_root.h"
 #include "third_party/blink/renderer/core/dom/text.h"
@@ -842,6 +843,24 @@ void StyleEngine::ClassChangedForElement(const SpaceSplitString& old_classes,
                                                          element);
 }
 
+namespace {
+
+bool HasAttributeDependentGeneratedContent(const Element& element) {
+  if (PseudoElement* before = element.GetPseudoElement(kPseudoIdBefore)) {
+    const ComputedStyle* style = before->GetComputedStyle();
+    if (style && style->HasAttrContent())
+      return true;
+  }
+  if (PseudoElement* after = element.GetPseudoElement(kPseudoIdAfter)) {
+    const ComputedStyle* style = after->GetComputedStyle();
+    if (style && style->HasAttrContent())
+      return true;
+  }
+  return false;
+}
+
+}  // namespace
+
 void StyleEngine::AttributeChangedForElement(
     const QualifiedName& attribute_name,
     Element& element) {
@@ -853,6 +872,13 @@ void StyleEngine::AttributeChangedForElement(
       invalidation_lists, element, attribute_name);
   pending_invalidations_.ScheduleInvalidationSetsForNode(invalidation_lists,
                                                          element);
+
+  if (!element.NeedsStyleRecalc() &&
+      HasAttributeDependentGeneratedContent(element)) {
+    element.SetNeedsStyleRecalc(
+        kLocalStyleChange,
+        StyleChangeReasonForTracing::FromAttribute(attribute_name));
+  }
 }
 
 void StyleEngine::IdChangedForElement(const AtomicString& old_id,