Bug 1546621 - Ensure pseudo-element declarations are marked as overridden when host element is selected. r=pbro

This patch adjusts the method which marks CSS declarations as overridden to also consider pseudo-elements when they're not directly selected (i.e. their host element is selected).

For style rules on regular elements, there's a condition to only consider rules that match the selected element. This is done so that unmatched rules (you may get one after renaming a selector in the Rules view) don't influence the remaining matching rules. For pseudo-elements, this strict check caused pseudo-element rules to not show overridden declarations if they weren't directly selected in the markup view. For some types of pseudo-elements, it is impossible to ever select their nodes in the markup view because they don't exist, for example ::first-line, ::selection, ::first-letter, etc.

Also introduced an optimization to not call the logic to mark declarations as overridden for non-existent pseudo-elements. The previous code ran a minimum of 17 times, once for every entry in `cssProperties.pseudoElements` array which includes the full dictionary of pseudo-elements supported by Firefox.

Differential Revision: https://phabricator.services.mozilla.com/D32760

--HG--
extra : moz-landing-system : lando

devtools/client/inspector/rules/models/element-style.js

@@ -44,6 +44,7 @@ class ElementStyle {
     this.ruleView = ruleView;
     this.store = store || {};
     this.pageStyle = pageStyle;
+    this.pseudoElements = [];
     this.showUserAgentStyles = showUserAgentStyles;
     this.rules = [];
     this.cssProperties = this.ruleView.cssProperties;
@@ -82,6 +83,7 @@ class ElementStyle {
     }
 
     this.destroyed = true;
+    this.pseudoElements = [];
 
     for (const rule of this.rules) {
       if (rule.editor) {
@@ -135,6 +137,11 @@ class ElementStyle {
         this._maybeAddRule(entry, existingRules);
       }
 
+      // Store a list of all pseudo-element types found in the matching rules.
+      this.pseudoElements = this.rules
+        .filter(r => r.pseudoElement)
+        .map(r => r.pseudoElement);
+
       // Mark overridden computed styles.
       this.onRuleUpdated();
 
@@ -259,20 +266,27 @@ class ElementStyle {
     this.variables.clear();
     this.updateDeclarations();
 
-    for (const pseudo of this.cssProperties.pseudoElements) {
+    // Update declarations for matching rules for pseudo-elements.
+    for (const pseudo of this.pseudoElements) {
       this.updateDeclarations(pseudo);
     }
   }
 
   /**
-   * Mark the declarations for a given pseudo element with an overridden flag if
-   * an earlier property overrides it and update the editor to show it in the
-   * UI. If there is any inactive CSS we also update the editors state to show
-   * the inactive CSS icon.
+   * Go over all CSS rules matching the selected element and mark the CSS declarations
+   * (aka TextProperty instances) with an `overridden` Boolean flag if an earlier or
+   * higher priority declaration overrides it. Rules are already ordered by specificity.
+   *
+   * If a pseudo-element type is passed (ex: ::before, ::first-line, etc),
+   * restrict the operation only to declarations in rules matching that pseudo-element.
+   *
+   * At the end, update the declaration's view (TextPropertyEditor instance) so it relects
+   * the latest state. Use this opportunity to also trigger checks for the "inactive"
+   * state of the declaration (whether it has effect or not).
    *
    * @param  {String} pseudo
-   *         Which pseudo element to flag as overridden.
-   *         Empty string or undefined will default to no pseudo element.
+   *         Optional pseudo-element for which to restrict marking CSS declarations as
+   *         overridden.
    */
   /* eslint-disable complexity */
   updateDeclarations(pseudo = "") {
@@ -282,10 +296,37 @@ class ElementStyle {
     // as time, and animation overlay are required to be check in order to
     // determine if the property is overridden.
     const textProps = [];
+
     for (const rule of this.rules) {
-      if ((rule.matchedSelectors.length > 0 ||
-           rule.domRule.type === ELEMENT_STYLE) &&
-          rule.pseudoElement === pseudo && !rule.keyframes) {
+      // Skip @keyframes rules
+      if (rule.keyframes) {
+        continue;
+      }
+
+      // Style rules must be considered only when they have selectors that match the node.
+      // When renaming a selector, the unmatched rule lingers in the Rule view, but it no
+      // longer matches the node. This strict check avoids accidentally causing
+      // declarations to be overridden in the remaining matching rules.
+      const isStyleRule = rule.pseudoElement === "" && rule.matchedSelectors.length > 0;
+
+      // Style rules for pseudo-elements must always be considered, regardless if their
+      // selector matches the node. As a convenience, declarations in rules for
+      // pseudo-elements show up in a separate Pseudo-elements accordion when selecting
+      // the host node (instead of the pseudo-element node directly, which is sometimes
+      // impossible, for example with ::selection or ::first-line).
+      // Loosening the strict check on matched selectors ensures these declarations
+      // participate in the algorithm below to mark them as overridden.
+      const isPseudoElementRule = rule.pseudoElement !== "" &&
+                                  rule.pseudoElement === pseudo;
+
+      const isElementStyle = rule.domRule.type === ELEMENT_STYLE;
+
+      const filterCondition = pseudo === ""
+        ? (isStyleRule || isElementStyle)
+        : isPseudoElementRule;
+
+      // First, gather all relevant CSS declarations (aka TextProperty instances).
+      if (filterCondition) {
         for (const textProp of rule.textProps.slice(0).reverse()) {
           if (textProp.enabled) {
             textProps.push(textProp);

devtools/client/inspector/rules/test/browser.ini

@@ -221,6 +221,7 @@ skip-if = os == 'linux' # focusEditableField times out consistently on linux.
 [browser_rules_mark_overridden_05.js]
 [browser_rules_mark_overridden_06.js]
 [browser_rules_mark_overridden_07.js]
+[browser_rules_mark_overridden_08.js]
 [browser_rules_mathml-element.js]
 [browser_rules_media-queries_reload.js]
 [browser_rules_media-queries.js]

devtools/client/inspector/rules/test/browser_rules_mark_overridden_08.js

@@ -0,0 +1,43 @@
+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Tests that the rule view marks overridden rules correctly in pseudo-elements when
+// selecting their host node.
+
+const TEST_URI = `
+  <style type='text/css'>
+  #testid::before {
+    content: 'Pseudo-element';
+    color: red;
+    color: green;
+  }
+  #testid {
+    color: blue;
+  }
+  </style>
+  <div id='testid'>Styled Node</div>
+`;
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  const {inspector, view} = await openRuleView();
+  await selectNode("#testid", inspector);
+
+  info("Check the CSS declarations for ::before in the Pseudo-elements accordion.");
+  const pseudoRule = getRuleViewRuleEditor(view, 1, 0).rule;
+  const pseudoProp1 = pseudoRule.textProps[1];
+  const pseudoProp2 = pseudoRule.textProps[2];
+  ok(pseudoProp1.overridden,
+    "First declaration of color in pseudo-element should be overridden.");
+  ok(!pseudoProp2.overridden,
+    "Second declaration of color in pseudo-element should not be overridden.");
+
+  info("Check that pseudo-element declarations do not override the host's declarations");
+  const idRule = getRuleViewRuleEditor(view, 4).rule;
+  const idProp = idRule.textProps[0];
+  ok(!idProp.overridden,
+      "The single declaration of color in ID selector should not be overridden");
+});