Bug 1851978 - [devtools] Pass colorUnit to CssColor.toString. r=ochameau,devtools-reviewers.

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

devtools/client/inspector/inspector.js

@@ -10,6 +10,7 @@ const { executeSoon } = require("resource://devtools/shared/DevToolsUtils.js");
 const { Toolbox } = require("resource://devtools/client/framework/toolbox.js");
 const createStore = require("resource://devtools/client/inspector/store.js");
 const InspectorStyleChangeTracker = require("resource://devtools/client/inspector/shared/style-change-tracker.js");
+const { PrefObserver } = require("resource://devtools/client/shared/prefs.js");
 
 // Use privileged promise in panel documents to prevent having them to freeze
 // during toolbox destruction. See bug 1402779.
@@ -103,6 +104,7 @@ const THREE_PANE_CHROME_ENABLED_PREF =
 const TELEMETRY_EYEDROPPER_OPENED = "devtools.toolbar.eyedropper.opened";
 const TELEMETRY_SCALAR_NODE_SELECTION_COUNT =
   "devtools.inspector.node_selection_count";
+const DEFAULT_COLOR_UNIT_PREF = "devtools.defaultColorUnit";
 
 /**
  * Represents an open instance of the Inspector for a tab.
@@ -156,6 +158,8 @@ function Inspector(toolbox, commands) {
   this._panels = new Map();
 
   this._clearSearchResultsLabel = this._clearSearchResultsLabel.bind(this);
+  this._handleDefaultColorUnitPrefChange =
+    this._handleDefaultColorUnitPrefChange.bind(this);
   this._handleRejectionIfNotDestroyed =
     this._handleRejectionIfNotDestroyed.bind(this);
   this._onTargetAvailable = this._onTargetAvailable.bind(this);
@@ -185,6 +189,13 @@ function Inspector(toolbox, commands) {
   this.onSidebarToggle = this.onSidebarToggle.bind(this);
   this.onReflowInSelection = this.onReflowInSelection.bind(this);
   this.listenForSearchEvents = this.listenForSearchEvents.bind(this);
+
+  this.prefObserver = new PrefObserver("devtools.");
+  this.prefObserver.on(
+    DEFAULT_COLOR_UNIT_PREF,
+    this._handleDefaultColorUnitPrefChange
+  );
+  this.defaultColorUnit = Services.prefs.getStringPref(DEFAULT_COLOR_UNIT_PREF);
 }
 
 Inspector.prototype = {
@@ -537,6 +548,12 @@ Inspector.prototype = {
   // This value is exposed on Inspector so individual tests can restore it when needed.
   HIGHLIGHTER_AUTOHIDE_TIMER: flags.testing ? 0 : 1000,
 
+  _handleDefaultColorUnitPrefChange() {
+    this.defaultColorUnit = Services.prefs.getStringPref(
+      DEFAULT_COLOR_UNIT_PREF
+    );
+  },
+
   /**
    * Handle promise rejections for various asynchronous actions, and only log errors if
    * the inspector panel still exists.
@@ -1719,6 +1736,12 @@ Inspector.prototype = {
 
     this.teardownToolbar();
 
+    this.prefObserver.on(
+      DEFAULT_COLOR_UNIT_PREF,
+      this._handleDefaultColorUnitPrefChange
+    );
+    this.prefObserver.destroy();
+
     this.breadcrumbs.destroy();
     this.styleChangeTracker.destroy();
     this.searchboxShortcuts.destroy();

devtools/client/inspector/rules/views/text-property-editor.js

@@ -555,7 +555,8 @@ TextPropertyEditor.prototype = {
       shapeSwatchClass: SHAPE_SWATCH_CLASS,
       // Only ask the parser to convert colors to the default color type specified by the
       // user if the property hasn't been changed yet.
-      defaultColorType: !propDirty,
+      useDefaultColorUnit: !propDirty,
+      defaultColorUnit: this.ruleView.inspector.defaultColorUnit,
       urlClass: "theme-link",
       fontFamilyClass: FONT_FAMILY_CLASS,
       baseURI: this.sheetHref,

devtools/client/shared/output-parser.js

@@ -1570,14 +1570,15 @@ class OutputParser {
         container.appendChild(swatch);
       }
 
-      if (!options.defaultColorType) {
+      let colorUnit = options.defaultColorUnit;
+      if (!options.useDefaultColorUnit) {
         // If we're not being asked to convert the color to the default color type
         // specified by the user, then force the CssColor instance to be set to the type
         // of the current color.
         // Not having a type means that the default color type will be automatically used.
-        colorObj.colorUnit = colorUtils.classifyColor(color);
+        colorUnit = colorUtils.classifyColor(color);
       }
-      color = colorObj.toString();
+      color = colorObj.toString(colorUnit);
       container.dataset.color = color;
 
       // Next we create the markup to show the value of the property.
@@ -1906,35 +1907,33 @@ class OutputParser {
    *
    * @param  {Object} overrides
    *         The option values to override e.g. #mergeOptions({colors: false})
-   *
+   * @param {Boolean} overrides.useDefaultColorUnit: Convert colors to the default type
-   *         Valid options are:
+   *                                                 selected in the options panel.
-   *           - defaultColorType: true // Convert colors to the default type
+   * @param {String} overrides.angleClass: The class to use for the angle value that follows
-   *                                    // selected in the options panel.
+   *                                       the swatch.
-   *           - angleClass: ""         // The class to use for the angle value
+   * @param {String} overrides.angleSwatchClass: The class to use for angle swatches.
+   * @param {} overrides.bezierClass: ""        // The class to use for the bezier value
    *                                    // that follows the swatch.
-   *           - angleSwatchClass: ""   // The class to use for angle swatches.
+   * @param {} overrides.bezierSwatchClass: ""  // The class to use for bezier swatches.
-   *           - bezierClass: ""        // The class to use for the bezier value
+   * @param {} overrides.colorClass: ""         // The class to use for the color value
    *                                    // that follows the swatch.
-   *           - bezierSwatchClass: ""  // The class to use for bezier swatches.
+   * @param {} overrides.colorSwatchClass: ""   // The class to use for color swatches.
-   *           - colorClass: ""         // The class to use for the color value
+   * @param {} overrides.filterSwatch: false    // A special case for parsing a
-   *                                    // that follows the swatch.
-   *           - colorSwatchClass: ""   // The class to use for color swatches.
-   *           - filterSwatch: false    // A special case for parsing a
    *                                    // "filter" property, causing the
    *                                    // parser to skip the call to
    *                                    // #wrapFilter.  Used only for
    *                                    // previewing with the filter swatch.
-   *           - flexClass: ""          // The class to use for the flex icon.
+   * @param {} overrides.flexClass: ""          // The class to use for the flex icon.
-   *           - gridClass: ""          // The class to use for the grid icon.
+   * @param {} overrides.gridClass: ""          // The class to use for the grid icon.
-   *           - shapeClass: ""         // The class to use for the shape value
+   * @param {} overrides.shapeClass: ""         // The class to use for the shape value
    *                                    // that follows the swatch.
-   *           - shapeSwatchClass: ""   // The class to use for the shape swatch.
+   * @param {} overrides.shapeSwatchClass: ""   // The class to use for the shape swatch.
-   *           - supportsColor: false   // Does the CSS property support colors?
+   * @param {} overrides.supportsColor: false   // Does the CSS property support colors?
-   *           - urlClass: ""           // The class to be used for url() links.
+   * @param {} overrides.urlClass: ""           // The class to be used for url() links.
-   *           - fontFamilyClass: ""    // The class to be used for font families.
+   * @param {} overrides.fontFamilyClass: ""    // The class to be used for font families.
-   *           - baseURI: undefined     // A string used to resolve
+   * @param {} overrides.baseURI: undefined     // A string used to resolve
    *                                    // relative links.
-   *           - getVariableValue       // A function taking a single
+   * @param {} overrides.getVariableValue       // A function taking a single
    *                                    // argument, the name of a variable.
    *                                    // This should return the variable's
    *                                    // value, if it is in use; or null.
@@ -1947,7 +1946,8 @@ class OutputParser {
    */
   #mergeOptions(overrides) {
     const defaults = {
-      defaultColorType: true,
+      useDefaultColorUnit: true,
+      defaultColorUnit: "authored",
       angleClass: "",
       angleSwatchClass: "",
       bezierClass: "",

devtools/client/shared/test/browser_css_color.js

@@ -46,17 +46,11 @@ function testColorUtils(canvas) {
 }
 
 function testToString(color, name, hex, hsl, rgb) {
-  color.colorUnit = colorUtils.CssColor.COLORUNIT.name;
+  const { COLORUNIT } = colorUtils.CssColor;
-  is(color.toString(), name, "toString() with authored type");
+  is(color.toString(COLORUNIT.name), name, "toString() with authored type");
-
+  is(color.toString(COLORUNIT.hex), hex, "toString() with hex type");
-  color.colorUnit = colorUtils.CssColor.COLORUNIT.hex;
+  is(color.toString(COLORUNIT.hsl), hsl, "toString() with hsl type");
-  is(color.toString(), hex, "toString() with hex type");
+  is(color.toString(COLORUNIT.rgb), rgb, "toString() with rgb type");
-
-  color.colorUnit = colorUtils.CssColor.COLORUNIT.hsl;
-  is(color.toString(), hsl, "toString() with hsl type");
-
-  color.colorUnit = colorUtils.CssColor.COLORUNIT.rgb;
-  is(color.toString(), rgb, "toString() with rgb type");
 }
 
 function testColorMatch(name, hex, hsl, rgb, rgba, canvas) {

devtools/client/shared/test/xpcshell/test_cssColor-01.js

@@ -30,14 +30,6 @@ function run_test() {
     const result = colorUtils.classifyColor(test.input);
     equal(result, test.output, "test classifyColor(" + test.input + ")");
 
-    const obj = new colorUtils.CssColor("purple");
-    obj.setAuthoredUnitFromColor(test.input);
-    equal(
-      obj.colorUnit,
-      test.output,
-      "test setAuthoredUnitFromColor(" + test.input + ")"
-    );
-
     ok(
       InspectorUtils.colorToRGBA(test.input) !== null,
       "'" + test.input + "' is a color"
@@ -55,12 +47,9 @@ function run_test() {
 
   // Regression test for bug 1303826.
   const black = new colorUtils.CssColor("#000");
-  black.colorUnit = "name";
+  equal(black.toString("name"), "black", "test non-upper-case color cycling");
-  equal(black.toString(), "black", "test non-upper-case color cycling");
 
   const upper = new colorUtils.CssColor("BLACK");
-  upper.colorUnit = "hex";
+  equal(upper.toString("hex"), "#000", "test upper-case color cycling");
-  equal(upper.toString(), "#000", "test upper-case color cycling");
+  equal(upper.toString("name"), "BLACK", "test upper-case color preservation");
-  upper.colorUnit = "name";
-  equal(upper.toString(), "BLACK", "test upper-case color preservation");
 }

devtools/client/shared/test/xpcshell/test_cssColor-02.js

@@ -41,12 +41,10 @@ function run_test() {
  */
 function runCycle(value, times) {
   let color = new colorUtils.CssColor(value);
-  //console.log("color", value, color.toString(), color);
+  const colorUnit = colorUtils.classifyColor(value);
   for (let i = 0; i < times; i++) {
-    color.nextColorUnit();
+    const newColor = color.nextColorUnit();
-    //console.log("color.nextColorUnit", color.toString(), color);
+    color = new colorUtils.CssColor(newColor);
-    color = new colorUtils.CssColor(color.toString());
-    //console.log("new color", color.toString(), color);
   }
-  return color.toString() === value;
+  return color.toString(colorUnit) === value;
 }

devtools/client/shared/test/xpcshell/test_cssColor-8-digit-hex.js

@@ -14,9 +14,7 @@ const EIGHT_CHARACTER_HEX = "#fefefef0";
 // eslint-disable-next-line
 function run_test() {
   const cssColor = new colorUtils.CssColor(EIGHT_CHARACTER_HEX);
-  cssColor.colorUnit = colorUtils.CssColor.COLORUNIT.hex;
+  const color = cssColor.toString(colorUtils.CssColor.COLORUNIT.hex);
-
-  const color = cssColor.toString();
 
   equal(color, EIGHT_CHARACTER_HEX, "alpha value is correct");
 }

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

@@ -327,9 +327,15 @@ class SwatchColorPickerTooltip extends SwatchBasedEditorTooltip {
   }
 
   _toDefaultType(color) {
+    let unit = this.inspector.defaultColorUnit;
+    let forceUppercase = false;
+    if (unit === colorUtils.CssColor.COLORUNIT.authored) {
+      unit = colorUtils.classifyColor(this._originalColor);
+      forceUppercase = colorUtils.colorIsUppercase(this._originalColor);
+    }
+
     const colorObj = new colorUtils.CssColor(color);
-    colorObj.setAuthoredUnitFromColor(this._originalColor);
+    return colorObj.toString(unit, forceUppercase);
-    return colorObj.toString();
   }
 
   /**

devtools/shared/css/color.js

@@ -5,7 +5,6 @@
 "use strict";
 
 const COLOR_UNIT_PREF = "devtools.defaultColorUnit";
-
 const SPECIALVALUES = new Set([
   "currentcolor",
   "initial",
@@ -62,7 +61,6 @@ class CssColor {
     // returned when needed.
     this.lowerCased = colorValue.toLowerCase();
     this.authored = colorValue;
-    this.#setColorUnitUppercase(colorValue);
   }
 
   /**
@@ -77,51 +75,12 @@ class CssColor {
     hwb: "hwb",
   };
 
-  #colorUnit = null;
-  #colorUnitUppercase = false;
-
   // The value as-authored.
   authored = null;
   // A lower-cased copy of |authored|.
   lowerCased = null;
 
-  #setColorUnitUppercase(color) {
+  #currentFormat;
-    // Specifically exclude the case where the color is
-    // case-insensitive.  This makes it so that "#000" isn't
-    // considered "upper case" for the purposes of color cycling.
-    this.#colorUnitUppercase =
-      color === color.toUpperCase() && color !== color.toLowerCase();
-  }
-
-  get colorUnit() {
-    if (this.#colorUnit === null) {
-      const defaultUnit = Services.prefs.getCharPref(COLOR_UNIT_PREF);
-      this.#colorUnit = CssColor.COLORUNIT[defaultUnit];
-      this.#setColorUnitUppercase(this.authored);
-    }
-    return this.#colorUnit;
-  }
-
-  set colorUnit(unit) {
-    this.#colorUnit = unit;
-  }
-
-  /**
-   * If the current color unit pref is "authored", then set the
-   * default color unit from the given color.  Otherwise, leave the
-   * color unit untouched.
-   *
-   * @param {String} color The color to use
-   */
-  setAuthoredUnitFromColor(color) {
-    if (
-      Services.prefs.getCharPref(COLOR_UNIT_PREF) ===
-      CssColor.COLORUNIT.authored
-    ) {
-      this.#colorUnit = classifyColor(color);
-      this.#setColorUnitUppercase(color);
-    }
-  }
 
   get hasAlpha() {
     if (!this.valid) {
@@ -384,29 +343,42 @@ class CssColor {
     // Put "name" at the end as that provides a hex value if there's
     // no name for the color.
     let formats = ["hex", "hsl", "rgb", "hwb", "name"];
-    const currentFormat = classifyColor(this.toString());
+
+    let currentFormat = this.#currentFormat;
+    // If we don't have determined the current format yet
+    if (!currentFormat) {
+      // If the pref value is COLORUNIT.authored, get the actual unit from the authored color,
+      // otherwise use the pref value.
+      const defaultFormat = Services.prefs.getCharPref(COLOR_UNIT_PREF);
+      currentFormat =
+        defaultFormat === CssColor.COLORUNIT.authored
+          ? classifyColor(this.authored)
+          : defaultFormat;
+    }
     const putOnEnd = formats.splice(0, formats.indexOf(currentFormat));
     formats = [...formats, ...putOnEnd];
 
     const currentDisplayedColor = this[formats[0]];
 
+    let colorUnit;
     for (const format of formats) {
       if (this[format].toLowerCase() !== currentDisplayedColor.toLowerCase()) {
-        this.colorUnit = CssColor.COLORUNIT[format];
+        colorUnit = CssColor.COLORUNIT[format];
         break;
       }
     }
 
-    return this.toString();
+    this.#currentFormat = colorUnit;
+    return this.toString(colorUnit);
   }
 
   /**
    * Return a string representing a color of type defined in COLOR_UNIT_PREF.
    */
-  toString() {
+  toString(colorUnit, forceUppercase) {
     let color;
 
-    switch (this.colorUnit) {
+    switch (colorUnit) {
       case CssColor.COLORUNIT.authored:
         color = this.authored;
         break;
@@ -430,8 +402,9 @@ class CssColor {
     }
 
     if (
-      this.#colorUnitUppercase &&
+      forceUppercase ||
-      this.colorUnit != CssColor.COLORUNIT.authored
+      (colorUnit != CssColor.COLORUNIT.authored &&
+        colorIsUppercase(this.authored))
     ) {
       color = color.toUpperCase();
     }
@@ -627,7 +600,7 @@ function roundTo(number, digits) {
 
 /**
  * Given a color, classify its type as one of the possible color
- * units, as known by |CssColor.colorUnit|.
+ * units, as known by |CssColor.COLORUNIT|.
  *
  * @param  {String} value
  *         The color, in any form accepted by CSS.
@@ -770,6 +743,13 @@ function calculateContrastRatio(backgroundColor, textColor) {
   return ratio > 1.0 ? ratio : 1 / ratio;
 }
 
+function colorIsUppercase(color) {
+  // Specifically exclude the case where the color is
+  // case-insensitive.  This makes it so that "#000" isn't
+  // considered "upper case" for the purposes of color cycling.
+  return color === color.toUpperCase() && color !== color.toLowerCase();
+}
+
 module.exports.colorUtils = {
   CssColor,
   rgbToHsl,
@@ -780,4 +760,5 @@ module.exports.colorUtils = {
   calculateDeltaE,
   calculateLuminance,
   blendColors,
+  colorIsUppercase,
 };