All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
mozilla1.9.3a5
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Filed bug 539836 for the winstripe versions of these icons.
(Assignee)

Comment 2

9 years ago
Created attachment 421735 [details]
Current placeholder :-)

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

Updated

9 years ago
Blocks: 539848
Assignee: nobody → shorlander
>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).

Updated

9 years ago
No longer blocks: 538910
(Assignee)

Updated

9 years ago
Summary: Need new pinstripe icons for in-content plugin problem UI → Need new pinstripe icons plugin problem notification bar
Created attachment 434678 [details]
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.
(Assignee)

Comment 5

9 years ago
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).
Created attachment 444548 [details]
OSX 16x16 Block Glyph for Notification Bar
(Assignee)

Comment 7

9 years ago
Created attachment 445005 [details] [diff] [review]
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).
Assignee: shorlander → dolske
Attachment #421735 - Attachment is obsolete: true
Attachment #434678 - Attachment is obsolete: true
Attachment #444548 - Attachment is obsolete: true
Attachment #445005 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Summary: Need new pinstripe icons plugin problem notification bar → Need updated content plugin-problem icons and new pinstripe notification icon
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.
Attachment #445005 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 9

9 years ago
Created attachment 445235 [details] [diff] [review]
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.)
Attachment #445005 - Attachment is obsolete: true
Attachment #445235 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #445235 - Flags: review?(gavin.sharp)
Attachment #445235 - Flags: review?(dao)
Attachment #445235 - Flags: review?
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.
(Assignee)

Comment 11

9 years ago
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.]
(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.)
(Assignee)

Comment 13

9 years ago
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?
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.
(Assignee)

Comment 15

9 years ago
Created attachment 445822 [details] [diff] [review]
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.
Attachment #445235 - Attachment is obsolete: true
Attachment #445822 - Flags: review?
Attachment #445235 - Flags: review?(gavin.sharp)
Attachment #445235 - Flags: review?(dao)
(Assignee)

Updated

9 years ago
Attachment #445822 - Flags: review? → review?(dao)

Updated

9 years ago
Attachment #445822 - Flags: review?(dao) → review+
(Assignee)

Comment 16

9 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/7ea6794eb365
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5

Comment 17

9 years ago
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).
You need to log in before you can comment on or make changes to this bug.