1476311 - Re-add the new ColorWidget code

Status: RESOLVED WONTFIX

(DevTools :: Inspector, enhancement, P3)

Description

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

This bug consists of 2 parts:

1 - Backing out the changes we done in Bug 1446316 in removing the new ColorWidget code.
2 - Removing the innerHTML usage in the ColorWidget

Comment 1

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

Attachment: Bug 1476311 - Part 1: Backout the changes in Bug 1446316 in removing the new ColorWidget code.

Review commit: https://reviewboard.mozilla.org/r/257536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257536/

Comment 2

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

Comment on attachment 8992679 [details]
Bug 1476311 - Part 1: Backout the changes in Bug 1446316 in removing the new ColorWidget code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257536/diff/1-2/

Comment 3

[Micah [:mtigley] (she/her)]

Attachment: Bug 1476311 - Part 2: Remove form elements from innerHTML.

Review commit: https://reviewboard.mozilla.org/r/257654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257654/

Comment 4

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

Comment on attachment 8992819 [details]
Bug 1476311 - Part 2: Remove form elements from innerHTML.

https://reviewboard.mozilla.org/r/257654/#review264564

::: devtools/client/shared/widgets/ColorWidget.js:45
(Diff revision 1)
>   * Fires the following events:
>   * - changed : When the user changes the current color
>   */
>  function ColorWidget(parentEl, rgb) {
>    EventEmitter.decorate(this);
>  

We can do initialize this.document that can be reusued later so we don't have to pass around the document in our build method.

this.document = parentEl.ownerDocument

::: devtools/client/shared/widgets/ColorWidget.js:267
(Diff revision 1)
>    },
>  
> +  buildContrastHelpButton(doc) {
> +    this.contrastHelp = doc.createElementNS(XHTML_NS, "button");
> +    this.contrastHelp.className = "colorwidget-contrast-help";
> +    this.contrastHelp.classList.add("devtools-button");

You can simplify this a bit by doing 

this.contrastHelp.className = "colorwidget-contrast-help devtools-button";

::: devtools/client/shared/widgets/ColorWidget.js:273
(Diff revision 1)
> +
> +    const contrastInner = this.element.querySelector(".colorwidget-contrast-inner");
> +    contrastInner.appendChild(this.contrastHelp);
> +  },
> +
> +  buildColorSelect(doc) {

I am wondering if we should change "build" to "create" for the method name since you used create in the comment. What do you think?

::: devtools/client/shared/widgets/ColorWidget.js:274
(Diff revision 1)
> +    const contrastInner = this.element.querySelector(".colorwidget-contrast-inner");
> +    contrastInner.appendChild(this.contrastHelp);
> +  },
> +
> +  buildColorSelect(doc) {
> +    // Create markup for color widget's select component

Since these are effectively JSDocs, let's move them outside the function block and into

/**
 * Create markup for color widgets's select component.
 */

::: devtools/client/shared/widgets/ColorWidget.js:275
(Diff revision 1)
> +    contrastInner.appendChild(this.contrastHelp);
> +  },
> +
> +  buildColorSelect(doc) {
> +    // Create markup for color widget's select component
> +    this.colorWidgetValue = this.element.querySelector(".colorwidget-value");

s/colorWidget/colorValue 

I think I would prefer if we don't prefix these variable names with "colorWidget" and simplify it to just "color"

We might also want to add suffix like "El" to significant the Element. 

I am wondering if we need to hold onto some of this variables like this.colorWidgetValue since they aren't used elsewhere. I would do a quick audit of the (this.X) variables we create to make sure we really need them, otherwise, just assign them to a local variable.

::: devtools/client/shared/widgets/ColorWidget.js:319
(Diff revision 1)
> +  buildRGBAInput(doc) {
> +   // Create markup for color widget's rgba's inputs
> +    this.rgbaValue = this.element.querySelector(".colorwidget-rgba");
> +    this.rgbaR = doc.createElementNS(XHTML_NS, "input");
> +    this.rgbaR.className = "colorwidget-rgba-r";
> +    this.rgbaR["data-id"] = "r";

Looking at how we set data attributes, our preferred method seems to be using datset.

this.rgbaR.dataset.id = "r"

::: devtools/client/shared/widgets/ColorWidget.js:439
(Diff revision 1)
> -        <button class="colorwidget-contrast-help devtools-button"></button>
>        </div>
>      </div>
>      `;
>  
> +    const doc = this.element.ownerDocument;

We can avoid passing the doc if we initialize this.document and simply call this.document where we need to call createElementNS

::: devtools/client/shared/widgets/ColorWidget.js:445
(Diff revision 1)
> +
> +    // Form elements need to be appended manually after innerHTML is declared.
> +    // This must be done due to security issues of having form elements available
> +    // in innerHTML.
> +    this.buildColorSelect(doc);
> +    this.buildHexInput(doc);

I am also wondering if we can build the different inputs on demand based on what color value we have selected, but we can investigate this in another bug.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8992679 [details]
Bug 1476311 - Part 1: Backout the changes in Bug 1446316 in removing the new ColorWidget code.

https://reviewboard.mozilla.org/r/257536/#review264618

R+ because this is just a backout of code we already had before and decided to remove because it was unused.
Now, we do have new mockups and people interested in picking up where we left off last time.
Having said this, please do not land as is. Please only land together with a part 2 (or an update of this commit) which we feel confident it will replace the current color picker.
I wouldn't want to be in a position where we, again, have 2 color pickers and 1 is disabled/unfinished for a long time.
I suggestion I can make is: make the new color picker as close as possible from the current one so enabling it and removing the current one is easy. And then add the new features/new design incrementally, on a color picker that is already enabled by default.

Comment 6

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

We are gonna move towards an incremental approach and not build on the ColorWidget. Closing the bug as WONTFIX.