Closed Bug 997163 Opened 6 years ago Closed 6 years ago

Add button for eyedropper in Developer Tools' Toolbox area.

Categories

(DevTools :: Inspector, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: danemacmillan, Assigned: harth)

Details

Attachments

(4 files, 3 obsolete files)

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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
We'll also need a eyedropper icon that includes a 'checked' look, similar to the responsive design mode.
Darrin, please add this to the pile. Would love to ship in 31 as a complete piece.
Flags: needinfo?(dhenein)
Attached file Eyedropper.png (obsolete) —
Here are the @1x and @2x pngs for the icon, with normal and toggles states.
Flags: needinfo?(dhenein)
Attached image 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.
Flags: needinfo?(dhenein)
Attached file Eyedropper.png v2 (obsolete) —
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.
Attachment #8410272 - Attachment is obsolete: true
Flags: needinfo?(dhenein)
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.
Flags: needinfo?(dhenein)
> 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.
Attached patch Add eyedropper command button (obsolete) — Splinter Review
This adds an 'eyedropper' command and button (default hidden), and a test for the command.
Attachment #8411455 - Flags: review?(jwalker)
Attached file 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.
Attachment #8411032 - Attachment is obsolete: true
Flags: needinfo?(dhenein)
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?
Attachment #8411455 - Flags: review?(jwalker) → review+
(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.
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.
Try: https://tbpl.mozilla.org/?tree=Try&rev=795f6d132221
Assignee: nobody → fayearthur
Attachment #8411455 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/70fdf06731b8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
(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.
QA Whiteboard: [good first verify]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.