Bug 1594402 - Display a color swatch for CSS variables in CSS autocomplete r=ladybenko,rcaliman

Depends on D68963 Differential Revision: https://phabricator.services.mozilla.com/D61063
2020-04-29 10:03:31 +00:00 · 2020-04-29 10:03:31 +00:00 · d13658bff7
--- a/devtools/client/inspector/rules/test/browser.ini
+++ b/devtools/client/inspector/rules/test/browser.ini
@ -93,6 +93,7 @@ skip-if = !debug && (os == 'linux' && bits == 64 && os_version == '18.04') || (o
 [browser_rules_colorpicker-release-outside-frame.js]
 [browser_rules_colorpicker-revert-on-ESC.js]
 [browser_rules_colorpicker-swatch-displayed.js]
+[browser_rules_colorpicker-works-with-css-vars.js]
 [browser_rules_colorpicker-wrap-focus.js]
 [browser_rules_colorUnit.js]
 [browser_rules_completion-existing-property_01.js]
--- a/devtools/client/inspector/rules/test/browser_rules_colorpicker-appears-on-swatch-click-or-keyboard-activation.js
+++ b/devtools/client/inspector/rules/test/browser_rules_colorpicker-appears-on-swatch-click-or-keyboard-activation.js
@ -24,11 +24,11 @@ add_task(async function() {
  const propertiesToTest = ["color", "background-color", "border"];

  for (const property of propertiesToTest) {
-    info("Testing that the colorpicker appears on swatch click");
+    info(`Test that the colorpicker appears on swatch click for ${property}`);
    await testColorPickerAppearsOnColorSwatchActivation(view, property);

    info(
-      "Testing that swatch is focusable and colorpicker can be activated with a keyboard"
+      `Test that swatch is focusable and colorpicker can be activated with a keyboard for ${property}`
    );
    await testColorPickerAppearsOnColorSwatchActivation(view, property, true);
  }
@ -43,7 +43,7 @@ async function testColorPickerAppearsOnColorSwatchActivation(
  const swatch = value.querySelector(".ruleview-colorswatch");

  const cPicker = view.tooltips.getTooltip("colorPicker");
-  ok(cPicker, "The rule-view has the expected colorPicker property");
+  ok(cPicker, "The rule-view has an expected colorPicker widget");

  const cPickerPanel = cPicker.tooltip.panel;
  ok(cPickerPanel, "The XUL panel for the color picker exists");
@ -65,11 +65,8 @@ async function testColorPickerAppearsOnColorSwatchActivation(
  }
  await onColorPickerReady;

-  ok(true, "The color picker was shown on click of the color swatch");
-  ok(
-    !inplaceEditor(swatch.parentNode),
-    "The inplace editor wasn't shown as a result of the color swatch click"
-  );
+  info("The color picker was displayed");
+  ok(!inplaceEditor(swatch.parentNode), "The inplace editor wasn't displayed");

  await hideTooltipAndWaitForRuleViewChanged(cPicker, view);
 }
--- a/devtools/client/inspector/rules/test/browser_rules_colorpicker-release-outside-frame.js
+++ b/devtools/client/inspector/rules/test/browser_rules_colorpicker-release-outside-frame.js
@ -64,7 +64,7 @@ add_task(async function() {

 async function openColorPickerForSwatch(swatch, view) {
  const cPicker = view.tooltips.getTooltip("colorPicker");
-  ok(cPicker, "The rule-view has the expected colorPicker property");
+  ok(cPicker, "The rule-view has an expected colorPicker widget");

  const cPickerPanel = cPicker.tooltip.panel;
  ok(cPickerPanel, "The XUL panel for the color picker exists");
--- a/devtools/client/inspector/rules/test/browser_rules_colorpicker-works-with-css-vars.js
+++ b/devtools/client/inspector/rules/test/browser_rules_colorpicker-works-with-css-vars.js
@ -0,0 +1,74 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Tests color pickers work with CSS variables.
+
+const TEST_URI = `
+  <style type="text/css">
+    :root {
+      --main-bg-color: coral;
+    }
+    body {
+      color: red;
+      background-color: var(--main-bg-color);
+      border: 1px solid var(--main-bg-color);
+    }
+  </style>
+  Testing the color picker tooltip with CSS variables!
+`;
+
+add_task(async function() {
+  await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
+  const { view } = await openRuleView();
+
+  const propertiesToTest = ["color", "background-color", "border"];
+
+  for (const property of propertiesToTest) {
+    info(`Test that the colorpicker appears on swatch click for ${property}`);
+    await testColorPickerAppearsOnColorSwatchActivation(view, property);
+
+    info(
+      `Test that swatch is focusable and colorpicker can be activated with a keyboard for ${property}`
+    );
+    await testColorPickerAppearsOnColorSwatchActivation(view, property, true);
+  }
+});
+
+async function testColorPickerAppearsOnColorSwatchActivation(
+  view,
+  property,
+  withKeyboard = false
+) {
+  const value = getRuleViewProperty(view, "body", property).valueSpan;
+  const swatch = value.querySelector(".ruleview-colorswatch");
+
+  const cPicker = view.tooltips.getTooltip("colorPicker");
+  ok(cPicker, "The rule-view has an expected colorPicker widget");
+
+  const cPickerPanel = cPicker.tooltip.panel;
+  ok(cPickerPanel, "The XUL panel for the color picker exists");
+
+  const onColorPickerReady = cPicker.once("ready");
+  if (withKeyboard) {
+    // Focus on the property value span
+    const doc = value.ownerDocument;
+    value.focus();
+
+    // Tab to focus on the color swatch
+    EventUtils.sendKey("Tab");
+    is(doc.activeElement, swatch, "Swatch successfully receives focus.");
+
+    // Press enter on the swatch to simulate click and open color picker
+    EventUtils.sendKey("Return");
+  } else {
+    swatch.click();
+  }
+  await onColorPickerReady;
+
+  info("The color picker was displayed");
+  ok(!inplaceEditor(swatch.parentNode), "The inplace editor wasn't displayed");
+
+  await hideTooltipAndWaitForRuleViewChanged(cPicker, view);
+}
--- a/devtools/client/inspector/rules/test/browser_rules_variables_01.js
+++ b/devtools/client/inspector/rules/test/browser_rules_variables_01.js
@ -44,7 +44,7 @@ add_task(async function() {
    "background-color"
  ).valueSpan.querySelector(".ruleview-unmatched-variable");
  const setVar = unsetVar.nextElementSibling;
-  const setVarName = setVar.firstElementChild.firstElementChild;
+  const setVarName = setVar.querySelector(".ruleview-variable");
  is(
    unsetVar.textContent,
    "--not-set",
--- a/devtools/client/inspector/shared/style-inspector-menu.js
+++ b/devtools/client/inspector/shared/style-inspector-menu.js
@ -317,7 +317,7 @@ StyleInspectorMenu.prototype = {
      return false;
    }

-    const colorNode = container.closest("[data-color");
+    const colorNode = container.closest("[data-color]");
    if (!colorNode) {
      return false;
    }
--- a/devtools/client/shared/output-parser.js
+++ b/devtools/client/shared/output-parser.js
@ -211,13 +211,8 @@ OutputParser.prototype = {
        options.isVariableInUse
      ) {
        sawVariable = true;
-        const variableNode = this._parseVariable(
-          token,
-          text,
-          tokenStream,
-          options
-        );
-        functionData.push(variableNode);
+        const { node } = this._parseVariable(token, text, tokenStream, options);
+        functionData.push(node);
      } else if (token.tokenType === "function") {
        ++depth;
      }
@ -252,10 +247,11 @@ OutputParser.prototype = {
   * @param  {Object} options
   *         The options object in use; @see _mergeOptions.
   * @return {Object}
-   *         A node for the variable, with the appropriate text and
-   *         title. Eg. a span with "var(--var1)" as the textContent
-   *         and a title for --var1 like "--var1 = 10" or
-   *         "--var1 is not set".
+   *         - node: A node for the variable, with the appropriate text and
+   *           title. Eg. a span with "var(--var1)" as the textContent
+   *           and a title for --var1 like "--var1 = 10" or
+   *           "--var1 is not set".
+   *         - value: The value for the variable.
   */
  _parseVariable: function(initialToken, text, tokenStream, options) {
    // Handle the "var(".
@ -330,7 +326,7 @@ OutputParser.prototype = {
    }
    variableNode.appendChild(this.doc.createTextNode(")"));

-    return variableNode;
+    return { node: variableNode, value: varValue };
  },

  /**
@ -407,13 +403,24 @@ OutputParser.prototype = {
            }
            ++parenDepth;
          } else if (token.text === "var" && options.isVariableInUse) {
-            const variableNode = this._parseVariable(
+            const { node: variableNode, value } = this._parseVariable(
              token,
              text,
              tokenStream,
              options
            );
-            this.parsed.push(variableNode);
+            if (
+              value &&
+              colorOK() &&
+              colorUtils.isValidCSSColor(value, this.cssColor4)
+            ) {
+              this._appendColor(value, {
+                ...options,
+                variableContainer: variableNode,
+              });
+            } else {
+              this.parsed.push(variableNode);
+            }
          } else {
            const { functionData, sawVariable } = this._parseMatchingParens(
              text,
@ -1513,6 +1520,7 @@ OutputParser.prototype = {
        this.colorSwatches.set(swatch, colorObj);
        swatch.addEventListener("mousedown", this._onColorSwatchMouseDown);
        EventEmitter.decorate(swatch);
+
        container.appendChild(swatch);
      }

@ -1526,15 +1534,27 @@ OutputParser.prototype = {
      color = colorObj.toString();
      container.dataset.color = color;

-      const value = this._createNode(
-        "span",
-        {
-          class: options.colorClass,
-        },
-        color
-      );
+      // Next we create the markup to show the value of the property.
+      if (options.variableContainer) {
+        // If we are creating a color swatch for a CSS variable we simply reuse
+        // the markup created for the variableContainer.
+        if (options.colorClass) {
+          options.variableContainer.classList.add(options.colorClass);
+        }
+        container.appendChild(options.variableContainer);
+      } else {
+        // Otherwise we create a new element with the `color` as textContent.
+        const value = this._createNode(
+          "span",
+          {
+            class: options.colorClass,
+          },
+          color
+        );
+
+        container.appendChild(value);
+      }

-      container.appendChild(value);
      this.parsed.push(container);
    } else {
      this._appendTextNode(color);
--- a/devtools/client/shared/test/browser_outputparser.js
+++ b/devtools/client/shared/test/browser_outputparser.js
@ -515,32 +515,57 @@ function testParseVariable(doc, parser) {
      text: "var(--seen)",
      variables: { "--seen": "chartreuse" },
      expected:
-        '<span>var(<span data-variable="--seen = chartreuse">--seen</span>)' +
+        /* eslint-disable */
+        '<span data-color="chartreuse">' +
+          "<span>var(" +
+            '<span data-variable="--seen = chartreuse">--seen</span>)' +
+          "</span>" +
        "</span>",
+        /* eslint-enable */
    },
    {
      text: "var(--not-seen)",
      variables: {},
      expected:
-        '<span>var(<span class="unmatched-class" ' +
-        'data-variable="--not-seen is not set">--not-seen</span>)</span>',
+        /* eslint-disable */
+        "<span>var(" +
+          '<span class="unmatched-class" data-variable="--not-seen is not set">--not-seen</span>' +
+        ")</span>",
+        /* eslint-enable */
    },
    {
      text: "var(--seen, seagreen)",
      variables: { "--seen": "chartreuse" },
      expected:
-        '<span>var(<span data-variable="--seen = chartreuse">--seen</span>,' +
-        '<span class="unmatched-class"> <span data-color="seagreen"><span>seagreen' +
-        "</span></span></span>)</span>",
+        /* eslint-disable */
+        '<span data-color="chartreuse">' +
+          "<span>var(" +
+            '<span data-variable="--seen = chartreuse">--seen</span>,' +
+            '<span class="unmatched-class"> ' +
+              '<span data-color="seagreen">' +
+                "<span>seagreen</span>" +
+              "</span>" +
+            "</span>)" +
+          "</span>" +
+        "</span>",
+        /* eslint-enable */
    },
    {
      text: "var(--not-seen, var(--seen))",
      variables: { "--seen": "chartreuse" },
      expected:
-        '<span>var(<span class="unmatched-class" ' +
-        'data-variable="--not-seen is not set">--not-seen</span>,<span> <span>var' +
-        '(<span data-variable="--seen = chartreuse">--seen</span>)</span></span>)' +
+        /* eslint-disable */
+        "<span>var(" +
+          '<span class="unmatched-class" data-variable="--not-seen is not set">--not-seen</span>,' +
+          "<span> " +
+            '<span data-color="chartreuse">' +
+              "<span>var(" +
+                '<span data-variable="--seen = chartreuse">--seen</span>)' +
+              "</span>" +
+            "</span>" +
+          "</span>)" +
        "</span>",
+        /* eslint-enable */
    },
  ];