Bug 1528288 - Introduce a tooltip when hovering over box-model values to show the rule where a value is coming from r=rcaliman

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

--HG--
rename : devtools/client/inspector/boxmodel/test/browser_boxmodel_jump-to-rule-on-hover.js => devtools/client/inspector/boxmodel/test/browser_boxmodel_jump-to-rule-on-click.js
extra : moz-landing-system : lando

devtools/client/inspector/boxmodel/box-model.js

@@ -4,6 +4,7 @@
 
 "use strict";
 
+const Services = require("Services");
 const boxModelReducer = require("devtools/client/inspector/boxmodel/reducers/box-model");
 const {
   updateGeometryEditorEnabled,
@@ -11,6 +12,10 @@ const {
   updateOffsetParent,
 } = require("devtools/client/inspector/boxmodel/actions/box-model");
 
+const HIGHLIGHT_RULE_PREF = Services.prefs.getBoolPref(
+  "devtools.layout.boxmodel.highlightProperty"
+);
+
 loader.lazyRequireGetter(
   this,
   "EditingSession",
@@ -309,14 +314,16 @@ BoxModel.prototype = {
    *         The name of the property.
    */
   onShowRulePreviewTooltip(target, property) {
-    const { highlightProperty } = this.inspector.getPanel("ruleview").view;
-    const isHighlighted = highlightProperty(property);
+    const { getApplicableTextProperty } = this.inspector.getPanel(
+      "ruleview"
+    ).view;
+    const textProp = getApplicableTextProperty(property);
 
-    // Only show the tooltip if the property is not highlighted.
-    // TODO: In the future, use an associated ruleId for toggling the tooltip instead of
-    // the Boolean returned from highlightProperty.
-    if (!isHighlighted) {
-      this.rulePreviewTooltip.show(target);
+    // Only show the tooltip if there is a corresponding TextProperty
+    if (textProp) {
+      // Get the specific declaration from the CSS rule's text content
+      const rule = textProp.rule.stringifyRule([textProp]);
+      this.rulePreviewTooltip.show(target, rule);
     }
   },
 
@@ -332,6 +339,12 @@ BoxModel.prototype = {
    *         The name of the property.
    */
   onShowBoxModelEditor(element, event, property) {
+    if (HIGHLIGHT_RULE_PREF && event.shiftKey) {
+      const { highlightProperty } = this.inspector.getPanel("ruleview").view;
+      highlightProperty(property);
+      return;
+    }
+
     const session = new EditingSession({
       inspector: this.inspector,
       doc: this.document,

devtools/client/inspector/boxmodel/components/BoxModelEditable.js

@@ -51,7 +51,7 @@ class BoxModelEditable extends PureComponent {
   onMouseOver(event) {
     const { onShowRulePreviewTooltip, property } = this.props;
 
-    if (event.shiftKey && HIGHLIGHT_RULE_PREF) {
+    if (HIGHLIGHT_RULE_PREF) {
       onShowRulePreviewTooltip(event.target, property);
     }
   }

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

@@ -21,7 +21,7 @@ disabled=too many intermittent failures (bug 1009322)
 [browser_boxmodel_editablemodel_pseudo.js]
 [browser_boxmodel_editablemodel_stylerules.js]
 [browser_boxmodel_guides.js]
-[browser_boxmodel_jump-to-rule-on-hover.js]
+[browser_boxmodel_jump-to-rule-on-click.js]
 [browser_boxmodel_layout-accordion-state.js]
 [browser_boxmodel_navigation.js]
 [browser_boxmodel_offsetparent.js]
@@ -29,7 +29,7 @@ disabled=too many intermittent failures (bug 1009322)
 [browser_boxmodel_properties.js]
 [browser_boxmodel_pseudo-element.js]
 [browser_boxmodel_rotate-labels-on-sides.js]
-[browser_boxmodel_show-tooltip-for-unassociated-rule.js]
+[browser_boxmodel_show-tooltip-for-rule.js]
 [browser_boxmodel_sync.js]
 [browser_boxmodel_tooltips.js]
 skip-if = true # Bug 1336198

devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel.js

@@ -50,7 +50,11 @@ async function testEditingMargins(inspector, boxmodel, testActor) {
   );
   is(span.textContent, 5, "Should have the right value in the box model.");
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, boxmodel.document.defaultView);
+  EventUtils.sendMouseEvent(
+    { type: "click" },
+    span,
+    boxmodel.document.defaultView
+  );
   const editor = boxmodel.document.querySelector(
     ".styleinspector-propertyeditor"
   );
@@ -95,7 +99,11 @@ async function testKeyBindings(inspector, boxmodel, testActor) {
   );
   is(span.textContent, 10, "Should have the right value in the box model.");
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, boxmodel.document.defaultView);
+  EventUtils.sendMouseEvent(
+    { type: "click" },
+    span,
+    boxmodel.document.defaultView
+  );
   const editor = boxmodel.document.querySelector(
     ".styleinspector-propertyeditor"
   );
@@ -163,7 +171,11 @@ async function testEscapeToUndo(inspector, boxmodel, testActor) {
   );
   is(span.textContent, 20, "Should have the right value in the box model.");
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, boxmodel.document.defaultView);
+  EventUtils.sendMouseEvent(
+    { type: "click" },
+    span,
+    boxmodel.document.defaultView
+  );
   const editor = boxmodel.document.querySelector(
     ".styleinspector-propertyeditor"
   );
@@ -204,7 +216,11 @@ async function testDeletingValue(inspector, boxmodel, testActor) {
   );
   is(span.textContent, 15, "Should have the right value in the box model.");
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, boxmodel.document.defaultView);
+  EventUtils.sendMouseEvent(
+    { type: "click" },
+    span,
+    boxmodel.document.defaultView
+  );
   const editor = boxmodel.document.querySelector(
     ".styleinspector-propertyeditor"
   );
@@ -241,7 +257,11 @@ async function testRefocusingOnClick(inspector, boxmodel, testActor) {
   );
   is(span.textContent, 1, "Should have the right value in the box model.");
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, boxmodel.document.defaultView);
+  EventUtils.sendMouseEvent(
+    { type: "click" },
+    span,
+    boxmodel.document.defaultView
+  );
   const editor = boxmodel.document.querySelector(
     ".styleinspector-propertyeditor"
   );

devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel_pseudo.js

@@ -68,7 +68,7 @@ async function checkValueInBoxModel(selector, expectedValue, doc) {
     "Should have the right value in the box model."
   );
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, doc.defaultView);
+  EventUtils.sendMouseEvent({ type: "click" }, span, doc.defaultView);
   const editor = doc.querySelector(".styleinspector-propertyeditor");
   ok(editor, "Should have opened the editor.");
   is(editor.value, expectedValue, "Should have the right value in the editor.");

devtools/client/inspector/boxmodel/test/browser_boxmodel_editablemodel_stylerules.js

@@ -39,7 +39,11 @@ async function testUnits(inspector, boxmodel, testActor) {
   );
   is(span.textContent, 3, "Should have the right value in the box model.");
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, boxmodel.document.defaultView);
+  EventUtils.sendMouseEvent(
+    { type: "click" },
+    span,
+    boxmodel.document.defaultView
+  );
   const editor = boxmodel.document.querySelector(
     ".styleinspector-propertyeditor"
   );
@@ -92,7 +96,11 @@ async function testValueComesFromStyleRule(inspector, boxmodel, testActor) {
   );
   is(span.textContent, 16, "Should have the right value in the box model.");
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, boxmodel.document.defaultView);
+  EventUtils.sendMouseEvent(
+    { type: "click" },
+    span,
+    boxmodel.document.defaultView
+  );
   const editor = boxmodel.document.querySelector(
     ".styleinspector-propertyeditor"
   );
@@ -134,7 +142,11 @@ async function testShorthandsAreParsed(inspector, boxmodel, testActor) {
   );
   is(span.textContent, 32, "Should have the right value in the box model.");
 
-  EventUtils.synthesizeMouseAtCenter(span, {}, boxmodel.document.defaultView);
+  EventUtils.sendMouseEvent(
+    { type: "click" },
+    span,
+    boxmodel.document.defaultView
+  );
   const editor = boxmodel.document.querySelector(
     ".styleinspector-propertyeditor"
   );

devtools/client/inspector/boxmodel/test/browser_boxmodel_jump-to-rule-on-click.js

@@ -3,15 +3,15 @@
 
 "use strict";
 
-// Test that hovering over a box model value will jump to its source CSS rule in the
-// rules view when the shift key is pressed.
+// Test that holding Shift while clicking a box model value will jump to its source CSS rule
+// in the rules view.
 
 const TEST_URI = `<style>
-    #box {
-      margin: 5px;
-    }
-  </style>
-  <div id="box"></div>`;
+     #box {
+       margin: 5px;
+     }
+   </style>
+   <div id="box"></div>`;
 
 add_task(async function() {
   await pushPref("devtools.layout.boxmodel.highlightProperty", true);
@@ -20,25 +20,30 @@ add_task(async function() {
   await selectNode("#box", inspector);
 
   info(
-    "Test that hovering over margin-top value highlights the property in rules view."
+    "Test that Shift-clicking the margin-top value highlights the property in rules view."
   );
   const ruleView = await inspector.getPanel("ruleview").view;
   const el = boxmodel.document.querySelector(
     ".boxmodel-margin.boxmodel-top > span"
   );
 
-  info("Wait for mouse to hover over margin-top element.");
+  info("Wait for mouse to click the margin-top element.");
   const onHighlightProperty = ruleView.once("scrolled-to-element");
   EventUtils.synthesizeMouseAtCenter(
     el,
-    { type: "mousemove", shiftKey: true },
+    { shiftKey: true },
     boxmodel.document.defaultView
   );
   await onHighlightProperty;
 
   info("Check that margin-top is visible in the rule view.");
   const { rules, styleWindow } = ruleView;
+  // when a CSS declaration is highlighted and it's a sub-property, it will automatically expand
+  // margin-top is the first expanded property of margin in the TEST_URI
+  // if Rules view implementation changes, this test may fail because margin-top may no longer be the
+  // first sub-property
   const marginTop = rules[1].textProps[0].computed[0];
+  // checks to see if expanded sub-property shows up in the viewport
   ok(
     isInViewport(marginTop.element, styleWindow),
     "margin-top is visible in the rule view."

devtools/client/inspector/boxmodel/test/browser_boxmodel_show-tooltip-for-rule.js

@@ -0,0 +1,74 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that hovering over a box model value shows tooltip with the source CSS rule
+// if there is an associated rule.
+
+const TEST_URI = `<style>
+      #box { margin-left: 1px; padding: 2px 3px; border: solid 4em; }
+    </style>
+    <div id="box"></div>`;
+
+add_task(async function() {
+  await pushPref("devtools.layout.boxmodel.highlightProperty", true);
+  await addTab("data:text/html," + encodeURIComponent(TEST_URI));
+  const { inspector, boxmodel } = await openLayoutView();
+  await selectNode("#box", inspector);
+
+  // Test value with associated rule for margin
+  info(
+    "Test that hovering over margin-left shows tooltip containing the CSS declaration for margin."
+  );
+  await testBoxmodelTooltip(
+    boxmodel,
+    ".boxmodel-margin.boxmodel-left > span",
+    "margin-left: 1px"
+  );
+
+  // Test value with associated rule for padding
+  info(
+    "Test that hovering over padding-left shows tooltip containing the CSS declaration for padding."
+  );
+  await testBoxmodelTooltip(
+    boxmodel,
+    ".boxmodel-padding.boxmodel-left > span",
+    "padding: 2px 3px"
+  );
+
+  // Test value with associated rule for border
+  info(
+    "Test that hovering over border-left shows tooltip containing the CSS declaration for border."
+  );
+  await testBoxmodelTooltip(
+    boxmodel,
+    ".boxmodel-border.boxmodel-left > span",
+    "border: solid 4em"
+  );
+});
+
+async function testBoxmodelTooltip(boxmodel, valueSelector, expectedTooltip) {
+  // const { rulePreviewTooltip } = boxmodel;
+  const el = boxmodel.document.querySelector(valueSelector);
+  info("Wait for mouse to hover over value element");
+  const onRulePreviewTooltipShown = boxmodel.rulePreviewTooltip._tooltip.once(
+    "shown",
+    () => {
+      info("Check that tooltip is shown.");
+      is(
+        boxmodel.rulePreviewTooltip.message.textContent.includes(
+          expectedTooltip
+        ),
+        true,
+        `Tooltip shows '${expectedTooltip}'`
+      );
+    }
+  );
+  EventUtils.synthesizeMouseAtCenter(
+    el,
+    { type: "mousemove" },
+    boxmodel.document.defaultView
+  );
+  await onRulePreviewTooltipShown;
+}

devtools/client/inspector/boxmodel/test/browser_boxmodel_show-tooltip-for-unassociated-rule.js

@@ -1,50 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
- http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-const L10N = new LocalizationHelper(
-  "devtools/client/locales/inspector.properties"
-);
-
-// Test that hovering over a box model value with no associated rule will show a tooltip
-// saying: "No associated rule".
-
-const TEST_URI = `<style>
-    #box {}
-  </style>
-  <div id="box"></div>`;
-
-add_task(async function() {
-  await pushPref("devtools.layout.boxmodel.highlightProperty", true);
-  await addTab("data:text/html," + encodeURIComponent(TEST_URI));
-  const { inspector, boxmodel } = await openLayoutView();
-  const { rulePreviewTooltip } = boxmodel;
-  await selectNode("#box", inspector);
-
-  info(
-    "Test that hovering over margin-top shows tooltip showing 'No associated rule'."
-  );
-  const el = boxmodel.document.querySelector(
-    ".boxmodel-margin.boxmodel-top > span"
-  );
-
-  info("Wait for mouse to hover over margin-top element.");
-  const onRulePreviewTooltipShown = rulePreviewTooltip._tooltip.once(
-    "shown",
-    () => {
-      ok(true, "Tooltip shown.");
-      is(
-        rulePreviewTooltip.message.textContent,
-        L10N.getStr("rulePreviewTooltip.noAssociatedRule"),
-        `Tooltip shows ${L10N.getStr("rulePreviewTooltip.noAssociatedRule")}`
-      );
-    }
-  );
-  EventUtils.synthesizeMouseAtCenter(
-    el,
-    { type: "mousemove", shiftKey: true },
-    boxmodel.document.defaultView
-  );
-  await onRulePreviewTooltipShown;
-});

devtools/client/inspector/rules/models/rule.js

@@ -855,13 +855,19 @@ class Rule {
 
   /**
    * Return a string representation of the rule.
+   *
+   * @param {TextProperty} filterTextProps
+   *        An optional array of TextProperty instances to filter by.
    */
-  stringifyRule() {
+  stringifyRule(filterTextProps = []) {
     const selectorText = this.selectorText;
     let cssText = "";
     const terminator = Services.appinfo.OS === "WINNT" ? "\r\n" : "\n";
 
     for (const textProp of this.textProps) {
+      if (filterTextProps.length && !filterTextProps.includes(textProp)) {
+        continue;
+      }
       if (!textProp.invisible) {
         cssText += "\t" + textProp.stringifyProperty() + terminator;
       }

devtools/client/inspector/rules/rules.js

@@ -161,6 +161,7 @@ function CssRuleView(inspector, document, store) {
   this._onTogglePrintSimulation = this._onTogglePrintSimulation.bind(this);
   this.highlightElementRule = this.highlightElementRule.bind(this);
   this.highlightProperty = this.highlightProperty.bind(this);
+  this.getApplicableTextProperty = this.getApplicableTextProperty.bind(this);
   this.refreshPanel = this.refreshPanel.bind(this);
 
   const doc = this.styleDocument;
@@ -1747,46 +1748,74 @@ CssRuleView.prototype = {
    *
    * @param  {String} name
    *         The property name to scroll to and highlight.
-   * @return {Boolean} true if the TextProperty name is found, and false otherwise.
+   * @return {Boolean} true if the TextProperty name was highlighted, and false otherwise.
    */
   highlightProperty: function(name) {
+    const textProp = this.getApplicableTextProperty(name);
+    if (!textProp) {
+      return false;
+    }
+
+    const rule = textProp.rule;
+    const {
+      editor: { selectorText },
+    } = rule;
+    let scrollBehavior = "smooth";
+
+    // If using 2-Pane mode, then switch to the Rules tab first.
+    if (!this.inspector.is3PaneModeEnabled) {
+      this.inspector.sidebar.select("ruleview");
+    }
+
+    // If the property is being applied by a pseudo element rule, expand the pseudo
+    // element list container.
+    if (rule.pseudoElement.length && !this.showPseudoElements) {
+      // Set the scroll behavior to "auto" to avoid timing issues between toggling
+      // the pseudo element container and scrolling smoothly to the rule.
+      scrollBehavior = "auto";
+      this._togglePseudoElementRuleContainer();
+    }
+
+    // Assume we scroll to the container of the CSS declaration (aka TextProperty)
+    let element = textProp.editor.element;
+    // If the returned applied TextProperty's value is not the same as the input property, then it
+    // is likely a shorthand notation and one of its sub-properties applies.
+    if (name !== textProp.name) {
+      const subProperty = textProp.computed.find(
+        subProp => subProp.name === name
+      );
+
+      // Expand the computed list.
+      textProp.editor.expandForFilter();
+      // Ensure we scroll to the container of the sub-property
+      element = subProperty.element;
+    }
+
+    // Scroll to the top of the property's rule so that both the property and its
+    // rule are visible.
+    this._scrollToElement(selectorText, element, scrollBehavior);
+    this._flashElement(element);
+
+    return true;
+  },
+
+  /**
+   * Finds the specified CSS property name in the rule view and returns the corresponding
+   * TextProperty instance for that CSS property which currently applies (overwritten and
+   * disabled ones are skipped).
+   *
+   * @param  {String} name
+   *         The property name to find.
+   * @return {TextProperty|null} textProp if the TextProperty name was found, and null otherwise.
+   */
+  getApplicableTextProperty: function(name) {
     for (const rule of this.rules) {
       for (const textProp of rule.textProps) {
         if (textProp.overridden || textProp.invisible || !textProp.enabled) {
           continue;
         }
-
-        const {
-          editor: { selectorText },
-        } = rule;
-        let scrollBehavior = "smooth";
-
-        // First, search for a matching authored property.
         if (textProp.name === name) {
-          // If using 2-Pane mode, then switch to the Rules tab first.
-          if (!this.inspector.is3PaneModeEnabled) {
-            this.inspector.sidebar.select("ruleview");
-          }
-
-          // If the property is being applied by a pseudo element rule, expand the pseudo
-          // element list container.
-          if (rule.pseudoElement.length && !this.showPseudoElements) {
-            // Set the scroll behavior to "auto" to avoid timing issues between toggling
-            // the pseudo element container and scrolling smoothly to the rule.
-            scrollBehavior = "auto";
-            this._togglePseudoElementRuleContainer();
-          }
-
-          // Scroll to the top of the property's rule so that both the property and its
-          // rule are visible.
-          this._scrollToElement(
-            selectorText,
-            textProp.editor.element,
-            scrollBehavior
-          );
-          this._flashElement(textProp.editor.element);
-
-          return true;
+          return textProp;
         }
 
         // If there is no matching property, then look in computed properties.
@@ -1796,35 +1825,12 @@ CssRuleView.prototype = {
           }
 
           if (computed.name === name) {
-            if (!this.inspector.is3PaneModeEnabled) {
-              this.inspector.sidebar.select("ruleview");
-            }
-
-            if (
-              textProp.rule.pseudoElement.length &&
-              !this.showPseudoElements
-            ) {
-              scrollBehavior = "auto";
-              this._togglePseudoElementRuleContainer();
-            }
-
-            // Expand the computed list.
-            textProp.editor.expandForFilter();
-
-            this._scrollToElement(
-              selectorText,
-              computed.element,
-              scrollBehavior
-            );
-            this._flashElement(computed.element);
-
-            return true;
+            return textProp;
           }
         }
       }
     }
-
-    return false;
+    return null;
   },
 };
 

devtools/client/locales/en-US/inspector.properties

@@ -503,9 +503,9 @@ markupView.scrollableBadge.label=scroll
 # when hovering over badges next to scrollable elements in the inspector.
 markupView.scrollableBadge.tooltip=This element has scrollable overflow.
 
-# LOCALIZATION NOTE (rulePreviewTooltip.noAssociatedRule): This is the text displayed inside
-# the RulePreviewTooltip when a rule cannot be found for a CSS property declaration.
-rulePreviewTooltip.noAssociatedRule=No associated rule
+# LOCALIZATION NOTE (rulePreviewTooltip.jumpToRule): This is the text displayed inside
+# the RulePreviewTooltip footer, describing the keyboard shortcut to jump to a rule.
+rulePreviewTooltip.jumpToRuleShortcut=Hold Shift and click to jump to rule
 
 # LOCALIZATION NOTE (colorPickerTooltip.contrastAgainstBgTitle): A title text for the
 # contrast ratio value description that labels the background the color contrast ratio is calculated

devtools/client/shared/widgets/tooltip/RulePreviewTooltip.js

@@ -16,8 +16,6 @@ const L10N = new LocalizationHelper(
 
 /**
  * Tooltip displayed for when a CSS property is selected/highlighted.
- * TODO: For now, the tooltip content only shows "No Associated Rule". In Bug 1528288,
- * we will be implementing content for showing the source CSS rule.
  */
 class RulePreviewTooltip {
   constructor(doc) {
@@ -36,12 +34,12 @@ class RulePreviewTooltip {
 
     this.message = doc.createElementNS(XHTML_NS, "span");
     this.message.className = "rule-preview-tooltip-message";
-    this.message.textContent = L10N.getStr(
-      "rulePreviewTooltip.noAssociatedRule"
-    );
-    this.container.appendChild(this.message);
 
-    // TODO: Implement structure for showing the source CSS rule.
+    this.footer = doc.createElementNS(XHTML_NS, "span");
+    this.footer.className = "rule-preview-tooltip-source-rule-footer";
+    this.footer.textContent = L10N.getStr(
+      "rulePreviewTooltip.jumpToRuleShortcut"
+    );
 
     this._tooltip.panel.innerHTML = "";
     this._tooltip.panel.appendChild(this.container);
@@ -49,12 +47,19 @@ class RulePreviewTooltip {
   }
 
   /**
-   * Shows the tooltip on a given element.
+   * Shows the tooltip on a given element. The tooltip will display
+   * the source CSS rule.
    *
    * @param  {Element} element
    *         The target element to show the tooltip with.
+   * @param  {String} ruleText
+   *         The source CSS rule to show in the tooltip.
    */
-  show(element) {
+  show(element, ruleText) {
+    this.message.textContent = ruleText;
+    this.container.appendChild(this.message);
+    this.container.appendChild(this.footer);
+
     element.addEventListener("mouseout", () => this._tooltip.hide());
     this._tooltip.show(element);
   }
@@ -63,6 +68,7 @@ class RulePreviewTooltip {
     this._tooltip.destroy();
     this.container = null;
     this.message = null;
+    this.footer = null;
   }
 }