967168 - DevTools Addon icons should not be inverted on light theme

Status: VERIFIED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

Command button icons and tab bar icons are inverted by default for light theme.  For addons, this can cause some weird looking icons (see the addons in https://bugzilla.mozilla.org/attachment.cgi?id=8369102), and we should probably just leave them be alone.

Comment 1

[Brian Grinstead [:bgrins]]

* Tab bar icon can be tested by adding Ember inspector: https://addons.mozilla.org/en-US/firefox/addon/ember-inspector/
* Command buttons can be tested by adding Open With: https://addons.mozilla.org/en-us/firefox/addon/open-with/

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: addon-colors.png

Comparison of icon colors with and without patch applied

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: addon-icons.patch

Joe, I see that you've worked with the registerTool functionality, so I'm wonder if this seems to be a good approach.  Basically, we want an addon author to have to opt in to the auto inverting that happens on the tab bar icons (see screenshot here: https://bugzilla.mozilla.org/attachment.cgi?id=8370752).  I've added a property to the toolDefinition called iconThemable that defaults to false.

There is also a question of if we should be allowing an addon author to specify a second image to display on the light toolbars.  I wonder if iconThemable solves both problems - brightly colored icons, as can be seen in the screenshot, work fine on both backgrounds.  While if the author uploaded a white-ish icon (to match our dark theme tools), they could presumably enable iconThemable.  Surely there would be some edge cases with icons here, but I'm not sure how likely it is (and maybe we could cross that bridge when we got there).

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 8370760 [details] [diff] [review]
addon-icons.patch

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

The approach seems ok, I think. I wonder about using invertIconForLightTheme rather than iconThemable? (and maybe command-button-invertable rather than command-button-themed).

I get that we're trying to describe the concept rather than the method, which seems like a good thing in theory, but maybe the method is always how the concept will be done, and easier to understand.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Joe Walker [:jwalker] from comment #4)
> Comment on attachment 8370760 [details] [diff] [review]
> addon-icons.patch
> 
> Review of attachment 8370760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The approach seems ok, I think. I wonder about using invertIconForLightTheme
> rather than iconThemable? (and maybe command-button-invertable rather than
> command-button-themed).
> 
> I get that we're trying to describe the concept rather than the method,
> which seems like a good thing in theory, but maybe the method is always how
> the concept will be done, and easier to understand.

I think the more specific (proposed) names are better in this case, since the decision to allow it to happen depends exactly on what 'themable' means and how it would affect the image.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: addon-icons.patch

Ready for review.  Renamed property and class names to use more specific names, and added a test for the defaults on the tool spec.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=57e025c2ddc0

Comment 7

[Joe Walker [:jwalker]]

Comment on attachment 8372407 [details] [diff] [review]
addon-icons.patch

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

::: browser/devtools/framework/gDevTools.jsm
@@ +57,5 @@
>     *         16x16 img tag (string|required)
> +   * - invertIconForLightTheme: The icon can automatically have an inversion
> +   *         filter applied (default is false).  All builtin tools are true, but
> +   *         addons may want to omit this to prevent unwanted changes to the
> +   *         `icon` image (boolean|optional)

*        See browser/themes/shared/devtools/filters.svg#invert

?
Might help someone to know how exactly we intend to invert their icons.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: addon-icons.patch

Updated comment

Comment 9

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/dc07278bb6f8
https://tbpl.mozilla.org/?tree=Fx-Team&rev=dc07278bb6f8

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc07278bb6f8

Comment 11

[Geoff Lankow (:darktrojan)]

Is this going to be fixed in Firefox 29 too?

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8372490 [details] [diff] [review]
addon-icons.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 957117
User impact if declined: DevTools addon icon colors will be inverted on the tab bar.
Testing completed (on m-c, etc.): On m-c since 02-10
Risk to taking this patch (and alternatives if risky): Low risk, makes changes to DevTools only, particularly DevTools addons.  Normal tab bar behavior does not change.
String or IDL/UUID changes made by this patch:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8aab2d3e92a

Comment 14

[Tracy Walker [:tracy]]

Verified with latest builds of Nightly and Aurora