Bug 1594402 - Store the swatchcolorpicker color in a data attribute instead of textcontent r=ladybenko,rcaliman

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

devtools/client/inspector/flexbox/components/FlexContainer.js

@@ -37,7 +37,7 @@ class FlexContainer extends PureComponent {
   constructor(props) {
     super(props);
 
-    this.colorValueEl = createRef();
+    this.swatchContainer = createRef();
     this.swatchEl = createRef();
 
     this.setFlexboxColor = this.setFlexboxColor.bind(this);
@@ -65,7 +65,7 @@ class FlexContainer extends PureComponent {
   }
 
   setFlexboxColor() {
-    const color = this.colorValueEl.current.textContent;
+    const color = this.swatchContainer.current.dataset.color;
     this.props.onSetFlexboxOverlayColor(color);
   }
 
@@ -82,7 +82,11 @@ class FlexContainer extends PureComponent {
       Fragment,
       null,
       dom.div(
-        { className: "flex-header-container-label" },
+        {
+          className: "flex-header-container-label",
+          "data-color": color,
+          ref: this.swatchContainer,
+        },
         getNodeRep(nodeFront, {
           onDOMNodeMouseOut: () => onHideBoxModelHighlighter(),
           onDOMNodeMouseOver: () => onShowBoxModelHighlighterForNode(nodeFront),
@@ -94,18 +98,7 @@ class FlexContainer extends PureComponent {
             backgroundColor: color,
           },
           title: color,
-        }),
-        // The SwatchColorPicker relies on the nextSibling of the swatch element to
-        // apply the selected color. This is why we use a span in display: none for
-        // now. Ideally we should modify the SwatchColorPickerTooltip to bypass this
-        // requirement. See https://bugzilla.mozilla.org/show_bug.cgi?id=1341578
-        dom.span(
-          {
-            className: "layout-color-value",
-            ref: this.colorValueEl,
-          },
-          color
-        )
+        })
       ),
       dom.div(
         { className: "flex-header-container-properties" },

devtools/client/inspector/grids/components/GridItem.js

@@ -46,7 +46,7 @@ class GridItem extends PureComponent {
   constructor(props) {
     super(props);
 
-    this.colorValueEl = createRef();
+    this.swatchContainer = createRef();
     this.swatchEl = createRef();
 
     this.onGridCheckboxClick = this.onGridCheckboxClick.bind(this);
@@ -79,7 +79,7 @@ class GridItem extends PureComponent {
   }
 
   setGridColor() {
-    const color = this.colorValueEl.current.textContent;
+    const color = this.swatchContainer.current.dataset.color;
     this.props.onSetGridOverlayColor(this.props.grid.nodeFront, color);
   }
 
@@ -133,7 +133,10 @@ class GridItem extends PureComponent {
       Fragment,
       null,
       dom.li(
-        {},
+        {
+          "data-color": grid.color,
+          ref: this.swatchContainer,
+        },
         dom.label(
           {},
           dom.input({
@@ -164,18 +167,7 @@ class GridItem extends PureComponent {
             backgroundColor: grid.color,
           },
           title: grid.color,
-        }),
-        // The SwatchColorPicker relies on the nextSibling of the swatch element to apply
-        // the selected color. This is why we use a span in display: none for now.
-        // Ideally we should modify the SwatchColorPickerTooltip to bypass this
-        // requirement. See https://bugzilla.mozilla.org/show_bug.cgi?id=1341578
-        dom.span(
-          {
-            className: "layout-color-value",
-            ref: this.colorValueEl,
-          },
-          grid.color
-        )
+        })
       ),
       this.renderSubgrids()
     );

devtools/client/inspector/shared/test/browser_styleinspector_context-menu-copy-color_02.js

@@ -92,13 +92,15 @@ async function testColorPickerEdit(inspector, view) {
   swatchElement.click();
   await onColorPickerReady;
 
-  const rgbaColor = [83, 183, 89, 1];
-  const rgbaColorText = "rgba(83, 183, 89, 1)";
-  await simulateColorPickerChange(view, picker, rgbaColor);
+  const newColor = "#53B759";
+  const { colorUtils } = require("devtools/shared/css/color");
+
+  const { r, g, b, a } = new colorUtils.CssColor(newColor).getRGBATuple();
+  await simulateColorPickerChange(view, picker, [r, g, b, a]);
 
   is(
     swatchElement.parentNode.dataset.color,
-    rgbaColorText,
+    newColor,
     "data-color was updated"
   );
 
@@ -106,5 +108,5 @@ async function testColorPickerEdit(inspector, view) {
   contextMenu.currentTarget = swatchElement;
   contextMenu._isColorPopup();
 
-  is(contextMenu._colorToCopy, rgbaColorText, "_colorToCopy has the new value");
+  is(contextMenu._colorToCopy, newColor, "_colorToCopy has the new value");
 }

devtools/client/shared/output-parser.js

@@ -1507,7 +1507,7 @@ OutputParser.prototype = {
         }
 
         // The swatch is a <span> instead of a <button> intentionally. See Bug 1597125.
-        // It is made keyboard accessbile via `tabindex` and has keydown handlers
+        // It is made keyboard accessible via `tabindex` and has keydown handlers
         // attached for pressing SPACE and RETURN in SwatchBasedEditorTooltip.js
         const swatch = this._createNode("span", attributes);
         this.colorSwatches.set(swatch, colorObj);
@@ -1590,6 +1590,7 @@ OutputParser.prototype = {
     const val = color.nextColorUnit();
 
     swatch.nextElementSibling.textContent = val;
+    swatch.parentNode.dataset.color = val;
     swatch.emit("unit-change", val);
   },
 

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

@@ -134,8 +134,7 @@ class SwatchColorPickerTooltip extends SwatchBasedEditorTooltip {
 
     // Then set spectrum's color and listen to color changes to preview them
     if (this.activeSwatch) {
-      this.currentSwatchColor = this.activeSwatch.nextSibling;
-      this._originalColor = this.currentSwatchColor.textContent;
+      this._originalColor = this.activeSwatch.parentNode.dataset.color;
       const color = this.activeSwatch.style.backgroundColor;
 
       this.spectrum.off("changed", this._onSpectrumColorChange);
@@ -208,10 +207,13 @@ class SwatchColorPickerTooltip extends SwatchBasedEditorTooltip {
   _selectColor(color) {
     if (this.activeSwatch) {
       this.activeSwatch.style.backgroundColor = color;
-      this.activeSwatch.parentNode.dataset.color = color;
 
       color = this._toDefaultType(color);
-      this.currentSwatchColor.textContent = color;
+
+      this.activeSwatch.parentNode.dataset.color = color;
+      if (this.activeSwatch.nextSibling) {
+        this.activeSwatch.nextSibling.textContent = color;
+      }
       this.preview(color);
 
       if (this.eyedropperOpen) {
@@ -318,7 +320,6 @@ class SwatchColorPickerTooltip extends SwatchBasedEditorTooltip {
   destroy() {
     super.destroy();
     this.inspector = null;
-    this.currentSwatchColor = null;
     this.spectrum.off("changed", this._onSpectrumColorChange);
     this.spectrum.destroy();
   }