1332091 - Add Color Value Editor component to the Color Widget

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

Add a color value editor component to the color widget. This bug is reserved for UCOSP students.

Comment 1

[Stanford Lockhart [:lockhart]]

Attachment: bug1332091-1.0.0.patch

Comment 2

[Stanford Lockhart [:lockhart]]

Attachment: bug1332091-2.0.0.patch

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8829253 [details] [diff] [review]
bug1332091-2.0.0.patch

Clearing the review for now. Please breakdown the patch into 2 parts - feature implementation and test. 

In the patch commit message, you can call it Bug 1332091 - Part 1: XYZ, and Bug 1332091 - Part 2 Add unit tests for color value editors. r=gl

Comment 4

[Stanford Lockhart [:lockhart]]

Attachment: bug1332091-3.0.0-part1.patch

Implementation

Comment 5

[Stanford Lockhart [:lockhart]]

Attachment: bug1332091-3.0.0-part2.patch

Tests

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8830964 [details] [diff] [review]
bug1332091-3.0.0-part1.patch

Review of attachment 8830964 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/widgets/ColorWidget.js
@@ +10,5 @@
>  "use strict";
>  
>  const EventEmitter = require("devtools/shared/event-emitter");
>  const XHTML_NS = "http://www.w3.org/1999/xhtml";
> +const {colorUtils} = require("devtools/shared/css/color");

I would move this require below to EventEmitter to keep the requires together, and add a new line to separate the XHTML_NS const from the block of requires.

@@ +105,5 @@
>    this.alphaSliderInner = this.element.querySelector(".colorwidget-alpha-inner");
>    this.alphaSliderHelper = this.element.querySelector(".colorwidget-alpha-handle");
>    ColorWidget.draggable(this.alphaSliderInner, this.onAlphaSliderMove.bind(this));
>  
> +  this.valueInput = this.element.querySelector(".colorwidget-select");

We should probably rename this variable to be this.colorSelect since it should not be mistaken for an input element

@@ +106,5 @@
>    this.alphaSliderHelper = this.element.querySelector(".colorwidget-alpha-handle");
>    ColorWidget.draggable(this.alphaSliderInner, this.onAlphaSliderMove.bind(this));
>  
> +  this.valueInput = this.element.querySelector(".colorwidget-select");
> +  this.valueInput.addEventListener("change", (evt) => {

Since we only have one argument, we can remove the () and simply do: evt => {

@@ +107,5 @@
>    ColorWidget.draggable(this.alphaSliderInner, this.onAlphaSliderMove.bind(this));
>  
> +  this.valueInput = this.element.querySelector(".colorwidget-select");
> +  this.valueInput.addEventListener("change", (evt) => {
> +    this.onSelectValueChange.bind(this)(evt.target.value);

I would take all the event handler bindings to 'this', and have a block do all binding as you see in http://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/rules.js#116. That way in these event handlers you can simply do this.onSelectValueChange(evt.target.value). You can even destructure the evt object and get {target} and then do this.onSelectValueChange(target.value)

@@ +113,5 @@
> +
> +  this.hexValue = this.element.querySelector(".colorwidget-hex");
> +  this.hexValueInput = this.element.querySelector(".colorwidget-hex-input");
> +  this.hexValueInput.addEventListener("input", (evt) => {
> +    this.onHexInputChange.bind(this)(evt.target.value);

We can remove the () around evt.

@@ +117,5 @@
> +    this.onHexInputChange.bind(this)(evt.target.value);
> +  });
> +
> +  this.rgbaValue = this.element.querySelector(".colorwidget-rgba");
> +  this.rgbaValueInput = {

s/this.rgbaValueInput/this.rgbaValueInputs

@@ +123,5 @@
> +    g: this.element.querySelector(".colorwidget-rgba-g"),
> +    b: this.element.querySelector(".colorwidget-rgba-b"),
> +    a: this.element.querySelector(".colorwidget-rgba-a"),
> +  };
> +  this.rgbaValue.addEventListener("input", (evt) => {

We can remove the () around evt.

@@ +124,5 @@
> +    b: this.element.querySelector(".colorwidget-rgba-b"),
> +    a: this.element.querySelector(".colorwidget-rgba-a"),
> +  };
> +  this.rgbaValue.addEventListener("input", (evt) => {
> +    this.onRgbaInputChange.bind(this)(evt.target.className.split("-")[2],

Instead of splitting the className, we can add a data attribute like 'data-id="r"' in the HTML and reference it by event.target.dataset.dataId. https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes.

@@ +129,5 @@
> +        evt.target.value);
> +  });
> +
> +  this.hslaValue = this.element.querySelector(".colorwidget-hsla");
> +  this.hslaValueInput = {

s/this.hslaValueInput/this.hslaValueInputs

@@ +135,5 @@
> +    s: this.element.querySelector(".colorwidget-hsla-s"),
> +    l: this.element.querySelector(".colorwidget-hsla-l"),
> +    a: this.element.querySelector(".colorwidget-hsla-a"),
> +  };
> +  this.hslaValue.addEventListener("input", (evt) => {

We can remove the () around evt.

@@ +137,5 @@
> +    a: this.element.querySelector(".colorwidget-hsla-a"),
> +  };
> +  this.hslaValue.addEventListener("input", (evt) => {
> +    this.onHslaInputChange.bind(this)(evt.target.className.split("-")[2],
> +        evt.target.value);

Use a data attribute.

@@ +195,5 @@
>    return [h, s, v, a];
>  };
>  
> +ColorWidget.hslToCssString = function (h, s, l, a) {
> +  return "hsla(" + h + ", " + s + "%, " + l + "%, " + a + ")";

We should use es6 template strings here `hsla(${h}, ${s}%....`

@@ +272,5 @@
>  ColorWidget.prototype = {
>    set rgb(color) {
>      this.hsv = ColorWidget.rgbToHsv(color[0], color[1], color[2], color[3]);
> +
> +    const hslaTuple = new colorUtils.CssColor(this.rgbCssString)._getHSLATuple();

We use some es6 destructuring here and do { h, s, l } = new colorUtils...

@@ +273,5 @@
>    set rgb(color) {
>      this.hsv = ColorWidget.rgbToHsv(color[0], color[1], color[2], color[3]);
> +
> +    const hslaTuple = new colorUtils.CssColor(this.rgbCssString)._getHSLATuple();
> +    this.hsl = [Math.round(hslaTuple.h), Math.round(hslaTuple.s), Math.round(hslaTuple.l), color[3]];

We shouldn't be rounding these values since we are also displaying 1 decimal place in the rules view.

@@ +367,5 @@
> +    const color = new colorUtils.CssColor(hex, true);
> +    if (!color.rgba) {
> +      return;
> +    }
> +    const rgbTuple = color._getRGBATuple();

We can also use es6 destructuring here as well const { r, g, b, a} = color.getRGBATuple();

@@ +439,5 @@
> +        break;
> +    }
> +
> +    const cssString = ColorWidget.hslToCssString(hsl[0], hsl[1], hsl[2], hsl[3]);
> +    const rgbaTuple = new colorUtils.CssColor(cssString)._getRGBATuple();

Can also use es6 destructuring here as well

::: devtools/client/shared/widgets/color-widget.css
@@ +152,5 @@
>    height: 5px;
>    left: -3px;
>    right: -3px;
>  }
> +

I would add a CSS comment block like 

/**
 * Color Widget Editor
 */

to signify that this block of CSS relates to the color value editor.

@@ +162,5 @@
> +.colorwidget-select {
> +  width: 100%;
> +}
> +
> +.colorwidget-spacing {

I would rename this to colorwidget-select-spacing to be a bit more specific. Typically we would leave this as-is if it was used in different elements, but this only affects the <Select>. Alternatively, I would create separate CSS comment blocks to indicate components that are styled by the CSS rules that follows.

Ex,

/**
 * Color Widget Select
 */

/**
 * Color Widget Inputs
 */

Comment 7

[Stanford Lockhart [:lockhart]]

Attachment: bug1332091-3.0.1-part1.patch

Comment 8

[Stanford Lockhart [:lockhart]]

Attachment: bug1332091-3.0.1-part2.patch

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8832059 [details] [diff] [review]
bug1332091-3.0.1-part1.patch

Review of attachment 8832059 [details] [diff] [review]:
-----------------------------------------------------------------

I am gonna pass the final review of the color conversion code to tromey since he is more familiar with this area. Otherewise, code changes look ok to me.

::: devtools/client/shared/widgets/ColorWidget.js
@@ +112,5 @@
>    this.alphaSliderHelper = this.element.querySelector(".colorwidget-alpha-handle");
>    ColorWidget.draggable(this.alphaSliderInner, this.onAlphaSliderMove.bind(this));
>  
> +  this.colorSelect = this.element.querySelector(".colorwidget-select");
> +  this.colorSelect.addEventListener("change", evt => {

This can simply be:

this.colorSelect.addEventListener("change", this.onSelectValueChange);

The event is automatically passed into the event handler, and you can break down the event object inside the event handler to retrieve the target value.

@@ +118,5 @@
> +  });
> +
> +  this.hexValue = this.element.querySelector(".colorwidget-hex");
> +  this.hexValueInput = this.element.querySelector(".colorwidget-hex-input");
> +  this.hexValueInput.addEventListener("input", evt => {

this.hexValueInput.addEventListener("input", this.onHexInputChange);

@@ +129,5 @@
> +    g: this.element.querySelector(".colorwidget-rgba-g"),
> +    b: this.element.querySelector(".colorwidget-rgba-b"),
> +    a: this.element.querySelector(".colorwidget-rgba-a"),
> +  };
> +  this.rgbaValue.addEventListener("input", evt => {

this.rgbaValue.addEventListener("input", this.onRgbaInputChange);

@@ +140,5 @@
> +    s: this.element.querySelector(".colorwidget-hsla-s"),
> +    l: this.element.querySelector(".colorwidget-hsla-l"),
> +    a: this.element.querySelector(".colorwidget-hsla-a"),
> +  };
> +  this.hslaValue.addEventListener("input", evt => {

this.hslaValue.addEventListener("input", this.onHslaInputChange);

@@ +318,5 @@
>    },
>  
>    onSliderMove: function (dragX, dragY) {
>      this.hsv[0] = (dragY / this.slideHeight);
> +    this.hsl[0] = Math.round((dragY / this.slideHeight) * 360);

We don't want to do round here.

@@ +331,5 @@
> +    this.hsl[2] = ((2 - this.hsv[1]) * this.hsv[2] / 2);
> +    if (this.hsl[2] && this.hsl[2] < 1) {
> +      this.hsl[1] = this.hsv[1] * this.hsv[2] /
> +          (this.hsl[2] < 0.5 ? this.hsl[2] * 2 : 2 - this.hsl[2] * 2);
> +      this.hsl[1] = Math.round(this.hsl[1] * 100);

Remove the Math.round. I noticed that we typically keep the decimal places when we switch color values in the rule view (shift click on the color value in the rule view) so we want to be consistent.

@@ +333,5 @@
> +      this.hsl[1] = this.hsv[1] * this.hsv[2] /
> +          (this.hsl[2] < 0.5 ? this.hsl[2] * 2 : 2 - this.hsl[2] * 2);
> +      this.hsl[1] = Math.round(this.hsl[1] * 100);
> +    }
> +    this.hsl[2] = Math.round(this.hsl[2] * 100);

Remove the Math.round

@@ +371,5 @@
> +  onHexInputChange: function (hex) {
> +    const color = new colorUtils.CssColor(hex, true);
> +    if (!color.rgba) {
> +      return;
> +    }

Add a new line after this to separate the if statement block

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

@ttromey, you need to enable devtools.inspector.colorWidget.enabled to see this feature in the color picker tooltip.

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8832059 [details] [diff] [review]
bug1332091-3.0.1-part1.patch

Review of attachment 8832059 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch.  I have one comment but there's nothing really to do about it.
I think the patch is ok.

::: devtools/shared/css/color.js
@@ +420,5 @@
> +    return {
> +      h,
> +      s,
> +      l,
> +      a: parseFloat(a.toFixed(1))

I don't really know why this parseFloat is needed, but considering that it's also done in _getRGBATuple, I suppose it is ok.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8832061 [details] [diff] [review]
bug1332091-3.0.1-part2.patch

Review of attachment 8832061 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/test/browser_color-widget-02.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that the spectrum color picker works correctly

Needs a better comment that describes what is happening in the test file.

Comment 13

[Stanford Lockhart [:lockhart]]

Attachment: bug1332091-part1-v5.patch

Comment 14

[Stanford Lockhart [:lockhart]]

Attachment: bug1332091-part2-v5.patch

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65dbdf4a2a5b
Part 1: Add colour value editors for Hex, RGBA, HSLA, and a selection box to go between them. r=gl

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65dbdf4a2a5b

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

To test this feature, you must have devtools.inspector.colorWidget.enabled set to true in about:config

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:gl, maybe it's time to close this bug?

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #18)
> The leave-open keyword is there and there is no activity for 6 months.
> :gl, maybe it's time to close this bug?

Closing the bug since the remaining patch to add unit test will be addressed by mtigley.

Comment 20

[Rares Doghi, Desktop QA]

I can Confirm this issue as Fixed in Firefox 64.0, Nightly 66.0a1 (2018-12-19) and Beta 65.0b5.