1136257 - Switch between color unit format in place

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[André Santos]

It would be a nice thing to add a way to switch between color unit formats (hex, rgb, hls and color name) in place, meaning with a shortcut I should be able to click the color and change it's format. I'm not talking about choosing my preferred color format in the settings tab.

This is a feature presented in chrom* dev tools, they choose shift+click as the shortcut.

Comment 1

[Patrick Brosset <:pbro>]

Choosing shift+click too would be wise, to avoid adding a new keybinding users would have to learn.
Mike, you know about color handling in devtools, do you mind commenting a bit on this bug so someone can easily pick it up?

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Quite a few things to change in a few places.

This is one of those things that is cool and reasonably simple to implement although difficult to explain so I will do it... in fact, I have already done 75% of it.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136257-shift-click-switch-color-type.patch

As I thought, we had to tweak things all over the place to make this work (tooltip.js, computed view, rule view and css-color).

Works perfectly even when there are multiple colors e.g. box shadow space invader.

I wish it was more discoverable but the fact that Chrome does it using the same key combination should be enough.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0401a4adde66

Comment 4

[André Santos]

Thank you, Michael.

Is that try for me though? I don't know what's that, I'm kinda new to this.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8572058 [details] [diff] [review]
1136257-shift-click-switch-color-type.patch

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

There is a problem when cycling through a swatch that has changed.  Open up about:home, then select the body element.  Now see the #000 color inherited from html - shift click that a few times.  Now, type in "red" into the field.  Now shift click some more.  Notice that "black" is part of the rotation.  Interestingly, it's not a problem when shift-clicking through the initial swatch.

::: browser/devtools/styleinspector/computed-view.js
@@ +1152,5 @@
>        });
>      this.valueNode.innerHTML = "";
>      this.valueNode.appendChild(frag);
>  
> +    let swatch = this.valueNode.querySelector(".computedview-colorswatch");

I believe almost all of this logic (click handling and color switching) should be encoded into the SwatchColorPickerTooltip so it could be shared between the rule-view and computed-view.  Could this work?

::: toolkit/devtools/css-color.js
@@ +278,5 @@
>      this.authored = color.toLowerCase();
>      return this;
>    },
>  
> +  nextColorUnit: function() {

I think this would do the same thing but without having to do the deduplication or grab checking:

  // Reorder the formats array to have the current format at the
  // front so we can cycle through.
  let formats = ["authored", "hex", "hsl", "rgb", "name"];
  let putOnEnd = formats.splice(0, formats.indexOf(this.colorUnit));
  formats = formats.concat(putOnEnd);
  let currentDisplayedColor = this[formats[0]];

  for (let format of formats) {
    if (this[format] !== currentDisplayedColor) {
      this._colorUnit = CssColor.COLORUNIT[format];
      break;
    }
  }

  return this.toString();

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to andrepaulino.santos from comment #4)
> Is that try for me though?

No, it's just a push to the test server to make sure all the tests are passing.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136257-shift-click-switch-color-type.patch

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Comment on attachment 8572058 [details] [diff] [review]
> 1136257-shift-click-switch-color-type.patch
> 
> Review of attachment 8572058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is a problem when cycling through a swatch that has changed.  Open up
> about:home, then select the body element.  Now see the #000 color inherited
> from html - shift click that a few times.  Now, type in "red" into the
> field.  Now shift click some more.  Notice that "black" is part of the
> rotation.  Interestingly, it's not a problem when shift-clicking through the
> initial swatch.
> 

Fixed... just needed to reset the stored color object when the property was updated.

> ::: browser/devtools/styleinspector/computed-view.js
> @@ +1152,5 @@
> >        });
> >      this.valueNode.innerHTML = "";
> >      this.valueNode.appendChild(frag);
> >  
> > +    let swatch = this.valueNode.querySelector(".computedview-colorswatch");
> 
> I believe almost all of this logic (click handling and color switching)
> should be encoded into the SwatchColorPickerTooltip so it could be shared
> between the rule-view and computed-view.  Could this work?
> 

That was my original approach but it falls short because:

1. The computed view uses DOM Templater. This templater clones everything as part of processing so any added eventlisteners would be lost. We would have to migrate away from DOM Templater in order for this approach to work.
2. The markup for the rule and computed views is slightly different when it comes to color swatches. Of course we could work around that but then we would have code specific to the rule and computed views in Tooltip.js.

> ::: toolkit/devtools/css-color.js
> @@ +278,5 @@
> >      this.authored = color.toLowerCase();
> >      return this;
> >    },
> >  
> > +  nextColorUnit: function() {
> 
> I think this would do the same thing but without having to do the
> deduplication or grab checking:
> 
>   // Reorder the formats array to have the current format at the
>   // front so we can cycle through.
>   let formats = ["authored", "hex", "hsl", "rgb", "name"];
>   let putOnEnd = formats.splice(0, formats.indexOf(this.colorUnit));
>   formats = formats.concat(putOnEnd);
>   let currentDisplayedColor = this[formats[0]];
> 
>   for (let format of formats) {
>     if (this[format] !== currentDisplayedColor) {
>       this._colorUnit = CssColor.COLORUNIT[format];
>       break;
>     }
>   }
> 
>   return this.toString();

I hadn't put a huge amount of thought into it but that does seem like a far better approach.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66636a17eb6

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136257-shift-click-switch-color-type.patch

Fixed color order issue:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7664ca2176bd

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #7)
> > ::: browser/devtools/styleinspector/computed-view.js
> > @@ +1152,5 @@
> > >        });
> > >      this.valueNode.innerHTML = "";
> > >      this.valueNode.appendChild(frag);
> > >  
> > > +    let swatch = this.valueNode.querySelector(".computedview-colorswatch");
> > 
> > I believe almost all of this logic (click handling and color switching)
> > should be encoded into the SwatchColorPickerTooltip so it could be shared
> > between the rule-view and computed-view.  Could this work?
> > 
> 
> That was my original approach but it falls short because:
> 
> 1. The computed view uses DOM Templater. This templater clones everything as
> part of processing so any added eventlisteners would be lost. We would have
> to migrate away from DOM Templater in order for this approach to work.
> 2. The markup for the rule and computed views is slightly different when it
> comes to color swatches. Of course we could work around that but then we
> would have code specific to the rule and computed views in Tooltip.js.

What about output-parser.js?  It has an _appendColor function already that creates the swatch in all cases.  Maybe we could put the shift+click logic there and prevent default on the event such that the tooltip never opens at all.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8572643 [details] [diff] [review]
1136257-shift-click-switch-color-type.patch

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

See comment 9 - it seems like we could share most of this logic in the output-parser and move it out of the tooltip/rule-view/computed-view.  This would also make any future swatches work automatically.

::: browser/devtools/styleinspector/computed-view.js
@@ +1167,5 @@
> +    }
> +
> +    // Shift-click causes text selection and we need that for copy paste so all
> +    // we can do here is quickly clear it.
> +    this.tree.styleDocument.getSelection().removeAllRanges();

From a quick test I think we can get rid of the range flicker here (and in rule view) by binding to mousedown instead of click and calling preventDefault.

@@ +1271,5 @@
>     * event.
>     */
>    onMatchedToggle: function PropertyView_onMatchedToggle(aEvent)
>    {
> +    if (aEvent.shiftKey) {

I'd prefer to see a preventDefault on a dblclick event on the swatch element since it's a more direct way to do this.  Or maybe preventing default on mousedown as in the comment above will do the same thing?

::: browser/devtools/styleinspector/rule-view.js
@@ +2561,5 @@
>          // Adding this swatch to the list of swatches our colorpicker knows about
>          this.ruleEditor.ruleView.tooltips.cubicBezier.addSwatch(span, {
>            onPreview: () => this._previewValue(this.valueSpan.textContent),
>            onCommit: () => this._applyNewValue(this.valueSpan.textContent),
> +          onRevert: () => this._applyNewValue(originalValue, true)

Looks like an unintentional change to the cubic bezier tooltip

Comment 11

[André Santos]

What's the status of this?

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136257-shift-click-switch-color-type.patch

(In reply to André Santos from comment #11)
> What's the status of this?

Funny you should ask.

I decided that this really does belong in the output parser as this is the place that the color swatches are created. This means that we get this behaviour for free wherever we use the output parser to create color swatches.

I worked around the problem with DOM Templater removing the listeners in the computed view by stopping using the DOM Templater for creating matched selectors.

Of course I have used event.preventDefault() etc. to prevent highlight flickering.

bgrins mentioned using a doubleclick event but we should copy Chrome here to make Chrome users feel warm and fuzzy.

I have also removed the change from the cubic bezier tooltip.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f81dff5e806

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8584544 [details] [diff] [review]
1136257-shift-click-switch-color-type.patch

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

::: browser/devtools/shared/test/browser_css_color.js
@@ -47,5 @@
>    testProcessCSSString();
>    testSetAlpha();
>  }
>  
>  function testToString(color, name, hex, hsl, rgb) {

Do we have a separate test for the default color unit pref?  If not, we should keep around a similar test to this one that initializes a new color after setting the default color pref and making sure the correct value comes out of toString().

::: browser/devtools/shared/widgets/Tooltip.js
@@ +968,5 @@
>    _onSwatchClick: function(event) {
>      let swatch = this.swatches.get(event.target);
> +
> +    if (event.shiftKey) {
> +      event.preventDefault();

I don't *think* the preventDefault() is necessary here, is it?  I know the stopProp is needed so the inplaceEditor doesn't open up

::: browser/devtools/styleinspector/computed-view.js
@@ +1540,5 @@
> + *        The tag name.
> + * @param {object} aAttributes
> + *        A set of attributes to set on the node.
> + */
> +function createChild(aParent, aTag, aAttributes={}) {

We should export this from a shared module since it's copy/pasted from the rule view.  Surprisingly, I don't think we have a shared file for this type of thing yet.  Maybe viewhelpers.jsm, or styleeditor/utils.js.  We can do that in a follow up though

::: browser/devtools/styleinspector/test/browser_ruleview_colorpicker-cycle-color.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test cycling color types in the rule view color picker tooltip

This file name and comment indicate that it's happening inside the tooltip, but really it's just testing the swatch in the rule view.  Please rename to match the computedview test name and update this comment, along with all the other strings in this file that say 'color picker'.

@@ +5,5 @@
> +"use strict";
> +
> +// Test cycling color types in the rule view color picker tooltip
> +
> +const PAGE_CONTENT = [

Nit: The way this test and the computed view test are setup are a bit different - can you use one style (PAGE_CONTENT or setting innerHTML) so these two new tests match?

@@ +39,5 @@
> +  EventUtils.synthesizeMouseAtCenter(swatch, {type: "mousedown", shiftKey: true}, win);
> +  is(valueNode.textContent, "rgb(255, 0, 0)",
> +                            "Color displayed as an RGB value.");
> +
> +  EventUtils.synthesizeMouseAtCenter(swatch, {type: "mousedown", shiftKey: true}, win);

This should cycle one more time to make sure it returns back to the original format

::: toolkit/devtools/output-parser.js
@@ +422,5 @@
> +    let val = color.nextColorUnit();
> +
> +    swatch.nextElementSibling.textContent = val;
> +
> +    event.preventDefault();

You could move this call to event.preventDefault() to the top of the function so that it prevents text selection in the case of double-clicking (non-shift) a swatch.  Also, please add a comment about how it's needed to prevent text selection.

@@ +423,5 @@
> +
> +    swatch.nextElementSibling.textContent = val;
> +
> +    event.preventDefault();
> +    return false;

Do we need to the return false here?

Comment 14

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136257-shift-click-switch-color-type.patch

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8584544 [details] [diff] [review]
> 1136257-shift-click-switch-color-type.patch
> 
> Review of attachment 8584544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/test/browser_css_color.js
> @@ -47,5 @@
> >    testProcessCSSString();
> >    testSetAlpha();
> >  }
> >  
> >  function testToString(color, name, hex, hsl, rgb) {
> 
> Do we have a separate test for the default color unit pref?  If not, we
> should keep around a similar test to this one that initializes a new color
> after setting the default color pref and making sure the correct value comes
> out of toString().
> 

A bunch of tests check this (e.g. the css text says red but the rule view shows #F00).

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +968,5 @@
> >    _onSwatchClick: function(event) {
> >      let swatch = this.swatches.get(event.target);
> > +
> > +    if (event.shiftKey) {
> > +      event.preventDefault();
> 
> I don't *think* the preventDefault() is necessary here, is it?  I know the
> stopProp is needed so the inplaceEditor doesn't open up
> 

Removed.

> ::: browser/devtools/styleinspector/computed-view.js
> @@ +1540,5 @@
> > + *        The tag name.
> > + * @param {object} aAttributes
> > + *        A set of attributes to set on the node.
> > + */
> > +function createChild(aParent, aTag, aAttributes={}) {
> 
> We should export this from a shared module since it's copy/pasted from the
> rule view.  Surprisingly, I don't think we have a shared file for this type
> of thing yet.  Maybe viewhelpers.jsm, or styleeditor/utils.js.  We can do
> that in a follow up though
> 

Logged bug 1152721.

> :::
> browser/devtools/styleinspector/test/browser_ruleview_colorpicker-cycle-
> color.js
> @@ +3,5 @@
> > + http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> > +
> > +// Test cycling color types in the rule view color picker tooltip
> 
> This file name and comment indicate that it's happening inside the tooltip,
> but really it's just testing the swatch in the rule view.  Please rename to
> match the computedview test name and update this comment, along with all the
> other strings in this file that say 'color picker'.
> 

Done.

Comments now say e.g. // Test cycling color types in the rule view

> @@ +5,5 @@
> > +"use strict";
> > +
> > +// Test cycling color types in the rule view color picker tooltip
> > +
> > +const PAGE_CONTENT = [
> 
> Nit: The way this test and the computed view test are setup are a bit
> different - can you use one style (PAGE_CONTENT or setting innerHTML) so
> these two new tests match?
> 

Done

> @@ +39,5 @@
> > +  EventUtils.synthesizeMouseAtCenter(swatch, {type: "mousedown", shiftKey: true}, win);
> > +  is(valueNode.textContent, "rgb(255, 0, 0)",
> > +                            "Color displayed as an RGB value.");
> > +
> > +  EventUtils.synthesizeMouseAtCenter(swatch, {type: "mousedown", shiftKey: true}, win);
> 
> This should cycle one more time to make sure it returns back to the original
> format
> 

Done

> ::: toolkit/devtools/output-parser.js
> @@ +422,5 @@
> > +    let val = color.nextColorUnit();
> > +
> > +    swatch.nextElementSibling.textContent = val;
> > +
> > +    event.preventDefault();
> 
> You could move this call to event.preventDefault() to the top of the
> function so that it prevents text selection in the case of double-clicking
> (non-shift) a swatch.  Also, please add a comment about how it's needed to
> prevent text selection.
> 

Done

> @@ +423,5 @@
> > +
> > +    swatch.nextElementSibling.textContent = val;
> > +
> > +    event.preventDefault();
> > +    return false;
> 
> Do we need to the return false here?

Not any more... removed.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8590195 [details] [diff] [review]
1136257-shift-click-switch-color-type.patch

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

Nice.  Looks ready to go if it's got a green try push

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136257-shift-click-switch-color-type.patch

Rebased.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25ef3533b4d

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136257-shift-click-switch-color-type.patch

Rebased. Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc09f3b17672

Comment 19

[Carsten Book [:Tomcat]]

Hi Michael, this seems still to have problems to apply:

patching file browser/devtools/styleinspector/computedview.xhtml
Hunk #1 FAILED at 74
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/styleinspector/computedview.xhtml.rej
patching file browser/devtools/styleinspector/test/browser.ini
Hunk #2 succeeded at 96 with fuzz 1 (offset 2 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 1136257-shift-click-switch-color-type.patch

maybe needs rebased against fx-team ?

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 1136257-shift-click-switch-color-type.patch

(In reply to Carsten Book [:Tomcat] from comment #19)
> Hi Michael, this seems still to have problems to apply:
> 
> patching file browser/devtools/styleinspector/computedview.xhtml
> Hunk #1 FAILED at 74
> 1 out of 1 hunks FAILED -- saving rejects to file
> browser/devtools/styleinspector/computedview.xhtml.rej
> patching file browser/devtools/styleinspector/test/browser.ini
> Hunk #2 succeeded at 96 with fuzz 1 (offset 2 lines).
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh
> 1136257-shift-click-switch-color-type.patch
> 
> maybe needs rebased against fx-team ?

Yeah, computedview.xhtml changed shortly after adding checkin-needed.

Rebased.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/d321fbee8cd8

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out in https://hg.mozilla.org/integration/fx-team/rev/bf544e556f37 under suspicion of it breaking e10s-dt:

https://treeherder.mozilla.org/logviewer.html#?job_id=2751395&repo=fx-team

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The dt orange kept going until I backed out bug 1098374. This can probably reland.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9c9c55112ff6

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c9c55112ff6