889638 - Inspector should have a doorhanger with a colorpicker for editing CSS colors

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It would be really useful if you could just hover over a color in the style editor and a tooltip would pop up and show you what that color actually looks like.

Comment 1

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

This also applies to the rule and computed views

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Are you going to take care of both in this patch, or file another bug for the style editor half now that this bug is in the inspector component, or...?

Comment 3

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

The tooltips should work in all of the CSS tools including the style editor. The plan is to have a doorhanger containing a color picker in the rule view and style editor. In the computed view the doorhanger will contain a color swatch instead.

In the rule and computed views we should have a small color indicator before each color.

Comment 4

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

Each color will have a small color swatch to the left. Hovering the swatch in the computed view will show the color tooltip. Clicking the swatch in the rule view and the style editor should display a color picker that can handle hex, rgb, rgba, hsl & hsla and has a pipette/dropper icon that can be used to open the zoom tool.

Comment 5

[Patrick Brosset <:pbro>]

This is part of our goal on doorhangers. See a roadmap here:https://gist.github.com/captainbrosset/6681978

As Mike explained, we want a color swatch next to the color (depends on bug 918716) and when clicking on it, a color picker should open in a doorhanger.

Comment 6

[Brian Grinstead [:bgrins]]

Is this initially planned for just the ruleview, or is it going to include colors in the markup view as well?

Comment 7

[Patrick Brosset <:pbro>]

Color swatches are already available today in the rule and computed view.
Color pickers will be added to the rule view only.

Comment 8

[Patrick Brosset <:pbro>]

I'm about to submit a patch that contains spectrum, bgrins's color picker script (http://github.com/bgrins/spectrum).

@bgrins : do you have a js test suite we could also include in mozilla's source code that we could run during our mochitest browser test suite?

@jwalker : I have kept bgrins' license header comments in the js and css files. Should I also add Mozilla's?

Comment 9

[Patrick Brosset <:pbro>]

Attachment: color picker tooltip patch V1 (no tests yet)

V1 patch for the color picker tooltip.
Tests are missing still.

I'm attaching this to get some feedback. Especially from Brian who wrote Spectrum in the first place.
@bgrins: It'd be great if you could quickly glance over the version of Spectrum in this patch, I've modified it a bit.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset from comment #8)
> I'm about to submit a patch that contains spectrum, bgrins's color picker
> script (http://github.com/bgrins/spectrum).
> 
> @bgrins : do you have a js test suite we could also include in mozilla's
> source code that we could run during our mochitest browser test suite

Regarding tests, there are some in the spectrum repo, but I don't think they will port well since it mostly covers features that don't exist in this version, and the API is quite different between the two.  It will probably be best to implement a new mochitest for this functionality.  Not sure if we should have one for both the picker itself and one for its integration within the rule view, or just its integration in the rule view.  FWIW, here is the existing qunit test: https://github.com/bgrins/spectrum/blob/master/test/tests.js.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 823246 [details] [diff] [review]
color picker tooltip patch V1 (no tests yet)

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

Went over spectrum.js, it is looking good.  Most of the notes have to do with trimming it down by getting rid of some unneeded cross browser compatibility code.  Overall note: we should switch the bracket / whitespace / ifelse formatting to match the conventions in the rest of the project.

::: browser/devtools/shared/widgets/Spectrum.js
@@ +57,5 @@
> +  this.element.addEventListener("click", this.onElementClick, false);
> +
> +  this.parentEl.appendChild(this.element);
> +
> +  this.slider = this.element.querySelectorAll(".spectrum-hue")[0];

querySelector("foo") instead of querySelectorAll("foo")[0] for all the declarations here.

@@ +164,5 @@
> +  onstop = onstop || function() {};
> +
> +  let doc = element.ownerDocument;
> +  let dragging = false;
> +  let offset = { };

Formatting nit: let offset = {};

@@ +168,5 @@
> +  let offset = { };
> +  let maxHeight = 0;
> +  let maxWidth = 0;
> +
> +  let duringDragEvents = {

I'd say get rid of Spectrum.addEvent/Spectrum.removeEvent and just flatten out the couple of places they are called.  So instead of keeping duringDragEvents around, just addEventListener/removeEventListener for all of them in line.  It was originally done this way because some of the events would be swapped out in a touch environment, but here we don't have to worry about it.

@@ +175,5 @@
> +    mousemove: move,
> +    mouseup: stop
> +  };
> +
> +  function prevent(e) {

This was for cross browser compat.  Can just do:
 
function prevent(e) {
  e.stopPropagation();
  e.preventDefault();
}

@@ +197,5 @@
> +      onmove.apply(element, [dragX, dragY]);
> +    }
> +  }
> +  function start(e) {
> +    let rightclick = (e.which) ? (e.which == 3) : (e.button == 2);

I believe just e.which === 3 is needed in this environment.

@@ +327,5 @@
> +
> +    this.rangeSlider.value = this.hsv[3] * 100;
> +  },
> +
> +  addChangeListener: function(listener) {

Possibly have this object be an event emitter instead of holding onto the change listener.   Then we would be able to just bind to "change" and get rid of both this.addChangeListener and this._onchange.

Comment 12

[Patrick Brosset <:pbro>]

Thanks Brian, I made those changes.

Comment 13

[Joe Walker [:jwalker]]

(In reply to Patrick Brosset from comment #8)
> @jwalker : I have kept bgrins' license header comments in the js and css
> files. Should I also add Mozilla's?

(CC: Gerv because this is about licensing)

I think we should try to avoid multiple licenses on a file wherever possible. I think our options are:

1. If Brian is can and is happy to allow Mozilla to have the files under MPLv2 then that's the best option (this probably implies that he owns the copyright to the files)

2. If we can't do that then I think the best option is to use the file under the MIT license that it's currently under.

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Joe Walker [:jwalker] from comment #13)
> (In reply to Patrick Brosset from comment #8)
> > @jwalker : I have kept bgrins' license header comments in the js and css
> > files. Should I also add Mozilla's?
> 
> (CC: Gerv because this is about licensing)
> 
> I think we should try to avoid multiple licenses on a file wherever
> possible. I think our options are:
> 
> 1. If Brian is can and is happy to allow Mozilla to have the files under
> MPLv2 then that's the best option (this probably implies that he owns the
> copyright to the files)
> 
> 2. If we can't do that then I think the best option is to use the file under
> the MIT license that it's currently under.

As the original author, I'm fine with licensing spectrum.js and spectrum.css under MPLv2.

Comment 15

[Gervase Markham [:gerv]]

(In reply to Joe Walker [:jwalker] from comment #13)
> I think we should try to avoid multiple licenses on a file wherever
> possible. 

"The force is strong with this one." :-)

> I think our options are:
> 
> 1. If Brian is can and is happy to allow Mozilla to have the files under
> MPLv2 then that's the best option (this probably implies that he owns the
> copyright to the files)
>
> 2. If we can't do that then I think the best option is to use the file under
> the MIT license that it's currently under.

If we haven't made significant changes to the files, then normal procedure is to use them under upstream's licence anyway. If we've excerpted code and/or copied in some already-MPLed code, then yes, option 1) is to be preferred if bgrins is willing and able to consent.

Gerv

Comment 16

[Joe Walker [:jwalker]]

Thanks Gerv. So a followup question for bgrins - Where do you see the 'home' of spectrum being, or in other words - is 'upstream' a meaningful concept?

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Joe Walker [:jwalker] from comment #16)
> Thanks Gerv. So a followup question for bgrins - Where do you see the 'home'
> of spectrum being, or in other words - is 'upstream' a meaningful concept?

The official home for the project is here: https://github.com/bgrins/spectrum/.  Regarding upstream, I originally made this version specifically to remove the dependencies on some external libraries and to remove a bunch of features, to the point that this file is quite different and there would be no sense in pushing or pulling changes between the two projects.

Comment 18

[Patrick Brosset <:pbro>]

Thanks for the clarification, I have added the MPLv2 license header to Spectrum.js and spectrum.css and removed the original license header.

Comment 19

[Patrick Brosset <:pbro>]

I have a patch basically ready for this, with tests too. I only need to work on one remaining use case that's failing: when editing a color included in a css declaration of the `style {...}` selector (the one coming from the current selection's style attribute).

That case fails as of today as the live preview edits the color of the style attribute, in turn triggering a markup mutation, in turn refreshing the rule view, meaning that the DOM elements that were used to open the color picker in the first place do not exist anymore.

A fix for this final case should be ready by tomorrow. I'll attach the patch then.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: teaser screenshot.png

Comment 21

[Patrick Brosset <:pbro>]

Green try build for my WIP patch: https://tbpl.mozilla.org/?tree=Try&rev=3eba404d5c75

Comment 22

[Patrick Brosset <:pbro>]

Adding a dependency on bug 929384 to make sure we have color swatches everywhere we need to, and also because that bug brings changes to how event listeners are added to things generated by the output-parser (namely, color swatches).

Comment 23

[Patrick Brosset <:pbro>]

Attachment: color picker tooltip patch V2

v2 patch ready for review. It should be more or less complete this time, with tests and all. It contains:

- Spectrum, which is the color picker developed by bgrins, simplified and customized for our needs, with our license, as discussed previously
- A new XHTML frame file used to load spectrum in an iframe and put that iframe in the tooltip's XUL panel
- New dark and light themes for the tooltips, according to the latest shorlander's mockups (http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]), with new images for the tooltip arrows.
- A new set of classes in the tooltip module, to support swatch-based tooltips (for now we only have colorpicker tooltips on color swatches, but we already planned to have gradient editor tooltips on gradient swatches, etc...)
- Some changes to the original Tooltip class, especially in terms of options management. Also, tooltips are always XUL Panels now, they can't be XUL Tooltips as before since this wasn't used. Finally, keyboard event listeners have been added to be able to close the tooltip on certain keys.
- Implementation of the color picker tooltip in the rule view panel. This should be a pretty small amount of code since most is done in the SwatchBased tooltip class.
- Tests for both spectrum itself and the color picker functionality in the rule view.

Over to you Heather for review.

Comment 24

[Patrick Brosset <:pbro>]

Here are 2 updated screenshots:

- http://people.mozilla.org/~pbrosset/tooltips/dark-color-picker.png
- http://people.mozilla.org/~pbrosset/tooltips/light-color-picker.png

Comment 25

[Patrick Brosset <:pbro>]

Also, ongoing try build https://tbpl.mozilla.org/?tree=Try&rev=29813b349267

Comment 26

[Patrick Brosset <:pbro>]

Try build is green!

Comment 27

[Heather Arthur [:harth]]

Going to review in a second here. Trying it out right now, and I love it! Thanks Patrick and Brian. Some functionality issues:

The first color I tried it on was grey so the hue slider overflowed and it was hard to tell what was going on and how to change the hue. See the screenshot here: http://i.imgur.com/pwDjmaE.png.

Single-clicking on the hue bar or gradient does not move the slider or crosshair, and I think it should. Because it's not obvious that you have to drag the slider/crosshair, and one might think it's just broken, like I did at first.

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Heather Arthur [:harth] from comment #27)

> Single-clicking on the hue bar or gradient does not move the slider or
> crosshair, and I think it should. Because it's not obvious that you have to
> drag the slider/crosshair, and one might think it's just broken, like I did
> at first.

I agree with this, I think it was an oversight from the original code that got imported.  The `start` callback in Spectrum.draggable should be calling `move(e)` right after setting offset.

Comment 29

[Heather Arthur [:harth]]

Comment on attachment 828641 [details] [diff] [review]
color picker tooltip patch V2

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

Cool. r-ing just because we'll want to fix the functionality things I mentioned in the previous comment.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +50,5 @@
> +    consumeOutsideClick: false,
> +    closeOnKeys: [27]
> +  };
> +  this.options = options || {};
> +}

'OptionsStore' name makes it sound like a generic object, but this seems very specific, just for use with Tooltips.

Maybe when creating the OptionsStore you could pass in the defaults?

@@ +150,5 @@
> +  this.panel.addEventListener("popuphidden", this._onPopupHidden, false);
> +
> +  // Listen to popupshowing to emit a shown event
> +  this._onPopupShowing = event => this.emit("showing");
> +  this.panel.addEventListener("popupshowing", this._onPopupShowing, false);

No need for these comments, code below them is pretty explicit about it.

@@ +154,5 @@
> +  this.panel.addEventListener("popupshowing", this._onPopupShowing, false);
> +
> +  // Listen to popuphidding to emit a hidden event
> +  this._onPopupHiding = event => this.emit("hiding");
> +  this.panel.addEventListener("popuphiding", this._onPopupHiding, false);

Might want to add all these listeners in a loop if you can? to avoid repeating this four times.

var events = ["shown", "hidden", "showing", "hiding"];
for (event in events) {
  this["_on" + event] = event => this.emit(event);
  this.panel.addEventListener("on" + event, this["_ on" + event], false);
}

Something like that if you think that looks better?

@@ +549,5 @@
> +  // close the tooltip by clicking out
> +  // It will also close on <escape> (27) and <enter> (13)
> +  this.tooltip = new Tooltip(doc, {
> +    consumeOutsideClick: true,
> +    closeOnKeys: [27, 13]

Instead of the magic numbers scattered in the code, maybe add some consts to the top of the file? const KEYCODE_ESC = 27

@@ +689,5 @@
> +}
> +
> +module.exports.SwatchColorPickerTooltip = SwatchColorPickerTooltip;
> +SwatchColorPickerTooltip.prototype = Object.create(SwatchBasedEditorTooltip.prototype);
> +

Heads up: Addon-SDK's heritage module has a cool thing to wrap this, so that you also don't have to write out obj.proto.funcname = ... for every function as well.

const Heritage = require("sdk/core/heritage");

SwatchColorPickerTooltip = Heritage.extend(SwatchBasedEditorTooltip.prototype, {
  funcname : ...
})

::: browser/devtools/styleinspector/rule-view.js
@@ +2049,5 @@
> +
> +      // Attach the color picker tooltip to the color swatches
> +      this._swatchSpans = this.valueSpan.querySelectorAll("." + swatchClass);
> +      if (this._swatchSpans.length) {
> +        [].forEach.call(this._swatchSpans, span => {

why [].forEach.call? not swatchSpans.forEach?

Comment 30

[Patrick Brosset <:pbro>]

> The first color I tried it on was grey so the hue slider overflowed and it
> was hard to tell what was going on and how to change the hue. See the
> screenshot here: http://i.imgur.com/pwDjmaE.png.

That's definitely a problem, I'm going to look at this issue right away.

> Single-clicking on the hue bar or gradient does not move the slider or
> crosshair, and I think it should. Because it's not obvious that you have to
> drag the slider/crosshair, and one might think it's just broken, like I did
> at first.

Yes, we should be moving the slider and crosshair on click and as Brian said, it's an oversight cause it's working fine in Spectrum's official version: bgrins.github.io/spectrum/

Comment 31

[Patrick Brosset <:pbro>]

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +50,5 @@
> > +    consumeOutsideClick: false,
> > +    closeOnKeys: [27]
> > +  };
> > +  this.options = options || {};
> > +}
> 
> 'OptionsStore' name makes it sound like a generic object, but this seems
> very specific, just for use with Tooltips.
> 
> Maybe when creating the OptionsStore you could pass in the defaults?

Done.

> @@ +150,5 @@
> > +  this.panel.addEventListener("popuphidden", this._onPopupHidden, false);
> > +
> > +  // Listen to popupshowing to emit a shown event
> > +  this._onPopupShowing = event => this.emit("showing");
> > +  this.panel.addEventListener("popupshowing", this._onPopupShowing, false);
> 
> No need for these comments, code below them is pretty explicit about it.
> @@ +154,5 @@
> > +  this.panel.addEventListener("popupshowing", this._onPopupShowing, false);
> > +
> > +  // Listen to popuphidding to emit a hidden event
> > +  this._onPopupHiding = event => this.emit("hiding");
> > +  this.panel.addEventListener("popuphiding", this._onPopupHiding, false);
> 
> Might want to add all these listeners in a loop if you can? to avoid
> repeating this four times.
> 
> var events = ["shown", "hidden", "showing", "hiding"];
> for (event in events) {
>   this["_on" + event] = event => this.emit(event);
>   this.panel.addEventListener("on" + event, this["_ on" + event], false);
> }
> 
> Something like that if you think that looks better?

Good point. Done.

> @@ +549,5 @@
> > +  // close the tooltip by clicking out
> > +  // It will also close on <escape> (27) and <enter> (13)
> > +  this.tooltip = new Tooltip(doc, {
> > +    consumeOutsideClick: true,
> > +    closeOnKeys: [27, 13]
> 
> Instead of the magic numbers scattered in the code, maybe add some consts to
> the top of the file? const KEYCODE_ESC = 27

Done

> @@ +689,5 @@
> > +}
> > +
> > +module.exports.SwatchColorPickerTooltip = SwatchColorPickerTooltip;
> > +SwatchColorPickerTooltip.prototype = Object.create(SwatchBasedEditorTooltip.prototype);
> > +
> 
> Heads up: Addon-SDK's heritage module has a cool thing to wrap this, so that
> you also don't have to write out obj.proto.funcname = ... for every function
> as well.
> 
> const Heritage = require("sdk/core/heritage");
> 
> SwatchColorPickerTooltip =
> Heritage.extend(SwatchBasedEditorTooltip.prototype, {
>   funcname : ...
> })

Cool, I didn't know. Done.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +2049,5 @@
> > +
> > +      // Attach the color picker tooltip to the color swatches
> > +      this._swatchSpans = this.valueSpan.querySelectorAll("." + swatchClass);
> > +      if (this._swatchSpans.length) {
> > +        [].forEach.call(this._swatchSpans, span => {
> 
> why [].forEach.call? not swatchSpans.forEach?

afaik querySelectorAll returns a NodeList that doesn't have Array.prototype methods, so forEach wouldn't work here. 
I just realized that a for .. of loop would work though, will switch to this.

Comment 32

[Patrick Brosset <:pbro>]

Attachment: color picker tooltip patch V3

This v3 patch addresses the points raised in the code review.
Also I have rebased.
Ongoing try: https://tbpl.mozilla.org/?tree=Try&rev=f23aaa243905

Comment 33

[Patrick Brosset <:pbro>]

Attachment: color picker tooltip patch V4

Sorry, last minute changes.
When I refactored the tooltip panel's event listeners, I created a bug that failed the tests.
I forgot to "capture" the event in the for (let event of [...]) loop.

New try build: https://tbpl.mozilla.org/?tree=Try&rev=e989b3a55a20

Comment 34

[Patrick Brosset <:pbro>]

Try is green!

Comment 35

[Heather Arthur [:harth]]

Comment on attachment 831484 [details] [diff] [review]
color picker tooltip patch V4

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

This is so awesome.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +22,5 @@
>  const BACKGROUND_IMAGE_RE = /url\([\'\"]?(.*?)[\'\"]?\)/;
> +const XHTML_NS = "http://www.w3.org/1999/xhtml";
> +const SPECTRUM_FRAME = "chrome://browser/content/devtools/spectrum-frame.xhtml";
> +const ESCAPE_KEYCODE = 27;
> +const ENTER_KEYCODE = 13;

Agh, just remembered about DomEvent's keycode constants, can just use Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE, Ci.nsIDOMKeyEvent.DOM_VK_RETURN instead of these. This should be totally fine though.

@@ +752,5 @@
>   */
>  function createTransparencyTiles(doc, parentEl) {
>    let tiles = doc.createElement("box");
> +  // For now, trying out without tiles, since our panel background is transparent
> +  // tiles.classList.add("devtools-tooltip-tiles");

Just noticed this. Related to colorpicker? It's not the end of the world, but if we're not using it we should take it out. It'll always be in the history for reference. Functions calling createTransparencyTiles() will be misleading if the function doesn't actually create tiles.

Comment 36

[Patrick Brosset <:pbro>]

Attachment: color picker tooltip patch V5

Thanks Heather for the review.
I have addressed the 2 points you raised in this new patch:

- removed the unnecessary transparency tiles creation function, and while I was at it, I also removed a bunch of tooltip content functions that we weren't using at all.

- removed the enter and escape numbers and used the Ci.nsIDOMKeyEvent constants instead.

Also, I realized that the changes I made to the CSS of the panel weren't applying to the Shader Editor error tooltips, so I made sure it did and tweaked a bit these tooltips' CSS styling after discussing with Victor.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

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

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6709808f07d8