939040 - Add eyedropper tool and include in the inspector colorpicker tooltip

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement)

Description

[Patrick Brosset <:pbro>]

Bug 889638 adds a colorpicker tooltip to the CSS rule view that appears when you click on a color swatch next to a color value.

It allows changing the color by moving color, hue and alpha sliders around.

It would be great if, from that same tooltip, there was a way to open up the pixel inspector so that we would be able to to pick a color from the page too.

I'm not sure about the way to go, we may need some UX advice here, but one way would be to have 2 modes in this tooltip: 1) color picker 2) pixel inspector.
Say at the top of the tooltip, you'd have a toggle button to switch between modes.
This way, when in pixel mode, the pixel inspector UI would be inside the tooltip too, not in a separate window.

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset from comment #0)
> Bug 889638 adds a colorpicker tooltip to the CSS rule view that appears when
> you click on a color swatch next to a color value.
> 
> It allows changing the color by moving color, hue and alpha sliders around.
> 
> It would be great if, from that same tooltip, there was a way to open up the
> pixel inspector so that we would be able to to pick a color from the page
> too.
> 
> I'm not sure about the way to go, we may need some UX advice here, but one
> way would be to have 2 modes in this tooltip: 1) color picker 2) pixel
> inspector.
> Say at the top of the tooltip, you'd have a toggle button to switch between
> modes.
> This way, when in pixel mode, the pixel inspector UI would be inside the
> tooltip too, not in a separate window.

I think we could do something simple and integrated with the colorpicker with the UI for this.  Just add an eyedropper button somewhere on the colorpicker, maybe next to the alpha slider.  Then once you click it, as you hover over things on the page it sets the colorpicker to that color - but doesn't update the CSS value until you click (though it would be interesting to play with more of a live preview on hover).  And pressing escape or clicking on the icon again would take it out of this mode without applying changes.

In this case, a more general purpose pixel inspector tool that allows magnification and shows more context would be another task with different UX needs.  But we would have most of the code in place for that feature after this is done.

Comment 2

[Heather Arthur [:harth]]

Attachment: Eyedropper standalone tool and color picker integration

Brian and I hacked on a Pixel Inspector awhile ago (see http://www.briangrinstead.com/blog/pixel-inspector), but after discussion on the mailing list and dev feedback, we decided that the tool should be reworked to be a smaller one-off color grabbing tool.

I took our pixel inspector code and made a more eyedropper-like tool. The tool is a top-level tool. Invoking it will open a little magnifying glass. Moving your mouse around the page will place the zoomed area over the pixel you're hovering. Clicking will copy the center pixel's color and close the tool.

It also adds an eyedropper icon to the color picker that will pop open the tool. Clicking on the page will set the color picker to that color instead of copying it.

Comment 3

[Heather Arthur [:harth]]

Attachment: Screenshot on OS X

Screenshot on OS X.

I haven't tried this on Windows or Linux yet. It will need to be tried out on them. Try server is down right now, but I'll post builds when it's up.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8367120 [details] [diff] [review]
Eyedropper standalone tool and color picker integration

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

Awesome to see this coming together!  A few thoughts on usage (will post a screenshot):

* It seems to be 'off' vertically by a couple of pixels (at least on OSX).  I think we bumped into this before, and it may have to do with the title bar in the browser chrome
* I'm getting 'undefined' as the result when selecting (when starting from both colorpicker and menu) even though the little swatch is updating properly
* We should constrain the preview circle inside of the bounds of the browser.  Right now I'm able to travel off into my desktop, which makes it seem like it will work for these other non-browser pieces of my screen, even though it won't.
* I think we should move the eyedropper icon on the colorpicker to be on the left side of the alpha slider, instead of below it.
* It would be *awesome* if we could scroll the page while eyedropping.  This could be a later addition, but it would make this more useful for smaller window sizes.

::: browser/devtools/eyedropper/commands.js
@@ +8,5 @@
> +Cu.import("resource://gre/modules/devtools/gcli.jsm");
> +
> +// Fetch EyedropperManager using the current loader, but don't save a
> +// reference to it, because it might change with a tool reload.
> +// We can clean this up once the command line is loadered.

What is this comment talking about when it says it can be cleaned up once the command line is loaded?  Also looks like a typo at the end of the sentence.

::: browser/devtools/eyedropper/eyedropper.js
@@ +70,5 @@
> +    let { width, zoom } = this.zoomWindow;
> +
> +    // Canvas will render whole "pixels" (cells) only, and an even
> +    // number at that. Round up to the nearest even number of pixels.
> +    let cellsWide = Math.ceil(width / zoom)

; at end of line

@@ +134,5 @@
> +      }
> +    });
> +
> +    let iframe = this.iframe = this.chromeDocument.createElement("iframe");
> +    iframe.addEventListener("load", this.frameLoaded.bind(this), true);

Do we want to remove the load listener from inside this.frameLoaded?  If so, this.frameLoaded should already bound to this so that this load listener.

@@ +167,5 @@
> +    this.loaded = true;
> +    this.emit("load");
> +  },
> +
> +  addPanelListeners: function() {

Do we need a removePanelListeners function here to clean up the listeners on destroy (not sure if it is required in this case)

@@ +387,5 @@
> +  },
> +
> +  destroy: function() {
> +    if (this._panel) {
> +      this.popupSet.removeChild(this._panel);

You could call this._panel.remove() to save a few characters

::: browser/devtools/eyedropper/test/browser_eyedropper_basic.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const TESTCASE_URI = TEST_BASE + "color-block.html";

We should also have a test for the integration within the colorpicker (and making sure it updates CSS rules, etc)

@@ +33,5 @@
> +
> +  let target = content.document.getElementById("test");
> +  let win = content.window;
> +
> +  EventUtils.synthesizeMouse(target, 20, 20, { type: "mousemove" }, win);

Would also be nice to test the boundary on this div, making sure that it switches from the body background to the div color at exactly the right pixel

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: usage-in-colorpicker.png

Screenshot showing a few of the notes from Comment 4.  Notice the vertical offset of the red stripe, the 'undefined' color value, and as mentioned it would be great to move the eyedropper icon to the left of the alpha slider instead of below it.

Comment 6

[Heather Arthur [:harth]]

Thanks for the great feedback, Brian. 

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Comment on attachment 8367120 [details] [diff] [review]
> Eyedropper standalone tool and color picker integration
> 
> Review of attachment 8367120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome to see this coming together!  A few thoughts on usage (will post a
> screenshot):
> 
> * It seems to be 'off' vertically by a couple of pixels (at least on OSX). 
> I think we bumped into this before, and it may have to do with the title bar
> in the browser chrome

Aha, thanks for noticing. I finally figured out how to take the title bar offset out of the picture, but this must have been regressed when I changed the height of the panel.

> * I'm getting 'undefined' as the result when selecting (when starting from
> both colorpicker and menu) even though the little swatch is updating properly

So odd. I can't reproduce this. Do you have your color format preference set?

> * We should constrain the preview circle inside of the bounds of the
> browser.  Right now I'm able to travel off into my desktop, which makes it
> seem like it will work for these other non-browser pieces of my screen, even
> though it won't.

Good call. I don't even know how we're getting mouse events outside the browser, but maybe we can do some calculations here.

> * I think we should move the eyedropper icon on the colorpicker to be on the
> left side of the alpha slider, instead of below it.

That's true, code wise might not be easy as spectrum is it's own thing, follow-up?

> * It would be *awesome* if we could scroll the page while eyedropping.  This
> could be a later addition, but it would make this more useful for smaller
> window sizes.

Definitely, I remember trying to scroll a couple times. You mean with swiping on the trackpad or mouse wheel I assume.

> ::: browser/devtools/eyedropper/commands.js
> @@ +8,5 @@
> > +Cu.import("resource://gre/modules/devtools/gcli.jsm");
> > +
> > +// Fetch EyedropperManager using the current loader, but don't save a
> > +// reference to it, because it might change with a tool reload.
> > +// We can clean this up once the command line is loadered.
> 
> What is this comment talking about when it says it can be cleaned up once
> the command line is loaded?  Also looks like a typo at the end of the
> sentence.

Copy and pasted, I'll take it out.

> @@ +33,5 @@
> > +
> > +  let target = content.document.getElementById("test");
> > +  let win = content.window;
> > +
> > +  EventUtils.synthesizeMouse(target, 20, 20, { type: "mousemove" }, win);
> 
> Would also be nice to test the boundary on this div, making sure that it
> switches from the body background to the div color at exactly the right pixel

True. I'm so paranoid about oranges now that I wanted to give it some cushion.

Comment 7

[Brian Grinstead [:bgrins]]

> > * It seems to be 'off' vertically by a couple of pixels (at least on OSX). 
> > I think we bumped into this before, and it may have to do with the title bar
> > in the browser chrome
> 
> Aha, thanks for noticing. I finally figured out how to take the title bar
> offset out of the picture, but this must have been regressed when I changed
> the height of the panel.

Nice, I remember this being a total mystery when we first bumped into it.

> 
> > * I'm getting 'undefined' as the result when selecting (when starting from
> > both colorpicker and menu) even though the little swatch is updating properly
> 
> So odd. I can't reproduce this. Do you have your color format preference set?
> 

Actually, my color preference was showing hex in the options panel dropdown.  But after switching to a different format, then back to hex the problem has gone away.  Guess we don't need to worry about this, could have been something messed up in my profile.
> 
> > * I think we should move the eyedropper icon on the colorpicker to be on the
> > left side of the alpha slider, instead of below it.
> 
> That's true, code wise might not be easy as spectrum is it's own thing,
> follow-up?
> 

Could be a follow up.  If you were to add it in, I would just add <button id="eyedropper-button"></button> in the element.innerHTML call in Spectrum.js (the markup / css may have to be shifted around a bit to fit it in the UI).  I would guess the rest of the code in Tooltip.js would still work since tooltipDoc.querySelector("#eyedropper-button") isn't accessed until the picker is shown (so it should be in place by this point).

> > * It would be *awesome* if we could scroll the page while eyedropping.  This
> > could be a later addition, but it would make this more useful for smaller
> > window sizes.
> 
> Definitely, I remember trying to scroll a couple times. You mean with
> swiping on the trackpad or mouse wheel I assume.
> 

Yes, exactly.  I was actually able to get this to happen once when the magnifier panel got stuck somewhere on the screen.  Maybe pointer-events:none would allow the scrolls to pass through?  Though we wouldn't want the click event to go through either, so that may not work.  There must be some way to pass the scroll events along to the main window.

Comment 8

[sjw]

Would it be possible to add an option to pick up gradients?

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to sjw from comment #8)
> Would it be possible to add an option to pick up gradients?

Check out Bug 706102 for a gradient picker.

Comment 10

[Heather Arthur [:harth]]

Attachment: Eyedropper tool and picker integration, v2

Thanks for the feedback Brian.

I'm going to leave the eyedropper icon positioning in the picker for another bug. I tried a couple positioning and CSS combos but broke the picker's alpha slider, so I'll have to try again later.

Comment 11

[Heather Arthur [:harth]]

Darrin, could you make an eyedropper icon that would fit in the toolbar (like the split console and 3d view buttons)? I think it could also double for the icon that shows in the color picker.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Heather Arthur [:harth] from comment #11)
> Darrin, could you make an eyedropper icon that would fit in the toolbar
> (like the split console and 3d view buttons)? I think it could also double
> for the icon that shows in the color picker.

I'm worried about increasing the total number of icons we have on the tab bar until we either have some way to remove certain buttons or to group them together.  We will have a keyboard shortcut / menu item mapped to the mode that the command button would open, so would still be accessible (but definitely not as discoverable).

We will need an eyedropper icon either way for the colorpicker, though if it was just for the picker and not command bar it could be an SVG instead.

Comment 13

[Heather Arthur [:harth]]

Attachment: Eyedropper tool and picker integration, v2

Small style update after testing on Windows. Windows is a few pixels off, but otherwise works.

Try builds: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d7b25d7af368/

Comment 14

[Sören Hentzschel]

Attachment: Bildschirmfoto 2014-02-15 um 03.28.15.png

Hi,

select the eyedropper tool and switch the window. The colorpicker remains in the foreground, see attached screenshot.

Comment 15

[sjw]

I found another bug which is may be related with Comment 14.
If you click on the small color circle in the rule view and then select the eyedropper, it's covered by the color picker. While you move the cursor, it flickers and if you click, it sets the color from the color picker.

Is it normal, that I can move the eyedropper out of the viewport? I think it would be better if the normal cursor appears if you hover the dev toolbar. So you can just click the icon again to exit the eyedropper and get normal access to all other menus.

I also compared the feature with other programs like photoshop. They have a much better performance. When I move over an image, the color updates very slow.

Another quite nice feature of photoshop is, that you always see the color that is currently set in comparison to the color you will select.

Comment 16

[Heather Arthur [:harth]]

(In reply to sjw from comment #15)
> Is it normal, that I can move the eyedropper out of the viewport? I think it
> would be better if the normal cursor appears if you hover the dev toolbar.
> So you can just click the icon again to exit the eyedropper and get normal
> access to all other menus.

I'm worried that then it would be hard to tell if it's working if we did that. You'd click on the eyedropper and nothing would happen. That would only happen the first time until you figure it out though.

> I also compared the feature with other programs like photoshop. They have a
> much better performance. When I move over an image, the color updates very
> slow.

What OS are you on?

Thanks for trying it out.

Comment 17

[sjw]

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

> What OS are you on?

I was testing on Windows 7.

Comment 18

[Patrick Brosset <:pbro>]

Attachment: eyedropper.png

First of all, great stuff! It's definitely very useful.

Before I do the code review, here are some things I noted about the functionality itself:

- the icon in the toolbar is broken, it seems stretched out, and it offsets on hover. It's probably just due to its format not being the same as the other ones.

- I'm also worried about adding yet another toolbar icon. I don't know how to go about it though. It's definitely useful to have this tool globally available (e.g. not just in the color-picker popup), but it's probably not as important as the inspector button or the split console. We know we need to remove some icons, but which ones? And where should they be put instead. This is not a question to be solved in this bug though so, could you please file a new one specifically for rearranging toolbar icons? And let's make sure we fix it before the next release.

- the eyedropper icon in the colorpicker has a few problems:
  - I think we could find a better place for it (maybe make the alpha slider smaller and put the icon next to it, so that they're on one line),
  - it's almost not visible when using the dark theme,
  - I think a pointer cursor would make it easier to discover,
  - the icon should change state on :hover and :active (like the toolbar icons)

- the eyedropper goes below the colorpicker tooltip
- once a color has been picked, the keybindings normally used by the colorpicker tooltip (ESC to revert, ENTER to commit) do not work anymore (the panel looses focus I think).
--> I think a way to solve these 2 things would be to close the colorpicker tooltip as soon as the eyedropper icon is clicked. Then, ESC would close the eyedropper (which it does already today), and clicking on a pixel (or pressing ENTER) would select that color and apply it to the css property (as it does today too).
--> Also, as seen in attachment 8376636 [details], the color picker tooltip remains visible when switching to a new tab. This doesn't occur when not in eyedropper mode, and closing the tooltip when clicking on the eyedropper icon would also solve this issue.

- I found one weird scenario: click on a colorswatch, click on the eyedropper, select a new color, click again on the colorswatch to dismiss the tooltip, then click on the value to switch to edit mode, press ESC. Nothing should happen other than the edit field should blur. Now it reverts the last change. Note that this already occurs without the eyedropper, so could be filed separately (it's just that with the eyedropper stealing the focus away from the panel, the only way to close is by clicking on the swatch again).

- not a big deal but when the eyedropper stops at one of the browser edges when the mouse goes outside, it would feel more natural if it continued to track the mouse movements on the remaining axis (e.g. if your mouse leaves the browser on the right side and goes down, the eyedropper should go down with it). I don't know if that's possible though.

- when using the eyedropper globally (not from the color-picker), and when selecting a color, the background behind the word "copied" changes color, it's a little lighter.

- the label below the magnifier which contains the color code is too big I think and is not horizontally aligned with the magnifier (see screenshot)

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8376633 [details] [diff] [review]
Eyedropper tool and picker integration, v2

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

Apart from the functional review done just before, I have a few comments below.

::: browser/base/content/browser-menubar.inc
@@ +486,5 @@
>                              observes="devtoolsMenuBroadcaster_ResponsiveUI"
>                              accesskey="&responsiveDesignTool.accesskey;"/>
> +                  <menuitem id="menu_eyedropper"
> +                            observes="devtoolsMenuBroadcaster_Eyedropper"
> +                            accesskey="&eyedropper.accesskey;"/>

Oh I didn't see that there was a new menu item this. Good!
Maybe then we can (as Brian was saying) keep it just in the menubar, and remove it from the toolbar? (as I said anyway, let's keep this for a follow-up bug).

Something I noticed though (not a huge deal), if you press cmd-alt-X, the eyedropper tool doesn't appear until you move your mouse. Not a big deal because it will for sure be moved at some stage, but it would be better if the tool appeared straight away.

::: browser/devtools/eyedropper/commands.js
@@ +18,5 @@
> +* 'eyedropper' command
> +*/
> +gcli.addCommand({
> +  name: "eyedropper",
> +  description: "Magnify areas of page to inspect pixels and colors",

This should be in the L10N file.

@@ +21,5 @@
> +  name: "eyedropper",
> +  description: "Magnify areas of page to inspect pixels and colors",
> +  buttonId: "command-button-eyedropper",
> +  buttonClass: "command-button",
> +  tooltipText: "Eyedropper",

This should be in the L10N file.

@@ +27,5 @@
> +  exec: function(args, context) {
> +    let chromeWindow = context.environment.chromeWindow;
> +
> +    let eyedropper = new Eyedropper(chromeWindow);
> +    eyedropper.open();

I made a comment in eyedropper.js about showing the eyedropper immediately after the open function has been called and I think it's even more important here. When using the cmdline, it's obvious that the user won't be having his/her hand on the mouse, so entering the command will have seemingly no effect.

::: browser/devtools/eyedropper/eyedropper.js
@@ +1,1 @@
> +const {Cc, Ci, Cu} = require("chrome");

Missing license information comment at the top of the file

@@ +2,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const {colorUtils} = require("devtools/css-color");
> +const CssColor = colorUtils.CssColor;

nit: maybe better written as: const {CssColor} = require("devtools/css-color").colorUtils;

@@ +18,5 @@
> +const ZOOM_PREF = "devtools.eyedropper.zoom";
> +const FORMAT_PREF = "devtools.defaultColorUnit";
> +
> +const PANEL_STYLE = "width:140px;height:120px;background-color:transparent;" +
> +                   "-moz-appearance:none;border:none;";

Wouldn't it be better to put this in the eyedropper.css file instead? Or in the appropriate browser css file since eyedropper.css seems to be loaded in the iframe only.

@@ +28,5 @@
> + * Eyedropper widget. Once opened, shows zoomed area above current pixel and
> + * displays the color value of the center pixel.
> + *
> + * let eyedropper = new Eyedropper(window);
> + * maginifier.open();

s/maginifier/eyedropper

Also, the "load" event seems pretty important, so I would add some comments here too.

@@ +32,5 @@
> + * maginifier.open();
> + */
> +
> +function Eyedropper(chromeWindow, opts = { copyOnSelect: true }) {
> +  const { copyOnSelect } = opts;

You already "this.copyOnSelect = copyOnSelect;" somewhere below, why not removing this line and instead having
this.copyOnSelect = opts.copyOnSelect;

@@ +46,5 @@
> +  this.dragging = true;
> +  this.loaded = false;
> +  this.popupSet = this.chromeDocument.querySelector("#mainPopupSet");
> +
> +  let zoom = 6; //Services.prefs.getIntPref(ZOOM_PREF);

I have seen that the pref has been added, so it should be used here.

@@ +105,5 @@
> +  /**
> +   * Show the eyedropper.
> +   */
> +  open: function() {
> +    this.chromeDocument.addEventListener("mousemove", this.onFirstMouseMove);

Why is the panel not created right away? As mentioned earlier, it means that as long as the mouse doesn't move, the eyedropper won't be visible (which can take a little while if you're using the keyboard shortcut to open the eyedropper, it takes some time to then reach the mouse and move the cursor).
I would merge open and onFirstMouseMove and remove the need for this firstMouseMove event.

One other thing is that since "open" is part of the public API, it should know how to handle multiple subsequent calls, and should therefore test if the panel has already been built before building it.

@@ +129,5 @@
> +    panel.setAttribute("close", true);
> +    panel.setAttribute("class", "devtools-eyedropper-panel");
> +    panel.setAttribute("style", PANEL_STYLE);
> +
> +    let iframe = this.iframe = this.chromeDocument.createElement("iframe");

Can you explain what is the reason for using an iframe here? The content is XUL, so it would make sense to get rid of this extra frame, but there might be things I don't know about that pushed you to do this.
Not using an iframe would avoid having to use the PANEL_STYLE const in here.

@@ +130,5 @@
> +    panel.setAttribute("class", "devtools-eyedropper-panel");
> +    panel.setAttribute("style", PANEL_STYLE);
> +
> +    let iframe = this.iframe = this.chromeDocument.createElement("iframe");
> +    iframe.addEventListener("load", this.onFrameLoaded, true);

It would also avoid having to have a load event listener and would make the whole tool synchronous to instantiate and use.

@@ +161,5 @@
> +    this.addPanelListeners();
> +
> +    this.drawWindow();
> +
> +    this.loaded = true;

This isn't used in this file. Is this part of the API, to be used by consumers?
If yes, it would probably make more sense as a getter with some comments.

About public/private, I know other files in devtools use a leading underscore to mark private functions, but this file does not. I'm tempted to say that it should be done here, but I don't know if it's a pattern we normally try to follow.

@@ +185,5 @@
> +    this.chromeDocument.addEventListener("mousedown", this.onMouseDown);
> +  },
> +
> +  removeListeners: function() {
> +    this.chromeDocument.removeEventListener("mousemove", this.onFirstMouseMove);

If you merge open and onFirstMouseMove (and show the panel right away when open is called), then I guess removing this event listener won't be needed.

@@ +260,5 @@
> +    }
> +  },
> +
> +  copyColor: function(callback) {
> +    Services.appShell.hiddenDOMWindow.clearTimeout(this.copyTimeout);

This problem I see with this is that if you during the timeout, if you click, it restarts it.
I think having a timeout to hide the eyedropper is nice, but once a color has been copied and the timeout has started, a click on the page should just hide the dropper immediately.

@@ +331,5 @@
> +      this.dragging = true;
> +    }
> +    else {
> +      this.dragging = !this.dragging;
> +    }

What about this one liner instead:
this.dragging = typeof mode === "boolean" ? mode : !this.dragging;

@@ +343,5 @@
> +
> +    let drawY = y - (height / 2);
> +    let drawX = x - (width / 2);
> +
> +    this.ctx.mozImageSmoothingEnabled = false;

Can you please add a short comment line for this? I seem to recall this was an important part.

@@ +413,5 @@
> +
> +  destroy: function() {
> +    if (this._panel) {
> +      this._panel.hidePopup();
> +      this._panel = null;

We should either reuse the XUL panel everytime an eyeDropper is instantiated or it should be removed from the tree here.
I used the eyedropper a few times, and then used the browser toolbox to inspect the page to find that there were a lot of <panel> nodes in the #mainPopupSet.

::: browser/devtools/eyedropper/test/browser_eyedropper_basic.js
@@ +10,5 @@
> +const DIV_COLOR = "#00F";
> +
> +function test()
> +{
> +  waitForExplicitFinish();

To be moved to head.js

@@ +15,5 @@
> +
> +  addTab(TESTCASE_URI, runTests);
> +}
> +
> +function runTests() {

This test is a little bit hard to read due to its async nature, and also due to the horrible waitForClipboard function (sorry, but every time I come across this function, I always need to go back to its code to remember which argument does what, also its name isn't great).
I would wrap waitForClipboard in a new function that returns a promise instead, and based on this, rewrite the test somewhat like this:

Task.spawn(function*() {
  yield addTab(TESTCASE_URI);

  let dropper = new Eyedropper(window);

  // listening for the dropper load event
  let onLoaded = dropper.once("load"); // (once() now returns a promise when no callback is passed)

  // Forcing the load of the eyedropper (although, if you fix the immediate loading as I commented in eyedropper.js, this won't be needed)
  EventUtils.synthesizeMouse(target, 20, 20, { type: "mousemove" }, win);
  yield onLoaded;

  // Listening for color selection
  let onSelect = dropper.once("select");
  // Waiting for the clipboard value
  let onClipboardValue = waitForClipboardValue(DIV_COLOR); // should return a promise

  // Simulating a mouse move and click to select a color
  EventUtils.synthesizeMouse(target, 30, 30, { type: "mousemove" }, win);
  EventUtils.synthesizeMouse(target, 30, 30, {}, win);
  yield onSelect;
 
  // Check the clipboard
  yield onClipboard;
}).then(null, ok.bind(null, false)).then(finish);

@@ +24,5 @@
> +  });
> +
> +  waitForClipboard(DIV_COLOR, () => {
> +    inspectPage(dropper);
> +  }, finish, finish);

iirc the last callback is executed on error, so you should fail the test in this case before calling finish

::: browser/devtools/eyedropper/test/head.js
@@ +6,5 @@
> +
> +const promise = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js").Promise;
> +const require = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools.require;
> +const { Eyedropper } = require("devtools/eyedropper/eyedropper");
> +

I haven't so far seen any non-async tests, so I'd add here the waitUntilExplicitFinish to avoid having to repeat it in each and every test file.

@@ +16,5 @@
> +}
> +
> +registerCleanupFunction(cleanup);
> +
> +function addTab(uri, callback) {

See bug 968727 and this diff: https://bugzilla.mozilla.org/attachment.cgi?id=8379385&action=diff#a/browser/devtools/markupview/test/head.js_sec2
I'd suggest making the addTab function you introduced just like that one so that it can be used within Task.spawn(function*(){}) tasks to avoid nested callbacks.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +190,5 @@
>        this["_onPopup" + event], false);
>    }
>  
> +  this.panel.addEventListener("popuphiding", (event) => {
> +    if (this.preventHiding) {

Hiding the tooltip while the eyedropper is visible removes the need for this.
In any case, if this stays, there should be a corresponding removeEventListener in the destroy function.

@@ +937,5 @@
>      }
> +
> +    let tooltipDoc = this.tooltip.content.contentDocument;
> +    let eyeButton = tooltipDoc.querySelector("#eyedropper-button");
> +    eyeButton.addEventListener("click", this._openEyeDropper);

If show is being called several times in a row (which we don't prevent today), then  event listeners will be added over and over again.

@@ +955,5 @@
>  
> +  _openEyeDropper: function() {
> +    // we want the click to be handled by the eyedropper, not the tooltip
> +    this.tooltip.consumeOutsideClick = false;
> +    this.tooltip.preventHiding = true;

Hiding the tooltip while the eyedropper is visible removes the need for this (also I just realized that hiding the tooltip would also solve cases where it partly hides the page and therefore may hide the actual color you want to pick).

@@ +958,5 @@
> +    this.tooltip.consumeOutsideClick = false;
> +    this.tooltip.preventHiding = true;
> +
> +    let chromeWindow = this.tooltip.doc.defaultView.top;
> +    let dropper = new Eyedropper(chromeWindow, { copyOnSelect: false });

We should cache the eyedropper instance at the tooltip level, so that it doesn't need to be re-created every time.

@@ +978,5 @@
> +    })
> +
> +    dropper.open();
> +
> +    this.tooltip.emit("eyedropper-opened", dropper);

If we need to keep the iframe (see my comment in eyedropper.js), then we should emit that even on "load" instead, since tests or other parts of devtools may rely on this event to interact with the eyedropper.

::: browser/devtools/shared/widgets/spectrum.css
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#eyedropper-button {

This rule probably be "themed": first of all we should have a light and dark version so that the border color and icon image work in both themes. And the hard coded color could probably be removed and one of the theme css class could be used instead. Better ping bgrins about this.

::: browser/devtools/styleinspector/test/browser_ruleview_eyedropper.js
@@ +49,5 @@
> +  });
> +}
> +
> +function startTests() {
> +  inspector.selection.setNode(contentDoc.body);

Please use
inspector.selection.setNode(contentDoc.body, "test");
This avoids the highlighter to be shown automatically on selection and therefore removes one server round-trip which usually creates error in the console when ending the test.

@@ +54,5 @@
> +  inspector.once("inspector-updated", testColorSwatchIsDisplayed);
> +}
> +
> +function endTests() {
> +  executeSoon(function() {

Please comment the reason for using executeSoon here (if this is related to my comment about the highlighter, it might be possible to remove it altogether)

::: browser/themes/shared/devtools/eyedropper.css
@@ +13,5 @@
> +
> +#canvas-container {
> +  display: block;
> +  position: relative;
> +  transform-origin: 50% 50%;

I do not understand why position:relative and transform-origin:50% 50% are used here.
If they are needed, can you please add a comment in the file explaining why? This doesn't seem immediately obvious.

Comment 20

[Brian Grinstead [:bgrins]]

> - I'm also worried about adding yet another toolbar icon. I don't know how
> to go about it though. It's definitely useful to have this tool globally
> available (e.g. not just in the color-picker popup), but it's probably not
> as important as the inspector button or the split console. We know we need
> to remove some icons, but which ones? And where should they be put instead.
> This is not a question to be solved in this bug though so, could you please
> file a new one specifically for rearranging toolbar icons? And let's make
> sure we fix it before the next release.

I've opened Bug 974947 to allow hiding these buttons from the options panel.

IMO we don't need to add a command button for this right now, but I'd like to hear Darrin's opinion on it (he's already needinfo'd for making the icon).

Comment 21

[Heather Arthur [:harth]]

No need for an icon right now.

Comment 22

[Darrin Henein [:darrin]]

(In reply to Heather Arthur [:harth] from comment #21)
> No need for an icon right now.

Sounds good! Just let me know when you're ready and which ones (I think you'll need command-icon and probably a new one for in the color picker) and I will create those for you.

Comment 23

[Brian Grinstead [:bgrins]]

> ::: browser/devtools/eyedropper/commands.js
> @@ +18,5 @@
> > +* 'eyedropper' command
> > +*/
> > +gcli.addCommand({
> > +  name: "eyedropper",
> > +  description: "Magnify areas of page to inspect pixels and colors",
> 
> This should be in the L10N file.
> 
> @@ +21,5 @@
> > +  name: "eyedropper",
> > +  description: "Magnify areas of page to inspect pixels and colors",
> > +  buttonId: "command-button-eyedropper",
> > +  buttonClass: "command-button",
> > +  tooltipText: "Eyedropper",
> 
> This should be in the L10N file.

Heather, I'm thinking you may be just removing these .addCommand lines if we are not doing to command buttons right now, but if not then something that may be helpful is what I did with the split console command - I simply set hidden: true on the command definition https://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/BuiltinCommands.jsm#2283, and didn't need to specify this text.

In that case it wasn't likely someone would open the split console from the developer toolbar, so it's no big deal if it doesn't autocomplete and have helper text.

Comment 24

[Heather Arthur [:harth]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #18) 
> - I'm also worried about adding yet another toolbar icon.

Taking icon and command out.

> - the eyedropper icon in the colorpicker has a few problems:
>   - I think we could find a better place for it (maybe make the alpha slider
> smaller and put the icon next to it, so that they're on one line)

That was hard, follow up.

> --> I think a way to solve these 2 things would be to close the colorpicker
> tooltip as soon as the eyedropper icon is clicked. Then, ESC would close the
> eyedropper (which it does already today), and clicking on a pixel (or
> pressing ENTER) would select that color and apply it to the css property (as
> it does today too).

I'll try this out. I wanted the developer to be able to modify the color after picking it, that was the
reason for keeping the picker up.

> - not a big deal but when the eyedropper stops at one of the browser edges
> when the mouse goes outside, it would feel more natural if it continued to
> track the mouse movements on the remaining axis (e.g. if your mouse leaves
> the browser on the right side and goes down, the eyedropper should go down
> with it). I don't know if that's possible though.

Not possible unfortunately.

> - when using the eyedropper globally (not from the color-picker), and when
> selecting a color, the background behind the word "copied" changes color,
> it's a little lighter.

It's supposed to be a little highlight box-shadow flash to make it stand out more. but I guess it's not doing it's job.

> - the label below the magnifier which contains the color code is too big I
> think and is not horizontally aligned with the magnifier (see screenshot)

It's large to make room for other color formats, rgb and hsl barely fit. We could change the length for each one. It's not aligned. That's a little tricky code-wise. I'd rather file a follow up.

Comment 25

[Heather Arthur [:harth]]

Thanks for the extensive review. I'll post a new patch soon.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #19)
> Why is the panel not created right away? As mentioned earlier, it means that
> as long as the mouse doesn't move, the eyedropper won't be visible (which
> can take a little while if you're using the keyboard shortcut to open the
> eyedropper, it takes some time to then reach the mouse and move the cursor).
> I would merge open and onFirstMouseMove and remove the need for this
> firstMouseMove event.

Here's why. When the panel is first opened we don't know where the mouse is. We only know that when we get an mouse event and we can query its clientX and clientY. Before I had it open in the center of the browser. Then it would jump to where your mouse was once you moved it. This is better, trust me.

I was thinking of making the cursor a crosshairs immediately, to give a cue to inspect with it. Maybe I'll try that out.

> @@ +129,5 @@
> > +    panel.setAttribute("close", true);
> > +    panel.setAttribute("class", "devtools-eyedropper-panel");
> > +    panel.setAttribute("style", PANEL_STYLE);
> > +
> > +    let iframe = this.iframe = this.chromeDocument.createElement("iframe");
> 
> Can you explain what is the reason for using an iframe here? The content is
> XUL, so it would make sense to get rid of this extra frame, but there might
> be things I don't know about that pushed you to do this.
> Not using an iframe would avoid having to use the PANEL_STYLE const in here.

The reason is just encapsulation. So we have a xul file we can edit instead of JS element creations, and also so that we can have a separate CSS file that doesn't add to the global browser CSS.

> @@ +24,5 @@
> > +  });
> > +
> > +  waitForClipboard(DIV_COLOR, () => {
> > +    inspectPage(dropper);
> > +  }, finish, finish);
> 
> iirc the last callback is executed on error, so you should fail the test in
> this case before calling finish

waitForClipboard will report a fail for you.

> @@ +958,5 @@
> > +    this.tooltip.consumeOutsideClick = false;
> > +    this.tooltip.preventHiding = true;
> > +
> > +    let chromeWindow = this.tooltip.doc.defaultView.top;
> > +    let dropper = new Eyedropper(chromeWindow, { copyOnSelect: false });
> 
> We should cache the eyedropper instance at the tooltip level, so that it
> doesn't need to be re-created every time.

tbh, I don't think re-creating it is a big deal.

> ::: browser/themes/shared/devtools/eyedropper.css
> @@ +13,5 @@
> > +
> > +#canvas-container {
> > +  display: block;
> > +  position: relative;
> > +  transform-origin: 50% 50%;
> 
> I do not understand why position:relative and transform-origin:50% 50% are
> used here.
> If they are needed, can you please add a comment in the file explaining why?
> This doesn't seem immediately obvious.

I don't know why either >.< Brian added this to the original code. It does break when you take them out. I'll figure it out.

Comment 26

[Heather Arthur [:harth]]

Attachment: Eyedropper tool and picker integration v3 - fixes for comments

Thanks again for that big review. Here's another one ): The main changes from last time, other than the fixes, are:

* No more toolbar item or command.

* Hide the color picker when the eyedropper button is clicked. This did make things easier, and feels right.

* Show a crosshairs cursor when the eyedropper is activated, so the user knows to start inspecting. The panel appears when the mouse is moved and we know the coordinates of the cursor.

* Color value display length depends on the format you're using, so if you use the default hex then the eyedropper will be all centered.

* Fix for a race condition that was activated by the eyedropper in browser/devtools/styleinspector/test/browser_bug940500_rule_view_pick_gradient_color.js, so some lines of Tooltip.js code might be different.

Comment 27

[Heather Arthur [:harth]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=ee544e7bda31

There's a test failing because I changed css-color.js to give rounded HSL strings. e.g. "hsl(120, 0%, 100%)" instead of "hsl(119.876, 0%, 100%)". I know it takes away some precision, but I'm not sure anyone would use a value like that in their code. Mike, is there a reason to keep the unrounded value?

Comment 28

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

(In reply to Heather Arthur [:harth] from comment #27)
> Try:
> https://tbpl.mozilla.org/?tree=Try&rev=ee544e7bda31
> 
> There's a test failing because I changed css-color.js to give rounded HSL
> strings. e.g. "hsl(120, 0%, 100%)" instead of "hsl(119.876, 0%, 100%)". I
> know it takes away some precision, but I'm not sure anyone would use a value
> like that in their code. Mike, is there a reason to keep the unrounded value?

I am happy with rounding.

Comment 29

[Heather Arthur [:harth]]

Comment on attachment 8386580 [details] [diff] [review]
Eyedropper tool and picker integration v3 - fixes for comments

Patrick's out for the week. I'm also currently investigating the issues on Linux.

Comment 30

[Heather Arthur [:harth]]

Attachment: Eyedropper tool and picker integration v4

This patch fixes a couple issues with Linux.

Moving the eyedropper around was quite laggy on the Ubuntu laptop I tried. I made a few optimizations to what was being called on every mouse move. drawWindow() was drawing a bit more of the window than it had to, now it only draws what it needs to. Also, we no longer use CSSColor objects, we just use the functionality that we need directly.

Both of those helped, but there's still a lot of room for improvement on Linux. The two remaining bottlenecks are the call to getImageData() to get the color of the center pixel. That call is really slow on Linux only. Might be bug 550845. Also we're drawing the grid by hand with canvas on every mouse move. We could position a grid image on top of the canvas instead. I have a patch for that, but it needs more investigating and I'll file a follow-up bug.

Linux was also off by a several pixels, so there's a special case for that now too.

Comment 31

[Patrick Brosset <:pbro>]

(In reply to Heather Arthur [:harth] from comment #26)
> Created attachment 8386580 [details] [diff] [review]
> Eyedropper tool and picker integration v3 - fixes for comments
> 
> Thanks again for that big review. Here's another one ): The main changes
> from last time, other than the fixes, are:
> 
> * No more toolbar item or command.
Great.

> * Hide the color picker when the eyedropper button is clicked. This did make
> things easier, and feels right.
Excellent. I've applied the patch locally and played with it, and it does feel right indeed.

> * Show a crosshairs cursor when the eyedropper is activated, so the user
> knows to start inspecting. The panel appears when the mouse is moved and we
> know the coordinates of the cursor.
Cool, that helps.

> * Color value display length depends on the format you're using, so if you
> use the default hex then the eyedropper will be all centered.
Ok

> * Fix for a race condition that was activated by the eyedropper in
> browser/devtools/styleinspector/test/
> browser_bug940500_rule_view_pick_gradient_color.js, so some lines of
> Tooltip.js code might be different.
Ok. I'm about to land the patch for bug 966424 which fixes a number of problems related to the tooltips in tests. Let me know if you need help merging this in your patch. Some things have changed quite a bit.

Comment 32

[Patrick Brosset <:pbro>]

Comment on attachment 8390242 [details] [diff] [review]
Eyedropper tool and picker integration v4

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

Looks very good to me.
r=me with a couple of test changes as explained below:

::: browser/devtools/eyedropper/test/browser_eyedropper_basic.js
@@ +1,4 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Can you add a comment at the beginning of the test that quickly explains what it is supposed to be testing.
This helps a lot with maintenance later as some tests tend to be hard to understand.

@@ +4,5 @@
> +
> +const TESTCASE_URI = TEST_BASE + "color-block.html";
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

Are these needed? They don't seem to be used.

@@ +9,5 @@
> +
> +const DIV_COLOR = "#0000FF";
> +
> +function test() {
> +  addTab(TESTCASE_URI, testEscape);

I think the test would look nicer if you wrapped it in a Task.
Something like this:

Task.spawn(function*() {
  yield addTab(TESTCASE_URI);
  yield testEscape();
  yield testSelect();
}).then(finish);  

function testEscape*() {
  info("Creating a new eyedropper instance");
  let dropper = new Eyedropper(window);
  let onDestroy = dropper.once("destroy");

  info("Inspecting the page);
  yield inspectPage(dropper, false);

  info("Escaping out of the dropper");
  pressESC();

  yield onDestroy;
  ok(true, "escape closed the eyedropper");
}

...

::: browser/devtools/eyedropper/test/head.js
@@ +19,5 @@
> +}
> +
> +registerCleanupFunction(cleanup);
> +
> +function addTab(uri, callback) {

Why have both the callback and the returned promise?
Tests look a lot better when wrapped in Task.spawn(function*(){}) and use yield statements, so I remove the second callback parameter and only use the promise in the test.

::: browser/devtools/styleinspector/test/browser_ruleview_eyedropper.js
@@ +96,5 @@
> +    let color = cPicker.activeSwatch.style.backgroundColor;
> +    is(color, "rgb(255, 0, 153)", "swatch changed colors");
> +
> +    // the change to the content is done async after rule view change
> +    executeSoon(() => {

I don't think this test should be concerned with checking that the color has been applied to the element.
Your test should only really care about checking if the color picked by the eyedropper is now shown in the color swatch and in the rule-view. Which it does.
There's already another test that checks if the color is applied to the element, so you should simply remove this executeSoon and its content.

However, I'd add a couple of test cases here that are not covered yet:
- Pressing ESC and checking that the rule-view shows the previous value
- Checking that when the eyedropper is shown, then the colorpicker tooltip is hidden

Comment 33

[Patrick Brosset <:pbro>]

I just stumbled upon a bug that I hadn't seen during my review: 
- undock the toolbox into a separate window
- in the inspector, open a color picker and start the eye dropper from there
==> The cursor changes to the crosshair but the eye dropper never appears.

Comment 34

[Heather Arthur [:harth]]

Thanks a bunch for the review, some replies before I upload again:

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #32)
> @@ +9,5 @@
> > +
> > +const DIV_COLOR = "#0000FF";
> > +
> > +function test() {
> > +  addTab(TESTCASE_URI, testEscape);
> 
> I think the test would look nicer if you wrapped it in a Task.
> Something like this:
> 
> Task.spawn(function*() {
>   yield addTab(TESTCASE_URI);
>   yield testEscape();

We must have different taste here. It's easier for me to see the current way. I love what Task.jsm does for tests with a lot of unweildly async calls, but this test is so simple and only has a couple async calls. I can change it if you insist, but I think at this level it's a style preference.

> 
> ::: browser/devtools/styleinspector/test/browser_ruleview_eyedropper.js
> @@ +96,5 @@
> > +    let color = cPicker.activeSwatch.style.backgroundColor;
> > +    is(color, "rgb(255, 0, 153)", "swatch changed colors");
> > +
> > +    // the change to the content is done async after rule view change
> > +    executeSoon(() => {
> 
> I don't think this test should be concerned with checking that the color has
> been applied to the element.
> Your test should only really care about checking if the color picked by the
> eyedropper is now shown in the color swatch and in the rule-view. Which it
> does.

The code path for the color picker and inspector changing the style of the element is the same right now, but that could change. At some point in developing I got into a state where the color was changing in the swatch but not the page, so I think we should leave it in.

>I just stumbled upon a bug that I hadn't seen during my review: 
>- undock the toolbox into a separate window
>- in the inspector, open a color picker and start the eye dropper from there
>==> The cursor changes to the crosshair but the eye dropper never appears.

Whoa, awesome catch, thank you.

Comment 35

[Heather Arthur [:harth]]

Mike, the new box model highlighting is meddling with the color picking in my test and failing my inspector test. How do you hide the box model highlighting after inspecting an element in a test? I've tried firing mouseover events in the rule view and focusing other things as well.

Comment 36

[Patrick Brosset <:pbro>]

(In reply to Heather Arthur [:harth] from comment #34)
> Thanks a bunch for the review, some replies before I upload again:
> 
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #32)
> > @@ +9,5 @@
> > > +
> > > +const DIV_COLOR = "#0000FF";
> > > +
> > > +function test() {
> > > +  addTab(TESTCASE_URI, testEscape);
> > 
> > I think the test would look nicer if you wrapped it in a Task.
> > Something like this:
> > 
> > Task.spawn(function*() {
> >   yield addTab(TESTCASE_URI);
> >   yield testEscape();
> 
> We must have different taste here. It's easier for me to see the current
> way. I love what Task.jsm does for tests with a lot of unweildly async
> calls, but this test is so simple and only has a couple async calls. I can
> change it if you insist, but I think at this level it's a style preference.
I won't insist :)

> > 
> > ::: browser/devtools/styleinspector/test/browser_ruleview_eyedropper.js
> > @@ +96,5 @@
> > > +    let color = cPicker.activeSwatch.style.backgroundColor;
> > > +    is(color, "rgb(255, 0, 153)", "swatch changed colors");
> > > +
> > > +    // the change to the content is done async after rule view change
> > > +    executeSoon(() => {
> > 
> > I don't think this test should be concerned with checking that the color has
> > been applied to the element.
> > Your test should only really care about checking if the color picked by the
> > eyedropper is now shown in the color swatch and in the rule-view. Which it
> > does.
> 
> The code path for the color picker and inspector changing the style of the
> element is the same right now, but that could change. At some point in
> developing I got into a state where the color was changing in the swatch but
> not the page, so I think we should leave it in.
Ok, fine with me.

Comment 37

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

(In reply to Heather Arthur [:harth] from comment #35)
> Mike, the new box model highlighting is meddling with the color picking in
> my test and failing my inspector test. How do you hide the box model
> highlighting after inspecting an element in a test? I've tried firing
> mouseover events in the rule view and focusing other things as well.

We don't autohide the highlighter any more during tests as it is done using a timeout. When hide is enabled and a test server is under stress you can show the highlighter and check if it exists on the next line but if GC is triggered between the two lines it caused an intermittent orange.

If you really need to hide the highlighter at some point you would have to call highlighterActor.hideBoxModel().

Comment 38

[Heather Arthur [:harth]]

Attachment: Patch v5

Patch with the added tests. It'll need to be tried out on Linux and Windows again. Here's try:

https://tbpl.mozilla.org/?tree=Try&rev=3d2e6ba89265

Comment 39

[Heather Arthur [:harth]]

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

Comment 40

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

I had to back this out in http://hg.mozilla.org/integration/fx-team/rev/38fe3c6e2f4a for Windows (OSX was green, Linux hasn't finished running yet) mochitest-4 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=37031139&tree=Fx-Team

I don't see how this patch would be at fault, but could you do another try push with your patch and run m4 tests?

Comment 41

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

(In reply to Wes Kocher (:KWierso) from comment #40)
> (OSX was green, Linux hasn't finished running yet)
Linux finished and was also failing.

Comment 42

[Heather Arthur [:harth]]

Here's a try with the patch and all mochitests running: https://tbpl.mozilla.org/?tree=Try&rev=5d800dee808b

Comment 43

[Heather Arthur [:harth]]

Oh, it's related after all. Eyedropper uses a new shortcut, Cmd-Opt-X (Ctrl-Shift-X on Win and Linux). This is a textarea shortcut to switch the text direction, causing the editor/libeditor/text/tests/test_bug629172.html failure at least.

I guess I'll be taking the shortcut out.

Comment 44

[Heather Arthur [:harth]]

Taking out the keybinding makes it green: https://tbpl.mozilla.org/?tree=Try&rev=f28175f8df91

Comment 45

[Heather Arthur [:harth]]

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

Comment 46

[Ed Morley [:emorley]]

Whilst this was fine when it first landed (ignoring the intermittent failure):
https://tbpl.mozilla.org/?tree=Fx-Team&rev=535756591289&jobname=Windows%207.*debug.*browser-chrome

After this merge:
https://hg.mozilla.org/integration/fx-team/pushloghtml?startID=6083&endID=6084

We started getting these failures on every Windows 7 debug mochitest-browser-chrome push:
{
Assertion failure: GetTransform() == Matrix(mat.xx, mat.yx, mat.xy, mat.yy, mat.x0, mat.y0) (Transforms are out of sync), at c:\builds\moz2_slave\fx-team-w32-d-0000000000000000\build\gfx\2d\DrawTargetCairo.cpp:1011
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/eyedropper/test/browser_eyedropper_basic.js | application terminated with exit code 253
PROCESS-CRASH | chrome://mochitests/content/browser/browser/devtools/eyedropper/test/browser_eyedropper_basic.js | application crashed [@ mozilla::gfx::DrawTargetCairo::PopClip()]
}

See:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=2686abfb8e0d&jobname=Windows%207.*debug.*browser-chrome

Whilst it looks like one of the gfx changes in the merge (eg bug 989858, bug 990338, bug 989904; needinfo-ing :mattwoodrow) caused the problem, and the test in this bug just tickles it - at this point I don't know which of the changesets in the merge need backing out, so have had to backout this bug instead, until it's figured out (so we can reopen the tree):
https://hg.mozilla.org/integration/fx-team/rev/280b1db44f63

Comment 47

[Matt Woodrow (:mattwoodrow)]

Sorry for being slow to respond to this.

Is there an easy way to reproduce this locally, preferably with m-c?

If not, getting stack traces to all callers of _cairo_error would be really useful. That's the only way I can see us hitting this assertion.

Comment 48

[Heather Arthur [:harth]]

I'm going to disable the test on Windows debug. Unfortunately I'm having problems doing that. I added:

[browser_eyedropper_basic.js]
skip-if = debug && os == "win" # bug 963492

But the test still gets run on try.

Comment 49

[Heather Arthur [:harth]]

I also tried a syntax I saw elsewhere:

skip-if = os=='win'&&debug.

I can't find any details on the syntax and order of operations for this test stuff.

Comment 50

[Heather Arthur [:harth]]

Attachment: Eyedropper tool and picker integration v6

Figured out the test-skipping issue. I disabled both new tests on Windows debug builds:

https://tbpl.mozilla.org/?tree=Try&rev=8b3de6f964bf

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Looks like this hasn't been rebased since pbrosset's styleinspector test rewrite landed a couple days ago.

Comment 52

[Heather Arthur [:harth]]

Sorry Ryan for not updating.

fwiw: https://hg.mozilla.org/integration/fx-team/rev/1ee7a62f68eb

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ee7a62f68eb

Comment 54

[Joel Maher ( :jmaher ) (UTC -8)]

the eyedropper test is disabled on windows debug:
skip-if = os == "win" && debug # bug 963492

but bug 963492 is closed, is this the wrong bug?  Should we enable this for win debug?

Comment 55

[Heather Arthur [:harth]]

(In reply to Joel Maher (:jmaher) from comment #54)
> the eyedropper test is disabled on windows debug:
> skip-if = os == "win" && debug # bug 963492
> 
> but bug 963492 is closed, is this the wrong bug?  Should we enable this for
> win debug?

Looks like it was fixed right after this, we could definitely try it.

Comment 56

[sjw]

Just tested in current Nightly Build and there is a horrible issue on my Ubuntu 14.04:
If you move the mouse in a normal speed, the eyedropper is completely out of control. It flips around and follows your mouse making coils.
Another issue on Linux is that the icon is not fully visible. The bottom-line of the eyedropper button is covered.

And there are some minor performance issues on Windows and Linux:
- If an alert popped up (in my case 'confirm close' (You are about to close * tabs ...)) no mouse pointer is visible.
- If you click on the eyedropper icon and move the mouse quickly away, the eyedropper will rest on the colorpicker until you move the mouse again.
- If you close Firefox before you picked up a color, the eyedropper will disappear a few seconds after the window disappeared.
- If you move the eyedropper out of the viewport (addressbar, tabbar) you got a observable worse performance.

Comment 57

[Heather Arthur [:harth]]

(In reply to sjw from comment #56)

Thanks for trying it out and commenting.

> Just tested in current Nightly Build and there is a horrible issue on my
> Ubuntu 14.04:
> If you move the mouse in a normal speed, the eyedropper is completely out of
> control. It flips around and follows your mouse making coils.

That would be bug 990740, sounds like it should be a priority.

> Another issue on Linux is that the icon is not fully visible. The
> bottom-line of the eyedropper button is covered.

Filed bug 1001553 

> 
> And there are some minor performance issues on Windows and Linux:
> - If an alert popped up (in my case 'confirm close' (You are about to close
> * tabs ...)) no mouse pointer is visible.

Man, I don't know a way around that right now. We might want to make it so that you can only inspect content, but then we get the problem where you don't know the eyedropper is working immediately.

> - If you click on the eyedropper icon and move the mouse quickly away, the
> eyedropper will rest on the colorpicker until you move the mouse again.

Hm, the color picker should close right after the eyedropper button is clicked, file a bug with what you're seeing if you'd like.

Comment 58

[sjw]

(In reply to Heather Arthur [:harth] from comment #57)
> 
> That would be bug 990740, sounds like it should be a priority.
>
I didn't notice this bug. I was already irritating why there are no blocker.

> (In reply to sjw from comment #56)
> > - If you click on the eyedropper icon and move the mouse quickly away, the
> > eyedropper will rest on the colorpicker until you move the mouse again.
> 
> Hm, the color picker should close right after the eyedropper button is
> clicked, file a bug with what you're seeing if you'd like.

This is just while you open the eyedropper. But it's not major and I don't know if it's reproducible on faster pcs. 


After I played a bit around I miss again this feature:
(In reply to sjw from comment #15)
> Another quite nice feature of photoshop is, that you always see the color
> that is currently set in comparison to the color you will select.

What do you think about it?

Comment 59

[Brian Grinstead [:bgrins]]

> After I played a bit around I miss again this feature:
> (In reply to sjw from comment #15)
> > Another quite nice feature of photoshop is, that you always see the color
> > that is currently set in comparison to the color you will select.
> 
> What do you think about it?

Sounds like it could be a nice new feature, but we would have to work out quite a few details - could you file a bug for it describing what you want?

Comment 60

[stuart]

Is it possible for this to be able to pick a color from any open window? I frequently will have multiple sources from where I want to pick colors from.

Thank you

Comment 61

[Heather Arthur [:harth]]

(In reply to stuart from comment #60)
> Is it possible for this to be able to pick a color from any open window? I
> frequently will have multiple sources from where I want to pick colors from.
> 
> Thank you

It's not possible right now. That would be nice, but that'd involve some significant platform work I'd imagine.