395034 - Password notification bar needs icon

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Tracy Walker [:tracy]]

The new password notification bar needs an icon to the left of the notification text. There is blank space there now.

Comment 1

[Justin Dolske [:Dolske]]

I'm not really sure if an icon adds value here or not. Alex?

Comment 2

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

Yeah, it needs an icon.  Tell me what size and I'll attach a temporary one to this bug, and add it to our list of icons to have produced.

Comment 3

[Justin Dolske [:Dolske]]

chrome://browser/skin/Info.png is currently being used in a few of the other notification bars, and is 16x16.

Comment 4

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

Attachment: temporary icon for the password manager notification bar

Comment 5

[Reed Loden [:reed]]

Attachment: patch - v1

Use chrome://mozapps/skin/passwordmgr/key.png as the icon.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 300609 [details] [diff] [review]
patch - v1

I think you'll also need to update toolkit/themes/*/mozapps/jar.mn to include a reference to key.png.

Comment 7

[Reed Loden [:reed]]

Attachment: add to jar.mn - v1

(In reply to comment #6)
> (From update of attachment 300609 [details] [diff] [review])
> I think you'll also need to update toolkit/themes/*/mozapps/jar.mn to include a
> reference to key.png.

Yep, I usually just do that when landing the new icons, but since you asked, here's a patch for winstripe and pinstripe. I'm including the addition to gnomestripe in another bug, so not changing it here.

Comment 8

[Reed Loden [:reed]]

Attachment: combined patch - v1

Combined patch... gnomestripe already has its new icon and jar.mn changes already done.

Comment 9

[Reed Loden [:reed]]

Comment on attachment 300616 [details] [diff] [review]
combined patch - v1

Simple patch to make the password manager's notification bar use an icon (using temporary icon for pinstripe/winstripe and completed/final icon for gnomestripe).

Comment 10

[Reed Loden [:reed]]

Checking in toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js,v  <--  nsLoginManagerPrompter.js
new revision: 1.19; previous revision: 1.18
done
Checking in toolkit/themes/pinstripe/mozapps/jar.mn;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/jar.mn,v  <--  jar.mn
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/passwordmgr/key.png,v
done
Checking in toolkit/themes/pinstripe/mozapps/passwordmgr/key.png;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/passwordmgr/key.png,v  <--  key.png
initial revision: 1.1
done
Checking in toolkit/themes/winstripe/mozapps/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/jar.mn,v  <--  jar.mn
new revision: 1.23; previous revision: 1.22
done
RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/mozapps/passwordmgr/key.png,v
done
Checking in toolkit/themes/winstripe/mozapps/passwordmgr/key.png;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/passwordmgr/key.png,v  <--  key.png
initial revision: 1.1
done

Comment 11

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008020104 Minefield/3.0b3pre. Also verified on Windows Vista nightly.

Comment 12

[Ed Hume]

One thing more, please. You need to allow the icon to be accessible through themes. Right now it is hard-coded. 

Although we themers can swap out the image with one we like, we might have a preexisting image we want to use for the purpose. Requiring a separate image can be wasteful.

In general, every icon you put in fx should be defined in a theme-based css file so we themrs have access to it.

Comment 13

[Justin Dolske [:Dolske]]

The save-password bar's icon is set the same way as every other notification bar icon, so there's a problem with that it should be taken up in a new bug (for the general issue). [That said, can't themes set |override| directives in chrome.manifest?]

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #12)
> Although we themers can swap out the image with one we like, we might have a
> preexisting image we want to use for the purpose. Requiring a separate image
> can be wasteful.

Not sure what you mean by "requiring a separate image". You can override multiple images with the same source image, right?

Comment 15

[Ed Hume]

When the name and address of an image is hard-coded, all we can do in theme files is swap out an image while keeping the same name and address. When an image is specified in css, OTOH, we can indeed assign many functions to the same image.

I'm not sure we can override things in chrome.manifest from themes. They are not extensions, after all.

If that does work, and if we have to set overrides for every hard-coded image, our chrome.manifest files are going to get big, complex and confusing. They will act as a barrier to thorough theming.

Now that you mention other notification bar icons, I can see that this is a more pervasive problem that I had thought. I'll file a separate bug.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> When the name and address of an image is hard-coded, all we can do in theme
> files is swap out an image while keeping the same name and address. When an
> image is specified in css, OTOH, we can indeed assign many functions to the
> same image.

I still don't understand. My understanding is that tour new image has a URI, something like chrome://yourtheme/skin/image.png. You can make the hard-coded URI used in Firefox code map to your new image's URI using chrome overrides. You can still use chrome://yourtheme/skin/password.png in other places, with CSS, etc.

I might be missing something; indeed I am not a theme developer and don't have much experience creating them. It would be good to clear up any misunderstandings with a newsgroup post rather than going back and forth in different bugs.

Comment 17

[Ed Hume]

I started a thread in the Mozillazine forums:

http://forums.mozillazine.org/viewtopic.php?p=3241514#3241514

I will just say that themes use chrome.manifest to tell Firefox where to find the various directories. I have never seen an override code in a theme's chrome.manifest. Although extensions have wide latitude, I do not know if mere themes can override anything in chrome.manifest.

Comment 18

[Justin Dolske [:Dolske]]

Looks like you filed bug 415400 on the general issue, thanks. [All: please use that bug for further comments.]