997163 - Add button for eyedropper in Developer Tools' Toolbox area.

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Dane MacMillan [:danemacmillan]]

The eyedropper functionality recently landed in the Nightlies, and currently it's accessible from the color picker tooltip when clicking on a color code in the inspector, which automatically fills in a color when an item on screen is chosen, and optionally from the menu (Tools > Web Developer > Eyedropper), which instead of filling in a color, it will copy the color code to the clipboard.

I think this tool should be more accessible, and would be a good candidate for the Toolbox Buttons area, with the ability to toggle its visibility on or off from the Developer Tools options page, under "Toolbox Buttons," like the other tools currently available as Toolbox Buttons.

Comment 1

[Heather Arthur [:harth]]

See attachment 8376633 [details] [diff] [review] for how I did this originally in browser/devtools/eyedropper/commands.js. It was taken out because we didn't have a way to disable buttons before and we were worried about space.

This button should probably be off by default.

Comment 2

[Heather Arthur [:harth]]

We'll also need a eyedropper icon that includes a 'checked' look, similar to the responsive design mode.

Comment 3

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

Darrin, please add this to the pile. Would love to ship in 31 as a complete piece.

Comment 4

[Darrin Henein [:darrin]]

Attachment: Eyedropper.png

Here are the @1x and @2x pngs for the icon, with normal and toggles states.

Comment 5

[Heather Arthur [:harth]]

Attachment: responsive example

Thanks Darrin, I'm sorry to ask, but could you make it like the other command buttons? They seem to have four states. Here's the responsive mode command button as an example.

Comment 6

[Darrin Henein [:darrin]]

Attachment: Eyedropper.png v2

My apologies! I was using an old template for our icons. I've also included variations for the light theme, at both 1 and 2x.

Can you post a screenshot once these are implemented for ui-review? Thanks.

Comment 7

[Heather Arthur [:harth]]

Attachment: low contrast 'checked' look on light theme with icon

These look great. A few things though: 1) the resting state for the light icon is a shade or two lighter than the other command icons next to it 2) the checked look on the light theme it's hard to see, see screenshot.

Also, the image shifts up by one pixel for the 'hover' state in both the light and dark icons.

Comment 8

[Heather Arthur [:harth]]

> Also, the image shifts up by one pixel for the 'hover' state in both the
> light and dark icons.

Shifts up by a pixel for the 'checked' state too.

Comment 9

[Heather Arthur [:harth]]

Attachment: Add eyedropper command button

This adds an 'eyedropper' command and button (default hidden), and a test for the command.

Comment 10

[Darrin Henein [:darrin]]

Attachment: Eyedropper.png v3

Attached is the updated icon. I've learned that the light theme icons are generated from the dark theme icons (https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#763), so we only need the dark theme ones. This version fixes the 1px shift on 2 states.

Comment 11

[Joe Walker [:jwalker]]

Comment on attachment 8411455 [details] [diff] [review]
Add eyedropper command button

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

Looks good to me.
I had a question about spectrum.css, but I've asked bgrins to weigh in.

::: browser/devtools/eyedropper/test/browser_eyedropper_basic.js
@@ -71,5 @@
> -  let deferred = promise.defer();
> -  dropper.once("load", deferred.resolve);
> -
> -  return deferred.promise;
> -}

Removing this is unrelated from the rest of the patch, right? (If it's not used this is the right thing to do, so I'm not complaining, just checking my understanding)

::: browser/devtools/eyedropper/test/head.js
@@ +4,5 @@
>  const TEST_BASE = "chrome://mochitests/content/browser/browser/devtools/eyedropper/test/";
>  const TEST_HOST = 'mochi.test:8888';
>  
> +let {devtools} = Components.utils.import("resource://gre/modules/devtools/Loader.jsm", {});
> +const { Eyedropper, EyedropperManager } = devtools.require("devtools/eyedropper/eyedropper");

I think we should:

  const { require } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;

So everything is using a top level require where possible. It fits in more with the JS code loaded from sdk modules.

@@ +42,5 @@
> +    return promise.resolve();
> +  }
> +
> +  let deferred = promise.defer();
> +  dropper.once("load", deferred.resolve);

Assuming dropper uses event-emitter, then you can just:

  return dropper.once("load");

::: browser/devtools/shared/widgets/spectrum.css
@@ +9,5 @@
> +  background-size: 64px 16px;
> +  background-position: 0 center;
> +  background-repeat: no-repeat;
> +  -moz-margin-start: 5px;
> +  border: 1px solid #AAA;

Some of this looks like it should be theme css, no?

Comment 12

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #11)
> ::: browser/devtools/shared/widgets/spectrum.css
> @@ +9,5 @@
> > +  background-size: 64px 16px;
> > +  background-position: 0 center;
> > +  background-repeat: no-repeat;
> > +  -moz-margin-start: 5px;
> > +  border: 1px solid #AAA;
> 
> Some of this looks like it should be theme css, no?

From IRC:

jwalker → bgrins: spectrum.css has lots of things that look like they might be better off in theme css files. Are we just assuming that themes can override those values?

bgrins → jwalker: which things in particular? some of those hardcoded colors are actually needed to create the hue slider and whatnot.  The frame is already loading the theme files, which you can see when switching btw dark and light.

jwalker → bgrins: I guess border colors is what I noticed

bgrins → jwalker: it gets tricky when wanting to set border colors, since none of the existing .theme-fg-colorX will work (those just do text color).  My preference as we move forward would be to keep the styles in each widgets file (like spectrum.css here), then use CSS variables to use colors from the theme.  But for now, I'm actually OK with the way it is done, with one modification: the border rules should be specified separately for .theme-light and .theme-dark, instead of having a default, then overriding in theme-dark.  It makes it easier to manage

bgrins → jwalker: so like this: https://gist.github.com/bgrins/fa90358dfb2316911ac4.  This is something victor and I bumped into a lot when doing the bigger theme changes - it just makes it easier

I think that applies here.

Comment 13

[Heather Arthur [:harth]]

Thanks for the review, I'll theme-ify the CSS

(In reply to Joe Walker [:jwalker] from comment #11)
> Removing this is unrelated from the rest of the patch, right? (If it's not
> used this is the right thing to do, so I'm not complaining, just checking my
> understanding)

Just moving it to head.js as I use it in another test now.

> I think we should:
> 
>   const { require } =
> Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> 
> So everything is using a top level require where possible. It fits in more
> with the JS code loaded from sdk modules.

Unfortunately require is defined again when I do the loading of the gcli helpers on the next line, so I have to namespace the devtools one.

Comment 14

[Heather Arthur [:harth]]

Attachment: Eyedropper button updated to new icon and comments

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

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70fdf06731b8

Comment 17

[Joe Walker [:jwalker]]

(In reply to Heather Arthur [:harth] from comment #13)
> > I think we should:
> > 
> >   const { require } =
> > Cu.import("resource://gre/modules/devtools/Loader.jsm", {}).devtools;
> > 
> > So everything is using a top level require where possible. It fits in more
> > with the JS code loaded from sdk modules.
> 
> Unfortunately require is defined again when I do the loading of the gcli
> helpers on the next line, so I have to namespace the devtools one.

Ug. That's revolting, I'm sorry. I raised bug 1003076.