539834 - Need updated content plugin-problem icons and new pinstripe notification icon

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Justin Dolske [:Dolske]]

We currently have a number of icons for the in-content UI shown when a plugin has problems (missing, blocked, disabled). Bug 538910 will be adding another state for when the plugin has crashed.

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/mozapps/plugins/

We're currently using the same icons used in the addons manager, but Alex says this is a mistake. They should instead be flat monochrome glyphs that don't compete with content, akin to the broken image icon. The size should also be increased to 48x48.

Not sure what to do about the 16x16 versions used in the notification bar... Alex?

While we're here, the missing plugin icon should have versions to differentiate between "missing and we don't know how to help you out" and "missing but click here to download". Filed bug 539832 for implementing code to show the difference.

Also, as a special case, bug 538910 will be landing in a 3.6.x release branch. That minimally requires a "plugin crashed" icon, but we should be able to just update all these icons at the same time (although I suspect they will all need to remain 32x32 to avoid theme breakage). Alternatively, we could spin off a new bug for creating the new 3.6.x in the existing style and not changing the existing icons.

Comment 1

[Justin Dolske [:Dolske]]

Filed bug 539836 for the winstripe versions of these icons.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Current placeholder :-)

Oh god how did this get here, engineers are not good at designing icons. (Motivation for getting something pretty landed! :-)

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

>Not sure what to do about the 16x16 versions used in the notification bar...
>Alex?

Notification bar icons need to be monochrome, stephen can take care of making sure that they are differentiated from each other and communicate enough (or we could just do that in text, and use a generic interlocking brick for all the types of bars).

Comment 4

[Stephen Horlander [:shorlander]]

Attachment: In-Content Plugin UI Icons

This shows an icon for:
- Missing Plugin can't help (Outline Brick)
- Missing Plugin link to Download (Arrow Brick)
- Plugin Blocked (Do-Not-Want Brick)
- Plugin Disabled (X Brick)

Also a generic glyph for the notification bar on OS. Windows and Linux have a generic bar brick icon already.

Comment 5

[Justin Dolske [:Dolske]]

Still need to land a monochrome 16x16 icon for the OS X notification bar.


The notification uses chrome://mozapps/skin/plugins/pluginGeneric-16.png, but the OS X flavor of this icon is a colored (blue) block:

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/mozapps/plugins/

[That icon is currently only in Prefs -> Applications, I'll probably special-case the plugin-problem notification bar for OS X to use a "pluginGenericMonochrome-16.png or something like that.]

Also, the updated in-content 48x48 icons need to land on trunk still (they landed in Lorentz in another bug).

Comment 6

[Stephen Horlander [:shorlander]]

Attachment: OSX 16x16 Block Glyph for Notification Bar

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Adds the monochrome pluginGeneric 16x16 notification bar icon, as well as the updated in-content icons (which already landed on branch in bug 554214).

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 445005 [details] [diff] [review]
Patch v.1

>-    notificationBox.appendNotification(notify.message, notify.barID, notify.iconURL,
>+#ifndef XP_MACOSX
>+    // Use the icon specific to the event type.
>+    let iconURL = notify.iconURL;
>+#else
>+    // OS X always gets a generic monochrome icon in the notification bar
>+    let iconURL = "chrome://mozapps/skin/plugins/pluginGenericMonochrome-16.png";
>+#endif
>+    notificationBox.appendNotification(notify.message, notify.barID, iconURL,
>                                        notificationBox.PRIORITY_WARNING_MEDIUM,
>                                        notify.buttons);
>   },

We should have fixed this API to accept a class name instead of a URL long ago. Since we haven't and since we still support third-party themes, the right solution here is to let pinstripe package the same icon multiple times with the required names.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

I was initially thinking about about just adding notification[value="foo"] to notifications.css, to avoid having a bunch more file copies/names. But this doesn't seem bad, and keeps things closer to the relevant code (vs. notifications.css).

Doorhangers will save us! (I hope.)

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 445235 [details] [diff] [review]
Patch v.2

Why did you change e.g. pluginBlocked-16.png to notifyPluginBlocked.png? This makes the former unused.

Comment 11

[Justin Dolske [:Dolske]]

I changed the names so that each notification bar type gets its own icon name, since their usage differs by platform and risks breakage if a theme works on one platform but not another, which I thought was the point of your comment 8. So I'm not sure what you actually want done here.

The OS X notification bar shouldn't use the current (colored) pluginBlocked-16.png icon, but it ought to be available for use elsewhere in chrome. [Eg, the Applications tab in prefs, which uses the pluginGeneric-16 icon, and ought to show the plugin state there too... should file that as a separate bug.]

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> The OS X notification bar shouldn't use the current (colored)
> pluginBlocked-16.png icon,

Right, so I assumed the simplest fix would be to make pluginBlocked-16.png monochrome.

> but it ought to be available for use elsewhere in
> chrome. [Eg, the Applications tab in prefs, which uses the pluginGeneric-16
> icon, and ought to show the plugin state there too... should file that as a
> separate bug.]

In that case we should remove pluginBlocked-16.png from the manifest and add it back when it's actually used. And we should remove icons from the repository when using them isn't even anticipated. (E.g. showing pluginOutdated-16.png in the Applications pane doesn't sound like it would make sense.)

Comment 13

[Justin Dolske [:Dolske]]

I'm still not sure what you're asking for -- do you want me to revert back to Patch v.1 and replace existing icons, or is Patch v.2 fine except for needing pluginBlocked-16 removed? Something else?

Comment 14

[Dão Gottwald [:dao]]

The second patch seems fine, except that unused images should be removed from the manifest and images without plans for being used should be removed from the repository.

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Ah, ok, I think I see what you're thinking now.

This removes the packaging for icons not being used, and removes pluginOutdated-16.png entirely on OS X.

Comment 16

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/7ea6794eb365

Comment 17

[Alfred Kayser]

The rename of icons that have hardcoded urls in browser.js is not nice, as now themes that support before and after need to provide these icons twice in their theme file (with the old and the new name).