Password notification bar needs icon

VERIFIED FIXED in mozilla1.9beta3

Status

()

Toolkit
Password Manager
--
trivial
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: tracy, Assigned: reed)

Tracking

Trunk
mozilla1.9beta3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [passwordManager-ui])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
The new password notification bar needs an icon to the left of the notification text. There is blank space there now.
I'm not really sure if an icon adds value here or not. Alex?
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.
Whiteboard: [passwordManager-ui]
chrome://browser/skin/Info.png is currently being used in a few of the other notification bars, and is 16x16.
Created attachment 279851 [details]
temporary icon for the password manager notification bar

Updated

10 years ago
Assignee: nobody → dolske
Target Milestone: --- → Firefox 3 M11

Updated

10 years ago
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
(Assignee)

Comment 5

10 years ago
Created attachment 300609 [details] [diff] [review]
patch - v1

Use chrome://mozapps/skin/passwordmgr/key.png as the icon.
Assignee: dolske → reed
Status: NEW → ASSIGNED
Attachment #300609 - Flags: review?(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.
Attachment #300609 - Flags: review?(gavin.sharp)
Attachment #300609 - Flags: review?(dolske)
Attachment #300609 - Flags: review+
(Assignee)

Comment 7

10 years ago
Created attachment 300610 [details] [diff] [review]
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.
Attachment #300610 - Flags: review?(dolske)
(Assignee)

Comment 8

10 years ago
Created attachment 300616 [details] [diff] [review]
combined patch - v1

Combined patch... gnomestripe already has its new icon and jar.mn changes already done.
Attachment #300609 - Attachment is obsolete: true
Attachment #300610 - Attachment is obsolete: true
Attachment #300616 - Flags: review?(gavin.sharp)
Attachment #300610 - Flags: review?(dolske)
Attachment #300609 - Flags: review?(gavin.sharp)
Attachment #300616 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 9

10 years ago
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).
Attachment #300616 - Flags: approval1.9b3?

Updated

10 years ago
Attachment #300616 - Flags: approval1.9b3? → approval1.9b3+
(Assignee)

Comment 10

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 beta4 → Firefox 3 beta3
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.
Status: RESOLVED → VERIFIED

Comment 12

10 years ago
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.
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?]
(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

10 years ago
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.

(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

10 years ago
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.
Looks like you filed bug 415400 on the general issue, thanks. [All: please use that bug for further comments.]
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.