Closed Bug 967168 Opened 6 years ago Closed 6 years ago

DevTools Addon icons should not be inverted on light theme

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox29 verified, firefox30 verified)

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
* 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/
Attached image addon-colors.png
Comparison of icon colors with and without patch applied
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attached patch addon-icons.patch (obsolete) — Splinter Review
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).
Attachment #8370760 - Flags: feedback?(jwalker)
Depends on: 957117
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.
Attachment #8370760 - Flags: feedback?(jwalker) → feedback+
(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.
Attached patch addon-icons.patch (obsolete) — Splinter Review
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
Attachment #8370760 - Attachment is obsolete: true
Attachment #8372407 - Flags: review?(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.
Attachment #8372407 - Flags: review?(jwalker) → review+
Updated comment
Attachment #8372407 - Attachment is obsolete: true
Attachment #8372490 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/dc07278bb6f8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Is this going to be fixed in Firefox 29 too?
Flags: needinfo?(bgrinstead)
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:
Attachment #8372490 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bgrinstead)
Attachment #8372490 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified with latest builds of Nightly and Aurora
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.