From f4a8ba14efadf325632483c545d820f073862a6e Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Thu, 14 Sep 2023 15:06:22 +0000 Subject: [PATCH] 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 | 23 ++++++ .../rules/views/text-property-editor.js | 3 +- devtools/client/shared/output-parser.js | 50 ++++++------ .../client/shared/test/browser_css_color.js | 16 ++-- .../shared/test/xpcshell/test_cssColor-01.js | 17 +--- .../shared/test/xpcshell/test_cssColor-02.js | 10 +-- .../xpcshell/test_cssColor-8-digit-hex.js | 4 +- .../tooltip/SwatchColorPickerTooltip.js | 10 ++- devtools/shared/css/color.js | 81 +++++++------------ 9 files changed, 102 insertions(+), 112 deletions(-) diff --git a/devtools/client/inspector/inspector.js b/devtools/client/inspector/inspector.js index df32154f3a6a..308e0d3ea0ed 100644 --- a/devtools/client/inspector/inspector.js +++ b/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(); diff --git a/devtools/client/inspector/rules/views/text-property-editor.js b/devtools/client/inspector/rules/views/text-property-editor.js index 5ba47e9be603..a1ac5b15a9b4 100644 --- a/devtools/client/inspector/rules/views/text-property-editor.js +++ b/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, diff --git a/devtools/client/shared/output-parser.js b/devtools/client/shared/output-parser.js index e6e3a133e28b..5ef39ca82a20 100644 --- a/devtools/client/shared/output-parser.js +++ b/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}) - * - * Valid options are: - * - defaultColorType: true // Convert colors to the default type - * // selected in the options panel. - * - angleClass: "" // The class to use for the angle value + * @param {Boolean} overrides.useDefaultColorUnit: Convert colors to the default type + * selected in the options panel. + * @param {String} overrides.angleClass: The class to use for the angle value that follows + * the swatch. + * @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. - * - bezierClass: "" // The class to use for the bezier value + * @param {} overrides.bezierSwatchClass: "" // The class to use for bezier swatches. + * @param {} overrides.colorClass: "" // The class to use for the color value * // that follows the swatch. - * - bezierSwatchClass: "" // The class to use for bezier swatches. - * - colorClass: "" // The class to use for the color value - * // that follows the swatch. - * - colorSwatchClass: "" // The class to use for color swatches. - * - filterSwatch: false // A special case for parsing a + * @param {} overrides.colorSwatchClass: "" // The class to use for color swatches. + * @param {} overrides.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. - * - gridClass: "" // The class to use for the grid icon. - * - shapeClass: "" // The class to use for the shape value + * @param {} overrides.flexClass: "" // The class to use for the flex icon. + * @param {} overrides.gridClass: "" // The class to use for the grid icon. + * @param {} overrides.shapeClass: "" // The class to use for the shape value * // that follows the swatch. - * - shapeSwatchClass: "" // The class to use for the shape swatch. - * - supportsColor: false // Does the CSS property support colors? - * - urlClass: "" // The class to be used for url() links. - * - fontFamilyClass: "" // The class to be used for font families. - * - baseURI: undefined // A string used to resolve + * @param {} overrides.shapeSwatchClass: "" // The class to use for the shape swatch. + * @param {} overrides.supportsColor: false // Does the CSS property support colors? + * @param {} overrides.urlClass: "" // The class to be used for url() links. + * @param {} overrides.fontFamilyClass: "" // The class to be used for font families. + * @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: "", diff --git a/devtools/client/shared/test/browser_css_color.js b/devtools/client/shared/test/browser_css_color.js index d2403238b8c7..d66656ac86a6 100644 --- a/devtools/client/shared/test/browser_css_color.js +++ b/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; - is(color.toString(), name, "toString() with authored type"); - - color.colorUnit = colorUtils.CssColor.COLORUNIT.hex; - is(color.toString(), hex, "toString() with hex 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"); + const { COLORUNIT } = colorUtils.CssColor; + is(color.toString(COLORUNIT.name), name, "toString() with authored type"); + is(color.toString(COLORUNIT.hex), hex, "toString() with hex type"); + is(color.toString(COLORUNIT.hsl), hsl, "toString() with hsl type"); + is(color.toString(COLORUNIT.rgb), rgb, "toString() with rgb type"); } function testColorMatch(name, hex, hsl, rgb, rgba, canvas) { diff --git a/devtools/client/shared/test/xpcshell/test_cssColor-01.js b/devtools/client/shared/test/xpcshell/test_cssColor-01.js index 516c937b39ef..3ba7b08fe51f 100644 --- a/devtools/client/shared/test/xpcshell/test_cssColor-01.js +++ b/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(), "black", "test non-upper-case color cycling"); + equal(black.toString("name"), "black", "test non-upper-case color cycling"); const upper = new colorUtils.CssColor("BLACK"); - upper.colorUnit = "hex"; - equal(upper.toString(), "#000", "test upper-case color cycling"); - upper.colorUnit = "name"; - equal(upper.toString(), "BLACK", "test upper-case color preservation"); + equal(upper.toString("hex"), "#000", "test upper-case color cycling"); + equal(upper.toString("name"), "BLACK", "test upper-case color preservation"); } diff --git a/devtools/client/shared/test/xpcshell/test_cssColor-02.js b/devtools/client/shared/test/xpcshell/test_cssColor-02.js index 7cfbc5ff01b0..77b692e6a742 100644 --- a/devtools/client/shared/test/xpcshell/test_cssColor-02.js +++ b/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(); - //console.log("color.nextColorUnit", color.toString(), color); - color = new colorUtils.CssColor(color.toString()); - //console.log("new color", color.toString(), color); + const newColor = color.nextColorUnit(); + color = new colorUtils.CssColor(newColor); } - return color.toString() === value; + return color.toString(colorUnit) === value; } diff --git a/devtools/client/shared/test/xpcshell/test_cssColor-8-digit-hex.js b/devtools/client/shared/test/xpcshell/test_cssColor-8-digit-hex.js index 93280cf99e6b..877920294f99 100644 --- a/devtools/client/shared/test/xpcshell/test_cssColor-8-digit-hex.js +++ b/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(); + const color = cssColor.toString(colorUtils.CssColor.COLORUNIT.hex); equal(color, EIGHT_CHARACTER_HEX, "alpha value is correct"); } diff --git a/devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js b/devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js index abcb63333b97..6cfceccc0b8d 100644 --- a/devtools/client/shared/widgets/tooltip/SwatchColorPickerTooltip.js +++ b/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(); + return colorObj.toString(unit, forceUppercase); } /** diff --git a/devtools/shared/css/color.js b/devtools/shared/css/color.js index 0c9a03cd10ff..41edfd1a4916 100644 --- a/devtools/shared/css/color.js +++ b/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) { - // 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); - } - } + #currentFormat; 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 && - this.colorUnit != CssColor.COLORUNIT.authored + forceUppercase || + (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, };