Patrick Brosset <:pbro> Assignee
	Description • 8 years ago

In bug 1259121, we're removing XUL from the devtools toolbox.

The eyedropper tool uses a XUL panel to be able to appear as a floating window on top of the browser element.
There is no equivalent in HTML that we could use.

It seems like constraining the eyedropper to inside the content frame would be acceptable. After all, the eyedropper's goal is to grab colors from the page.
If we agreed that this was ok, then we could turn the eyedropper into a highlighter.
Especially because our highlighter API now supports canvas (which the eyedropper uses to magnify the page).

	Patrick Brosset <:pbro> Assignee
	Comment 1 • 8 years ago

Filter on CLIMBING SHOES.

	Brian Grinstead [:bgrins]
	Comment 2 • 8 years ago

(In reply to Patrick Brosset [:pbro] from comment #0)
> It seems like constraining the eyedropper to inside the content frame would
> be acceptable. After all, the eyedropper's goal is to grab colors from the
> page.
> If we agreed that this was ok, then we could turn the eyedropper into a
> highlighter.
> Especially because our highlighter API now supports canvas (which the
> eyedropper uses to magnify the page).

Sounds good to me

	Patrick Brosset <:pbro> Assignee
	Comment 4 • 8 years ago

I am working on a patch for this.

	Patrick Brosset <:pbro> Assignee
	Comment 5 • 8 years ago

One note about backward compatibility:

My plan is to port the eye-dropper code to a highlighter. Highlighters are instantiated by the CustomHighlighterActor which means that I'm going to move the whole tool from the client to the server.

Once done, what should happen if you connect a new toolbox to an old backend?
If we want to maintain backward compatibility for this tool, then we have to keep both tools in: the old (XUL) eyedropper, and the new (highlighter-based) one.

Backward compatibility is mostly important when connecting to remove devices, where the eye-dropper isn't available today anyway, so I would vote for deleting the old eye-dropper, and not handling backward compatibility at all (other than just hiding the feature if the actor doesn't exist).

@Brian: how does that sound?

	Brian Grinstead [:bgrins]
	Comment 7 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #5)
> One note about backward compatibility:
> 
> My plan is to port the eye-dropper code to a highlighter. Highlighters are
> instantiated by the CustomHighlighterActor which means that I'm going to
> move the whole tool from the client to the server.
> 
> Once done, what should happen if you connect a new toolbox to an old backend?
> If we want to maintain backward compatibility for this tool, then we have to
> keep both tools in: the old (XUL) eyedropper, and the new
> (highlighter-based) one.
> 
> Backward compatibility is mostly important when connecting to remove
> devices, where the eye-dropper isn't available today anyway, so I would vote
> for deleting the old eye-dropper, and not handling backward compatibility at
> all (other than just hiding the feature if the actor doesn't exist).
> 
> @Brian: how does that sound?

I'm not worried about backwards compat for the eyedropper

	Patrick Brosset <:pbro> Assignee
	Comment 10 • 8 years ago

This is mostly porting code from the XUL eye-dropper over to a new
highlighter file.
While I have tested this locally, this highlighter isn't yet used
in devtools.
This will come in later patches.

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

	Patrick Brosset <:pbro> Assignee
	Comment 11 • 8 years ago

The inspector actor now has a new method that can be used to pick a
color from the page. This method just instantiates the eye-dropper
highlighter and shows it on the page.
The method doesn't return the color because this requires user interaction.
Instead, it returns immediately and an event is sent later, when the user
has selected a color or escaped.

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

	Patrick Brosset <:pbro> Assignee
	Comment 12 • 8 years ago

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

	Patrick Brosset <:pbro> Assignee
	Comment 13 • 8 years ago

The color-picker tooltip now uses the new eye-dropper highlighter, by
using the pickColorFromPage inspector method instead of the XUL-based
eye-dropper tool.

Telemetry hasn't yet been re-implemented in the new eye-dropper highlighter
so the telemetry-related test code has been removed for now. This will be
added again in a later commit.

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

	Patrick Brosset <:pbro> Assignee
	Comment 14 • 8 years ago

This moves the eyedropper button from the toolbar into the inspector,
therefore the old eyedropper command isn't needed anymore,
or at least not as it was.
The button in the inspector simply uses the pickColorFromPage inspector
actor method.
And to preserve a eyedropper gcli command, a new simpler one was added.

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

	Patrick Brosset <:pbro> Assignee
	Comment 15 • 8 years ago

Removes some old eye-dropper related code in browser.js which was not
used anymore since the devtools menu are added dynamically now.
And replaces the code in devtools/client/menus.js to call the gcli
eyedropper command now instead of the old eyedropper tool.

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

	Patrick Brosset <:pbro> Assignee
	Comment 16 • 8 years ago

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

	Patrick Brosset <:pbro> Assignee
	Comment 17 • 8 years ago

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 18 • 8 years ago

Comment on attachment 8761602 [details]
Bug 1262439 - 8 - Re-implement telemetry in the eye-dropper;

https://reviewboard.mozilla.org/r/58720/#review55684

	Alexandre Poirot [:ochameau]
	Comment 20 • 8 years ago

Comment on attachment 8761600 [details]
Bug 1262439 - 6 - Update the eye-dropper devtools menu entry to use the new eye-dropper;

https://reviewboard.mozilla.org/r/58716/#review56736

::: devtools/client/menus.js:154
(Diff revision 1)
> -                                                copyOnSelect: true });
> -      eyedropper.open();
> +
> +      CommandUtils.createRequisition(target, {
> +        environment: CommandUtils.createEnvironment({target})
> +      }).then(requisition => {
> +        requisition.updateExec("eyedropper");
> +      }, e => console.error(e));

I imagine there is a good reason to involve gcli codebase here?
Or could we factor out the code from inspector-commands.js:
```
let env = new HighlighterEnvironment();
    env.initFromWindow(environment.window);
    let eyeDropper = new EyeDropper(env);

    eyeDropper.show(environment.document.documentElement, {copyOnSelect: true});

    eyeDropper.once("hidden", () => {
      eyeDropper.destroy();
      env.destroy();
```
into a third party module used from here and inspector-commands.js?

	Alexandre Poirot [:ochameau]
	Comment 21 • 8 years ago

Comment on attachment 8761601 [details]
Bug 1262439 - 7 - Delete the old eyedropper implementation;

https://reviewboard.mozilla.org/r/58718/#review56738

	Brian Grinstead [:bgrins]
	Comment 22 • 8 years ago

Comment on attachment 8761596 [details]
Bug 1262439 - 2 - Add a new pickColor inspector method to use the new eye-dropper highlighter;

https://reviewboard.mozilla.org/r/58708/#review57144

	Brian Grinstead [:bgrins]
	Comment 23 • 8 years ago

Comment on attachment 8761598 [details]
Bug 1262439 - 4 - Use the new eye-dropper highlighter in the color-picker tooltip;

https://reviewboard.mozilla.org/r/58712/#review57146

Nice

	Brian Grinstead [:bgrins]
	Comment 24 • 8 years ago

Comment on attachment 8761599 [details]
Bug 1262439 - 5 - Move eyedropper btn to inspector and update gcli command;

https://reviewboard.mozilla.org/r/58714/#review57148

::: devtools/client/inspector/inspector-panel.js:416
(Diff revision 1)
> +    this.addNode = this.addNode.bind(this);
> +    this.addNodeButton = this.panelDoc.getElementById("inspector-element-add-button");
> +    this.addNodeButton.addEventListener("click", this.addNode);
> +
> +    // Setup the eye-dropper icon.
> +    this.toolbox.target.actorHasMethod("inspector", "pickColorFromPage").then(value => {

I wish this method was sync, or at least we could somehow have the toolbox make it sync before starting up other tools so they could use it as sync.  But I'm not sure if we can answer these questions before the actors are set up.  Either way, not your problem here

::: devtools/client/inspector/inspector-panel.js:1139
(Diff revision 1)
> +    this.stopEyeDropperListeners();
> +  },
> +
> +  showEyeDropper: function () {
> +    this.eyeDropperButton.setAttribute("checked", "true");
> +    this.inspector.pickColorFromPage({copyOnSelect: true}).catch(e => console.error(e));

What's the case when this would reject?

::: devtools/client/inspector/inspector-panel.js:1145
(Diff revision 1)
> +    this.startEyeDropperListeners();
> +  },
> +
> +  hideEyeDropper: function () {
> +    this.eyeDropperButton.removeAttribute("checked");
> +    this.inspector.cancelPickColorFromPage().catch(e => console.error(e));

Ditto

::: devtools/client/inspector/inspector.xul:165
(Diff revision 1)
>          <textbox id="inspector-searchbox"
>            type="search"
>            timeout="50"
>            class="devtools-searchinput"
>            placeholder="&inspectorSearchHTML.label3;"/>
> +        <html:button id="inspector-eyedropper-toggle"

Do you have UI review for adding the button to the inspector?

::: devtools/server/actors/highlighters/eye-dropper.js:176
(Diff revision 1)
>      pageListenerTarget.removeEventListener("DOMMouseScroll", this);
>  
>      this.getElement("root").setAttribute("hidden", "true");
>      this.getElement("root").removeAttribute("drawn");
> +
> +    this.emit("hidden");

Maybe this line should move into an earlier patch

	Matteo Ferretti [:zer0] [:matteo]
	Comment 26 • 8 years ago

https://reviewboard.mozilla.org/r/58706/#review58364

I reviewed the highlighter's code, but I wasn't able to applied the patch, it probably needs to be merged and rebased.

So far it's r+, but I'd like to test the highlighter in action: could you please ping me back when you've rebased the patch in order to test it? I think I've spotted a couple of potential issues that I would like to check.

Thanks!

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-clipboard.js:17
(Diff revision 1)
> +add_task(function* () {
> +  let helper = yield openInspectorForURL(TEST_URI)
> +               .then(getHighlighterHelperFor(HIGHLIGHTER_TYPE));
> +  helper.prefix = ID;
> +
> +  let {show, testActor, finalize} = helper;

One purpose of the `getHighligherHelperFor` function, was actually to incapsulate the `testActor`; so I would rather prefer avoding to expose it. So, I would suggest to add a method / object to synthesize the keystroke, like we already have for the mouse events – it will be also more consistent.
You could also expose directly the `synthesizeKey` – I think we still have `synthesizeMouse` also, even after the `mouse` proxy.

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-clipboard.js:23
(Diff revision 1)
> +
> +  info("Show the eyedropper with the copyOnSelect option");
> +  yield show("html", {copyOnSelect: true});
> +
> +  info("Make sure to wait until the eyedropper is done taking a screenshot of the page");
> +  yield waitForElementAttributeSet("root", "drawn", helper);

That's interesting. Even if the `poll` approach is something we should avoid, and maybe we could find a way to propagate the mutation from the highlighters in a different way, it could be a nice addition in the `helper` having a function that notifies when an attribute is set, removed, or changed.

::: devtools/client/inspector/test/head.js:584
(Diff revision 1)
>  
>      return {
>        set prefix(value) {
>          prefix = value;
>        },
> +      get testActor() {

As mentioned above, I would rather exposing from the helper what is needed, instead of the whole object.

::: devtools/client/performance/modules/widgets/graphs.js:21
(Diff revision 1)
>  const { CanvasGraphUtils } = require("devtools/client/shared/widgets/Graphs");
>  
>  const promise = require("promise");
>  const EventEmitter = require("devtools/shared/event-emitter");
>  
> -const { colorUtils } = require("devtools/client/shared/css-color");
> +const { colorUtils } = require("devtools/shared/css-color");

it seems to me we have a lot of module prefixed as `css-` something (e.g. `css-color`, `css-lexer`, etc); why don't we just create a subfolder `css` under `shared`? So it will be  `devtools/shared/css/color` and we'll have everything css related (and shared) under the same folder. I'm not suggesting to do it right now – we could file a new bug for that, in case.

	Patrick Brosset <:pbro> Assignee
	Comment 27 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #20)
> Comment on attachment 8761600 [details]
> Bug 1262439 - 6 - Update the eye-dropper devtools menu entry to use the new
> eye-dropper;
> 
> https://reviewboard.mozilla.org/r/58716/#review56736
> 
> ::: devtools/client/menus.js:154
> (Diff revision 1)
> > -                                                copyOnSelect: true });
> > -      eyedropper.open();
> > +
> > +      CommandUtils.createRequisition(target, {
> > +        environment: CommandUtils.createEnvironment({target})
> > +      }).then(requisition => {
> > +        requisition.updateExec("eyedropper");
> > +      }, e => console.error(e));
> 
> I imagine there is a good reason to involve gcli codebase here?
> Or could we factor out the code from inspector-commands.js:
> ```
> let env = new HighlighterEnvironment();
>     env.initFromWindow(environment.window);
>     let eyeDropper = new EyeDropper(env);
> 
>     eyeDropper.show(environment.document.documentElement, {copyOnSelect:
> true});
> 
>     eyeDropper.once("hidden", () => {
>       eyeDropper.destroy();
>       env.destroy();
> ```
> into a third party module used from here and inspector-commands.js?
HighlighterEnvironment and EyeDropper are classes that can only be instantiated on the server with a reference to the content window object. That's why the gcli command is set to run on the server only. I believe we don't have access to this in menus.js, we only get a reference to the selected tab but not to its content window, since the tab could be remote.

	Patrick Brosset <:pbro> Assignee
	Comment 28 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #24)
> ::: devtools/client/inspector/inspector-panel.js:1139
> (Diff revision 1)
> > +    this.stopEyeDropperListeners();
> > +  },
> > +
> > +  showEyeDropper: function () {
> > +    this.eyeDropperButton.setAttribute("checked", "true");
> > +    this.inspector.pickColorFromPage({copyOnSelect: true}).catch(e => console.error(e));
> 
> What's the case when this would reject?
> 
> ::: devtools/client/inspector/inspector-panel.js:1145
> (Diff revision 1)
> > +    this.startEyeDropperListeners();
> > +  },
> > +
> > +  hideEyeDropper: function () {
> > +    this.eyeDropperButton.removeAttribute("checked");
> > +    this.inspector.cancelPickColorFromPage().catch(e => console.error(e));
> 
> Ditto
pickColorFromPage and cancelPickColorFromPage can't reject, but I've added these rejections handlers to catch any protocol error and avoid unhandled promise rejections.

> ::: devtools/client/inspector/inspector.xul:165
> (Diff revision 1)
> >          <textbox id="inspector-searchbox"
> >            type="search"
> >            timeout="50"
> >            class="devtools-searchinput"
> >            placeholder="&inspectorSearchHTML.label3;"/>
> > +        <html:button id="inspector-eyedropper-toggle"
> 
> Do you have UI review for adding the button to the inspector?
Good point, I'll prepare a try build and ping Helen for a UI review.

> ::: devtools/server/actors/highlighters/eye-dropper.js:176
> (Diff revision 1)
> >      pageListenerTarget.removeEventListener("DOMMouseScroll", this);
> >  
> >      this.getElement("root").setAttribute("hidden", "true");
> >      this.getElement("root").removeAttribute("drawn");
> > +
> > +    this.emit("hidden");
> 
> Maybe this line should move into an earlier patch
Done.

	Patrick Brosset <:pbro> Assignee
	Comment 29 • 8 years ago

Comment on attachment 8761595 [details]
Bug 1262439 - 1 - Introduce a new eye-dropper highlighter;

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

	Patrick Brosset <:pbro> Assignee
	Comment 30 • 8 years ago

Comment on attachment 8761596 [details]
Bug 1262439 - 2 - Add a new pickColor inspector method to use the new eye-dropper highlighter;

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

	Patrick Brosset <:pbro> Assignee
	Comment 31 • 8 years ago

Comment on attachment 8761597 [details]
Bug 1262439 - 3 - inspector and eye-dropper eslint cleanup;

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

	Patrick Brosset <:pbro> Assignee
	Comment 32 • 8 years ago

Comment on attachment 8761598 [details]
Bug 1262439 - 4 - Use the new eye-dropper highlighter in the color-picker tooltip;

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

	Patrick Brosset <:pbro> Assignee
	Comment 33 • 8 years ago

Comment on attachment 8761599 [details]
Bug 1262439 - 5 - Move eyedropper btn to inspector and update gcli command;

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

	Patrick Brosset <:pbro> Assignee
	Comment 34 • 8 years ago

Comment on attachment 8761600 [details]
Bug 1262439 - 6 - Update the eye-dropper devtools menu entry to use the new eye-dropper;

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

	Patrick Brosset <:pbro> Assignee
	Comment 35 • 8 years ago

Comment on attachment 8761601 [details]
Bug 1262439 - 7 - Delete the old eyedropper implementation;

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

	Patrick Brosset <:pbro> Assignee
	Comment 36 • 8 years ago

Comment on attachment 8761602 [details]
Bug 1262439 - 8 - Re-implement telemetry in the eye-dropper;

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

	Patrick Brosset <:pbro> Assignee
	Comment 37 • 8 years ago

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #26)
> https://reviewboard.mozilla.org/r/58706/#review58364
> 
> I reviewed the highlighter's code, but I wasn't able to applied the patch,
> it probably needs to be merged and rebased.
> 
> So far it's r+, but I'd like to test the highlighter in action: could you
> please ping me back when you've rebased the patch in order to test it? I
> think I've spotted a couple of potential issues that I would like to check.
Done. I've just rebased and uploaded a new version for this patch.

	Patrick Brosset <:pbro> Assignee
	Comment 38 • 8 years ago

Helen, could you do a ui-review for this change please? Some info about the change:

- the eye-dropper itself has not changed at all, I have just ported the code over from its XUL implementation into a highlighter,
- being a highlighter, it means that the eye-dropper is now constrained to the content page, which we discussed in comment 2,
- the eye-dropper can still be accessed via: the devtools menu, a gcli command, the color-picker in the rule-view, nothing changed here,
- one thing has changed however: the eye-dropper is *not* a toolbar button anymore, which means it is not toggleable from the options panel anymore. Instead, the icon is inside the inspector toolbar now. This is as per bug 1260225.

You can see all 4 ways to activate the eye-dropper in the GIF. And you can also download a build when this try push succeeds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=152105e45d4e

	Matteo Ferretti [:zer0] [:matteo]
	Comment 39 • 8 years ago

Hi Patrick, as mentioned before on vidyo here the screenshot of the tool with the page zoomed. As you can see, the tool is zoomed too – where it shouldn't –, and also it doesn't point to the right coordinates in the image on the buffer.

I have to say, zoom is a bit tricky, also because it changes media query and potentially the whole aspect of the page: if zoomed, the image displayed could be for HiDPI instead of 1x. And so on. So, it could be worthy to actually get a new screenshot of the page if we detect a zoom, but I'll let you to decide that.

	Patrick Brosset <:pbro> Assignee
	Comment 40 • 8 years ago

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #39)
> Created attachment 8767236 [details]
> eye dropper with page zoomed
> 
> Hi Patrick, as mentioned before on vidyo here the screenshot of the tool
> with the page zoomed. As you can see, the tool is zoomed too – where it
> shouldn't –, and also it doesn't point to the right coordinates in the image
> on the buffer.
> 
> I have to say, zoom is a bit tricky, also because it changes media query and
> potentially the whole aspect of the page: if zoomed, the image displayed
> could be for HiDPI instead of 1x. And so on. So, it could be worthy to
> actually get a new screenshot of the page if we detect a zoom, but I'll let
> you to decide that.
I think you're right, on zoom, we should refresh the screenshot of the page so that the eyedropper gets the right pixels at the right place.
Either this, or we disable zoom somehow.

	Matteo Ferretti [:zer0] [:matteo]
	Comment 41 • 8 years ago

Comment on attachment 8761595 [details]
Bug 1262439 - 1 - Introduce a new eye-dropper highlighter;

https://reviewboard.mozilla.org/r/58706/#review58608

Note: apparently I have those comments as draft since the last review, my apologies! I'm setting r- because the zooming issue; request a new review once it's fixed – or disabled somehow as you mentioned, like you've done with the scroll.

::: devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-clipboard.js:27
(Diff revisions 1 - 2)
>    info("Make sure to wait until the eyedropper is done taking a screenshot of the page");
>    yield waitForElementAttributeSet("root", "drawn", helper);
>  
>    yield waitForClipboard(() => {
>      info("Activate the eyedropper so the background color is copied");
> -    testActor.synthesizeKey({key: "VK_RETURN", options: {}});
> +    let generateKey = synthesizeKey({key: "VK_RETURN", options: {}});

Is there any reason why you're doing that instead of `yield synthesizeKey` as below?

::: devtools/client/inspector/test/head.js:605
(Diff revisions 1 - 2)
>          options = Object.assign({selector: ":root"}, options);
>          yield testActor.synthesizeMouse(options);
>        },
>  
> +      synthesizeKey: function* (options) {
> +        console.log("1------------------------------------", options)

I guess they're leftovers?

::: devtools/server/actors/highlighters.css:443
(Diff revision 2)
> +}
> +
> +:-moz-native-anonymous .eye-dropper-color-value {
> +  text-shadow: 1px 1px 1px #fff;
> +  flex-grow: 1;
> +  text-align: center;

That should be probably `text-align: left` (the current eye dropper has the color value aligned to the left, close to the color's preview).

::: devtools/server/actors/highlighters/eye-dropper.js:248
(Diff revision 2)
> +    let sx = x - (zoomedWidth / 2);
> +    let sy = y - (zoomedHeight / 2);
> +    let sw = zoomedWidth;
> +    let sh = zoomedHeight;
> +
> +    this.ctx.drawImage(this.pageImage, sx, sy, sw, sh, 0, 0, width, height);

As mentioned in the comment with the screenshot attached about the zoom, we should considering the page's zoom here as well.

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 42 • 8 years ago

Comment on attachment 8767119 [details]
eye-dropper.gif

This work looks great! Looking over the build it all seems to work as I would expect it to, which is awesome.

In bug 1260225 we discussed that moving these icons might cause the same visual-clutter problem we had in the toolbar before, which is my only holdup about this patch—however, I think this patch isn't the place to solve those particular issues. I'll attach a visual to bug 1260225 and we can discuss follow-ups in there.

	Patrick Brosset <:pbro> Assignee
	Comment 43 • 8 years ago

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #41)
> I'm setting r- because the zooming issue; request a new review
> once it's fixed – or disabled somehow as you mentioned, like you've done
> with the scroll.
I spent some time on this. One decision I took was that I couldn't just disable the zoom, since the page might be zoomed in before the tool is started. So at least I need to make sure it works well when the page is zoomed in or out.
There are 3 things to take care of:

- the screenshot of the page should be correct when the page is zoomed (which was already fine, the function that takes the screenshot already accounted for this), and should be re-taken when the zoom changes (we can make this better, but for now I just hide/show the tool to reset it, I don't expect this to ever happen much, so I don't think this requires spending too much time on this),

- the eyedropper should show the right magnified area when it moves over the page: this was the biggest problem, but I basically just needed to multiply by the current page zoom in the right places. No big deal,

- the eyedropper itself shouldn't be zoomed. I decided to leave this for later, because I couldn't find a satisfying solution that looked good and didn't take too much time. We have a handy 'scaleRootElement' in highlighters/utils/markup.js that is made for this but, although it worked, made the eye-dropper look really really bad. Blurry or pixelated depending on the zoom level.
So, I preferred to keep it nice looking, even if it got zoomed in with the page.
I consider this not too much of a problem, and ok to solve in a follow-up bug if needed.

> :::
> devtools/client/inspector/test/browser_inspector_highlighter-eyedropper-
> clipboard.js:27
> (Diff revisions 1 - 2)
> >    info("Make sure to wait until the eyedropper is done taking a screenshot of the page");
> >    yield waitForElementAttributeSet("root", "drawn", helper);
> >  
> >    yield waitForClipboard(() => {
> >      info("Activate the eyedropper so the background color is copied");
> > -    testActor.synthesizeKey({key: "VK_RETURN", options: {}});
> > +    let generateKey = synthesizeKey({key: "VK_RETURN", options: {}});
> 
> Is there any reason why you're doing that instead of `yield synthesizeKey`
> as below?
Yes, that's because this code isn't inside a generator function itself, so yield won't work. So instead, I need to execute the generator with next().

> ::: devtools/client/inspector/test/head.js:605
> (Diff revisions 1 - 2)
> >          options = Object.assign({selector: ":root"}, options);
> >          yield testActor.synthesizeMouse(options);
> >        },
> >  
> > +      synthesizeKey: function* (options) {
> > +        console.log("1------------------------------------", options)
> 
> I guess they're leftovers?
Thanks, removed.

> ::: devtools/server/actors/highlighters.css:443
> (Diff revision 2)
> > +}
> > +
> > +:-moz-native-anonymous .eye-dropper-color-value {
> > +  text-shadow: 1px 1px 1px #fff;
> > +  flex-grow: 1;
> > +  text-align: center;
> 
> That should be probably `text-align: left` (the current eye dropper has the
> color value aligned to the left, close to the color's preview).
This actually was intentional, I thought it looked weird to have the text left-aligned, with all this empty space to the right.
So I centered the text, but didn't want to center the color preview since it would have moved constantly as the color text changed.

> ::: devtools/server/actors/highlighters/eye-dropper.js:248
> (Diff revision 2)
> > +    let sx = x - (zoomedWidth / 2);
> > +    let sy = y - (zoomedHeight / 2);
> > +    let sw = zoomedWidth;
> > +    let sh = zoomedHeight;
> > +
> > +    this.ctx.drawImage(this.pageImage, sx, sy, sw, sh, 0, 0, width, height);
> 
> As mentioned in the comment with the screenshot attached about the zoom, we
> should considering the page's zoom here as well.
Yep, I believe this works fine now.

	Patrick Brosset <:pbro> Assignee
	Comment 44 • 8 years ago

Comment on attachment 8761595 [details]
Bug 1262439 - 1 - Introduce a new eye-dropper highlighter;

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

	Patrick Brosset <:pbro> Assignee
	Comment 45 • 8 years ago

Comment on attachment 8761596 [details]
Bug 1262439 - 2 - Add a new pickColor inspector method to use the new eye-dropper highlighter;

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

	Patrick Brosset <:pbro> Assignee
	Comment 46 • 8 years ago

Comment on attachment 8761597 [details]
Bug 1262439 - 3 - inspector and eye-dropper eslint cleanup;

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

	Patrick Brosset <:pbro> Assignee
	Comment 47 • 8 years ago

Comment on attachment 8761598 [details]
Bug 1262439 - 4 - Use the new eye-dropper highlighter in the color-picker tooltip;

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

	Patrick Brosset <:pbro> Assignee
	Comment 48 • 8 years ago

Comment on attachment 8761599 [details]
Bug 1262439 - 5 - Move eyedropper btn to inspector and update gcli command;

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

	Patrick Brosset <:pbro> Assignee
	Comment 49 • 8 years ago

Comment on attachment 8761600 [details]
Bug 1262439 - 6 - Update the eye-dropper devtools menu entry to use the new eye-dropper;

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

	Patrick Brosset <:pbro> Assignee
	Comment 50 • 8 years ago

Comment on attachment 8761601 [details]
Bug 1262439 - 7 - Delete the old eyedropper implementation;

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

	Patrick Brosset <:pbro> Assignee
	Comment 51 • 8 years ago

Comment on attachment 8761602 [details]
Bug 1262439 - 8 - Re-implement telemetry in the eye-dropper;

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

	Patrick Brosset <:pbro> Assignee
	Comment 52 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #44)
> Comment on attachment 8761595 [details]
> Bug 1262439 - 1 - Introduce a new eye-dropper highlighter;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/58706/diff/2-3/
Matteo: see comment 43 about the changes made in this new version.

	Patrick Brosset <:pbro> Assignee
	Comment 53 • 8 years ago

Green TRY push for all 8 parts: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f1b3fa19309&group_state=expanded
(I've corrected the ESLint failures since that push).

	Patrick Brosset <:pbro> Assignee
	Comment 54 • 8 years ago

Inspector bug triage (filter on CLIMBING SHOES).

	Patrick Brosset <:pbro> Assignee
	Comment 55 • 8 years ago

Comment on attachment 8761595 [details]
Bug 1262439 - 1 - Introduce a new eye-dropper highlighter;

Matteo is off sick at the moment, and probably has better things to do (resting) than reviewing code changes!

	Matteo Ferretti [:zer0] [:matteo]
	Comment 56 • 8 years ago

Comment on attachment 8761595 [details]
Bug 1262439 - 1 - Introduce a new eye-dropper highlighter;

https://reviewboard.mozilla.org/r/58706/#review61662

There are a couple of nits and one thing about zoom / DPI; but there is a blocker: it seems that the eye dropper is not well positioned when the document contains iframe – I tested on codepen.io, that behaviors happens especially when moving between the frame below and the upper frames; probaby it depends by the `e.pageX` / `e.pageY` thingy; I think it should be fixed before land.

Do we have already a follow up bug to trace the fact that the highlighter is zoomed with the content? Maybe we won't be able to fix in the near term, but it's still a regression, and it would be useful to let people knows that we're aware about that.

I'm giving r+, but only once the iframe thing is fixed. I'll let you decide if you want to have another round of review after or not - depends by type / amount of code you're going to write.

	Matteo Ferretti [:zer0] [:matteo]
	Comment 57 • 8 years ago

https://reviewboard.mozilla.org/r/58706/#review62510

Okay, those comments shoul be everything I wrote before; hope I didn't forget anything! The most important, as said, was the dppx ones – that can also be done in a follow up bug, if you think it's not a big deal.

::: devtools/client/eyedropper/eyedropper.js:11
(Diff revisions 1 - 3)
>  const {rgbToHsl, rgbToColorName} = require("devtools/shared/css-color").colorUtils;
> +const {Cc, Ci} = require("chrome");
>  const Telemetry = require("devtools/client/shared/telemetry");
> -const {EventEmitter} = Cu.import("resource://devtools/shared/event-emitter.js");
> +const EventEmitter = require("devtools/shared/event-emitter");
>  const promise = require("promise");
> +const defer = require("devtools/shared/defer");

I didn't notice that before; why having two different modules for promises? One should be enough (e.g. you can have keep `promise` and having `promise.defer`, or you can keep `defer` and creates promises to resolve immediately instead of `promise.resolve`).

::: devtools/server/actors/highlighters/eye-dropper.js:146
(Diff revisions 2 - 3)
>      let {pageListenerTarget} = this.highlighterEnv;
>      pageListenerTarget.addEventListener("mousemove", this);
>      pageListenerTarget.addEventListener("click", this);
>      pageListenerTarget.addEventListener("keydown", this);
>      pageListenerTarget.addEventListener("DOMMouseScroll", this);
> +    pageListenerTarget.addEventListener("FullZoomChange", this);

That handles only the zoom, where I believe we should handles the change of DPPX in any case: that also includes if the user is moving windows between monitor with different display density, and / or if it's changes the settings of the current one (definitely possible in OS X, and also in Windows).

A code like that should be able to deal with both cases (zoom and display density):

```
    function onDPRChange(window) {
        let dpr = window.devicePixelRatio;
        let mql = window.matchMedia(`(resolution:${dpr}dppx)`);

        function listener() {
          // do your stuff here
          // ...

          // set again the trigger for a next change
          onDPRChange(window);
        };

        mql.addListener(listener);
    }

    // set for the first time
    onDPRChange(highlighterEnv.window);
```

That's just the idea, you want probably to store the `mql` and the `listener` somewhere, in order to remove them.

I don't mind, however, if you want to do that in a follow up bug, we just need to remember that we have to deal with that.

::: devtools/server/actors/highlighters.css:454
(Diff revision 3)
> +}
> +
> +:-moz-native-anonymous .eye-dropper-color-value {
> +  text-shadow: 1px 1px 1px #fff;
> +  flex-grow: 1;
> +  text-align: center;

As said previously, I believe that should be aligned to the left instead of centered, to be closest to the color preview; as we have now.

	Patrick Brosset <:pbro> Assignee
	Comment 58 • 8 years ago

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #57)
> https://reviewboard.mozilla.org/r/58706/#review62510
> ::: devtools/client/eyedropper/eyedropper.js:11
> (Diff revisions 1 - 3)
> >  const {rgbToHsl, rgbToColorName} = require("devtools/shared/css-color").colorUtils;
> > +const {Cc, Ci} = require("chrome");
> >  const Telemetry = require("devtools/client/shared/telemetry");
> > -const {EventEmitter} = Cu.import("resource://devtools/shared/event-emitter.js");
> > +const EventEmitter = require("devtools/shared/event-emitter");
> >  const promise = require("promise");
> > +const defer = require("devtools/shared/defer");
> 
> I didn't notice that before; why having two different modules for promises?
> One should be enough (e.g. you can have keep `promise` and having
> `promise.defer`, or you can keep `defer` and creates promises to resolve
> immediately instead of `promise.resolve`).
Yes but this is in the old eyedropper that I'm replacing, so this code will be deleted by another patch.

> ::: devtools/server/actors/highlighters/eye-dropper.js:146
> (Diff revisions 2 - 3)
> >      let {pageListenerTarget} = this.highlighterEnv;
> >      pageListenerTarget.addEventListener("mousemove", this);
> >      pageListenerTarget.addEventListener("click", this);
> >      pageListenerTarget.addEventListener("keydown", this);
> >      pageListenerTarget.addEventListener("DOMMouseScroll", this);
> > +    pageListenerTarget.addEventListener("FullZoomChange", this);
> 
> That handles only the zoom, where I believe we should handles the change of
> DPPX in any case: that also includes if the user is moving windows between
> monitor with different display density, and / or if it's changes the
> settings of the current one (definitely possible in OS X, and also in
> Windows).
> 
> A code like that should be able to deal with both cases (zoom and display
> density):
> 
> ```
>     function onDPRChange(window) {
>         let dpr = window.devicePixelRatio;
>         let mql = window.matchMedia(`(resolution:${dpr}dppx)`);
> 
>         function listener() {
>           // do your stuff here
>           // ...
> 
>           // set again the trigger for a next change
>           onDPRChange(window);
>         };
> 
>         mql.addListener(listener);
>     }
> 
>     // set for the first time
>     onDPRChange(highlighterEnv.window);
> ```
> 
> That's just the idea, you want probably to store the `mql` and the
> `listener` somewhere, in order to remove them.
> 
> I don't mind, however, if you want to do that in a follow up bug, we just
> need to remember that we have to deal with that.
Thanks for the code! I'll take a look at the problem.

> ::: devtools/server/actors/highlighters.css:454
> (Diff revision 3)
> > +}
> > +
> > +:-moz-native-anonymous .eye-dropper-color-value {
> > +  text-shadow: 1px 1px 1px #fff;
> > +  flex-grow: 1;
> > +  text-align: center;
> 
> As said previously, I believe that should be aligned to the left instead of
> centered, to be closest to the color preview; as we have now.
Helen: here are 2 screenshots to compare the text alignment:
https://dl.dropboxusercontent.com/u/714210/eyedropper-center-align.png
https://dl.dropboxusercontent.com/u/714210/eyedropper-left-align.png
Let me know what you think.

One thing: we need to have a rather large container for the color value because you can change the unit to rgb, hsl or hex, and so the width that the text requires varies.
That's why I originally wanted to center the text, so it'd look better. Matteo makes the point that it should be left aligned, so it's closer to the little color square box.

	Patrick Brosset <:pbro> Assignee
	Comment 59 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #58)
> > As said previously, I believe that should be aligned to the left instead of
> > centered, to be closest to the color preview; as we have now.
> Helen: here are 2 screenshots to compare the text alignment:
> https://dl.dropboxusercontent.com/u/714210/eyedropper-center-align.png
> https://dl.dropboxusercontent.com/u/714210/eyedropper-left-align.png
> Let me know what you think.
> 
> One thing: we need to have a rather large container for the color value
> because you can change the unit to rgb, hsl or hex, and so the width that
> the text requires varies.
> That's why I originally wanted to center the text, so it'd look better.
> Matteo makes the point that it should be left aligned, so it's closer to the
> little color square box.
I realize only now that my center-aligned version isn't center-aligned at all! It's aligned to the center without its container, but because the color preview is there, that pushes it to the right. I will at least fix that.

	Patrick Brosset <:pbro> Assignee
	Comment 60 • 8 years ago

Summary of the remaining issues in the last review comments:

- offset when hovering over iframes: I fixed that
- text centering in the label: I kept it centered (fixed the weird offset I had), and pinged Helen for a final decision
- adapt to changing dPR: filed bug 1288387
- adapt the UI on page zoom: filed bug 1288389

So I'll run this by try again and then land these patches.
This has been opened for a long time, and I'd like to land it. The follow-ups seem minor enough to me.

	Patrick Brosset <:pbro> Assignee
	Comment 61 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba41dadc687&group_state=expanded
Try is a christmas tree, but everything looks very unrelated. And I've seen the nearly permaorange browser_dbg_sources-webext-contentscript.js yesterday on another unrelated try push. After talking to Tomcat, it seems related to bug 1285063.
So, I'm attempting to land this in fx-team. We'll see what happens.

	Pulsebot
	Comment 62 • 8 years ago

Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/5903f05d003b
1 - Introduce a new eye-dropper highlighter; r=zer0
https://hg.mozilla.org/integration/fx-team/rev/d6064e5bc48f
2 - Add a new pickColor inspector method to use the new eye-dropper highlighter; r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/d8b8d18ab36d
3 - inspector and eye-dropper eslint cleanup; r=me
https://hg.mozilla.org/integration/fx-team/rev/ac3078c8bf35
4 - Use the new eye-dropper highlighter in the color-picker tooltip; r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/bf3e4deee24c
5 - Move eyedropper btn to inspector and update gcli command; r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/1a7579d0bf74
6 - Update the eye-dropper devtools menu entry to use the new eye-dropper; r=ochameau
https://hg.mozilla.org/integration/fx-team/rev/17f383218326
7 - Delete the old eyedropper implementation; r=ochameau
https://hg.mozilla.org/integration/fx-team/rev/a7a882d122e3
8 - Re-implement telemetry in the eye-dropper; r=miker

	Carsten Book [:Tomcat]
	Comment 63 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5903f05d003b
https://hg.mozilla.org/mozilla-central/rev/d6064e5bc48f
https://hg.mozilla.org/mozilla-central/rev/d8b8d18ab36d
https://hg.mozilla.org/mozilla-central/rev/ac3078c8bf35
https://hg.mozilla.org/mozilla-central/rev/bf3e4deee24c
https://hg.mozilla.org/mozilla-central/rev/1a7579d0bf74
https://hg.mozilla.org/mozilla-central/rev/17f383218326
https://hg.mozilla.org/mozilla-central/rev/a7a882d122e3

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 64 • 8 years ago

(In reply to Patrick Brosset <:pbro> (PTO until August 16, not doing reviews) from comment #58)
> > As said previously, I believe that should be aligned to the left instead of
> > centered, to be closest to the color preview; as we have now.
> Helen: here are 2 screenshots to compare the text alignment:
> https://dl.dropboxusercontent.com/u/714210/eyedropper-center-align.png
> https://dl.dropboxusercontent.com/u/714210/eyedropper-left-align.png
> Let me know what you think.
> 
> One thing: we need to have a rather large container for the color value
> because you can change the unit to rgb, hsl or hex, and so the width that
> the text requires varies.
> That's why I originally wanted to center the text, so it'd look better.
> Matteo makes the point that it should be left aligned, so it's closer to the
> little color square box.

I will trust you on this (large container). I think we can all agree it would look better smaller and with a variable width to house its contents: https://cl.ly/2F2E3H1B1m34 (top current, bottom desired).

If that's not possible, on the centered version, can you give the value a negative left margin of 10px or so? The value currently doesn't look centered because I assume the swatch is pushing it over to the left the width of the swatch.