Closed Bug 1136257 Opened 5 years ago Closed 5 years ago

Switch between color unit format in place

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: andrewskad, Assigned: miker)

References

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 file, 7 obsolete files)

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.
OS: Linux → All
Hardware: x86_64 → All
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?
Flags: needinfo?(mratcliffe)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
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
Attachment #8572058 - Flags: review?(bgrinstead)
Thank you, Michael.

Is that try for me though? I don't know what's that, I'm kinda new to this.
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();
Attachment #8572058 - Flags: review?(bgrinstead) → review-
(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.
(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
Attachment #8572058 - Attachment is obsolete: true
Attachment #8572582 - Flags: review?(bgrinstead)
Fixed color order issue:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7664ca2176bd
Attachment #8572582 - Attachment is obsolete: true
Attachment #8572582 - Flags: review?(bgrinstead)
Attachment #8572643 - Flags: review?(bgrinstead)
(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 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
Attachment #8572643 - Flags: review?(bgrinstead)
Blocks: 958164
What's the status of this?
(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
Attachment #8572643 - Attachment is obsolete: true
Attachment #8584544 - Flags: review?(bgrinstead)
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?
Attachment #8584544 - Flags: review?(bgrinstead) → feedback+
Whiteboard: [devedition-40]
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
(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.
Attachment #8584544 - Attachment is obsolete: true
Attachment #8590195 - Flags: review?(bgrinstead)
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
Attachment #8590195 - Flags: review?(bgrinstead) → review+
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
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 ?
Flags: needinfo?(mratcliffe)
Keywords: checkin-needed
(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.
Attachment #8593356 - Attachment is obsolete: true
Flags: needinfo?(mratcliffe)
Attachment #8593886 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/d321fbee8cd8
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
The dt orange kept going until I backed out bug 1098374. This can probably reland.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9c9c55112ff6
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9c9c55112ff6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Flags: needinfo?(mratcliffe)
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Depends on: 1248275
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.