418877 - Error/Warning/Information/Question icons

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Michael Monreal [:micmon]]

It has been decided that for the Error/Warning/Information/Question (M3) set of icons we will use GTK stock icons:

Error: gtk-dialog-error
Information: gtk-dialog-info
Question: gtk-dialog-question
Warning: gtk-dialog-warning

We need to find all the places where we currently use a buildin icon and replace them with the right GTK icons. Mostly:

- Error/Warning pages/fav icons
- Notifications
- Error console

Comment 1

[Michael Monreal [:micmon]]

Attachment: Screenshot

This shows a few places that need to be fixed, but there are many more... Hopefully someone can find a way to reliably find them all.

Comment 2

[Ian Spence]

School is closed thanks to 4-8 inches of snow, so I'll give this a shot today 

Comment 3

[Michael Monreal [:micmon]]

Attachment: Console icon for error console

Let's cover the whole error console here. The old general/console/console-toolbar.png included error/warning/info/clear, which can all be used from gtk: gtk-dialog-error, gtk-dialog-warning, gtk-dialog-information, gtk-clear. The "main" icon is the only remaining icon on this bitmap (I don't think we need teh disabled state).

Comment 4

[Ian Spence]

I include the winstripe results because in the case of "console.css", we need
to copy it from winstripe first.

gnometripe/global/findBar.css
gnomestripe/mozapps/update/updates.css
gnomestripe/browser/safebrowsing/browser-protection.css
winstripe/global/console/console.css
winstripe/mozapps/update/update.css
winstripe/mozapps/xpinstall/xpinstallConfirm.css
winstripe/global/config.css

Those files were found using error icons, info icons, blacklist icons, warning
icons

The only issue right now is that the "Page Load Error" icon that you see in the
location bar seems to be hardcoded (really hardcodes. src="data:image/png"
hardcoded)

Comment 5

[Michael Ventnor]

Didn't you say earlier that the default gtk theme only produces those icons in dialog size? In that case would it be better to find more suitable stock icons for the 16x16 case, like gtk-info?

Comment 6

[Michael Monreal [:micmon]]

(In reply to comment #5)
> Didn't you say earlier that the default gtk theme only produces those icons in
> dialog size? 
This is correct. However, i have tested how those icons look when scaled down and found them to still look good enough. Alex Faaborg agreed with that. Note that icon themes like gnome-icon-theme or tango-icon-theme provide pixel-perfect versions for all the sizes.

Comment 7

[Ian Spence]

ok, so before I go further, would it be reasonable to assume that the icon attached could be referenced via chrome://global/skin/console/console-large.png, or should I assume it will overwrite chrome://global/skin/console/console-toolbar.png?

Also, am I correct in assuming that we are just scaling down icons and no longer creating an entirely new set like previously thought?

Comment 8

[Michael Monreal [:micmon]]

(In reply to comment #7)
> ok, so before I go further, would it be reasonable to assume that the icon
> attached could be referenced via
> chrome://global/skin/console/console-large.png, or should I assume it will
> overwrite chrome://global/skin/console/console-toolbar.png?
Doesn't really matter... we can either just replace the current image or add the console icon with the new name and remove the old image.

> Also, am I correct in assuming that we are just scaling down icons and no
> longer creating an entirely new set like previously thought?
Yes. We would never be able to create a set of error/warning/etc icons that fit nicely with EVERY theme out there.
Note that only users that do not have any desktop icon theme set (fluxbox users for example) will ever see the scaled icons and I don't think they will really notice. Users of DEs like GNOME, Xfce, KDE4 etc will get all the required sizes from their theme.

Comment 9

[Ian Spence]

What I am wondering most about is if I do "moz-icon://stock/gtk-dialog-error?size=menu" on a system without a gnome theme, will I get back a scaled down icon, or a full sized icon that I will have to scale?   Unforunately, I have a gnome theme so I've been unable to find a case where I have a large version of an icon but not a small version (even with the nonstandard icons)

Comment 10

[Michael Monreal [:micmon]]

I tested this... quit FF3. Select the default gnome theme as your icon theme, then do:

# mv /usr/share/icons/gnome /usr/share/icons/gnome-bak

Now start FF3, you should get the GTK fallback icons. You can test moz-icon://stock/gtk-dialog-error?size=menu from there.

Comment 11

[Ian Spence]

Excellent. Turns out I get exactly what I want

Comment 12

[Ian Spence]

*Thank you*

(Hate it when I forget to thank someone)

Comment 13

[Ian Spence]

Attachment: Adds the use of Error, warning

This adds the correct icons to the error console, the about:config warning, the updates dialog, the find toolbar, and various extension notification bars.

The favicon for that error page is hardcoded into mozilla/docshell/resources/content/netError.xhtml, so I can't change it based on theme

Comment 14

[Michael Monreal [:micmon]]

Nice! But what about this hardcoded icon? The mac theme will run into the same problem I suppose...

Comment 15

[Reed Loden [:reed]]

Comment on attachment 305249 [details] [diff] [review]
Adds the use of Error, warning

roc isn't a valid reviewer of this.

Can we just override the chrome URL for extensions/question.png instead of doing the #ifdef everywhere?

Comment 16

[Ian Spence]

I don't have the knowledge of the chrome to do that

Comment 17

[Michael Ventnor]

I think it would be good to use gtk-execute on the error console's Evaluate button.

Comment 18

[Ian Spence]

So I looked up how to do chrome overrides and I'm getting an error because it does not consider moz-icon://XXX as a local file (Possibly a bug I need to file, but will wait until tomorrow to see any replies)

Comment 19

[Ian Spence]

Reed, since the chrome override doesn't seem like it's going to be possible, would you prefer I just had something like

#ifdef MOZ_WIDGET_GTK2
var notification_icon = "moz-icon://stock/gtk-dialog-info?size=menu";
#else
var notification_icon = "chrome://mozapps/skin/extensions/question.png";
#endif

somewhere appropriate towards the top of the code?

Comment 20

[Ian Spence]

Just a little bump to get Reed's feedback on Comment #19

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

This will not block the final release of Firefox 3.

Comment 22

[Reed Loden [:reed]]

Mano / Gavin / Ryan: Do you know if there's a better way than what comment #19 says?

Comment 23

[Andrés Delfino]

Here is a hopefully complete list of the occurrences of the icons.
I list where a given icon is located, and where it is used.

Error:

chrome/classic/skin/classic/global/console/bullet-error.png
chrome/classic/skin/classic/global/console/console.css

chrome/classic/skin/classic/global/console/console-toolbar.png
chrome/classic/skin/classic/global/console/console.css

chrome/classic/skin/classic/global/icons/Error.png
Dind't find where it is used, if at all.

chrome/classic/skin/classic/global/icons/notfound.png
chrome/classic/skin/classic/mozapps/update/updates.css




Information:

chrome/classic/skin/classic/global/console/bullet-question.png
chrome/classic/skin/classic/global/console/console.css

chrome/classic/skin/classic/global/console/console-toolbar.png
chrome/classic/skin/classic/global/console/console.css




Question:

chrome/classic/skin/classic/mozapps/extensions/question.png
chrome/toolkit/content/mozapps/extensions/extensions.js
chrome/classic/skin/classic/global/console/console.css

chrome/classic/skin/classic/global/icons/Question.png
Dind't find where it is used, if at all.




Warning:

chrome/classic/skin/classic/global/console/bullet-warning.png
chrome/classic/skin/classic/global/console/console.css

chrome/classic/skin/classic/global/console/console-toolbar.png
chrome/classic/skin/classic/global/console/console.css

chrome/classic/skin/classic/mozapps/update/extensionalert.png
Dind't find where it is used, if at all.

chrome/classic/skin/classic/global/icons/warning-large.png
chrome/pippki/content/pippki/exceptionDialog.xul
chrome/classic/skin/classic/global/config.css

chrome/classic/skin/classic/mozapps/update/warning.gif
Dind't find where it is used, if at all.

chrome/classic/skin/classic/global/icons/Warning.png
chrome/classic/skin/classic/mozapps/update/updates.css




--

To get the list, I first used:

#!/bin/sh

for i in `find . -name "*.jar"`; do
	unzip -d "${i%.jar}" "${i}"

	rm "${i}"
done

mkdir images

for i in `find . \
-name "*.bmp" -or -name "*.bmP" -or -name "*.bMp" -or -name "*.bMP" -or \
-name "*.Bmp" -or -name "*.BmP" -or -name "*.BMp" -or -name "*.BMP" -or \
-name "*.gif" -or -name "*.giF" -or -name "*.gIf" -or -name "*.gIF" -or \
-name "*.Gif" -or -name "*.GiF" -or -name "*.GIf" -or -name "*.GIF" -or \
-name "*.ico" -or -name "*.icO" -or -name "*.iCo" -or -name "*.iCO" -or \
-name "*.Ico" -or -name "*.IcO" -or -name "*.ICo" -or -name "*.ICO" -or \
-name "*.jpg" -or -name "*.jpG" -or -name "*.jPg" -or -name "*.jPG" -or \
-name "*.Jpg" -or -name "*.JpG" -or -name "*.JPg" -or -name "*.JPG" -or \
-name "*.png" -or -name "*.pnG" -or -name "*.pNg" -or -name "*.pNG" -or \
-name "*.Png" -or -name "*.PnG" -or -name "*.PNg" -or -name "*.PNG" -or \
-name "*.xbm" -or -name "*.xbM" -or -name "*.xBm" -or -name "*.xBM" -or \
-name "*.Xbm" -or -name "*.XbM" -or -name "*.XBm" -or -name "*.XBM" -or \
-name "*.xpm" -or -name "*.xpM" -or -name "*.xPm" -or -name "*.xPM" -or \
-name "*.Xpm" -or -name "*.XpM" -or -name "*.XPm" -or -name "*.XPM" -or \
-name "*.jpeg" -or -name "*.jpeG" -or -name "*.jpEg" -or -name "*.jpEG" -or \
-name "*.jPeg" -or -name "*.jPeG" -or -name "*.jPEg" -or -name "*.jPEG" -or \
-name "*.Jpeg" -or -name "*.JpeG" -or -name "*.JpEg" -or -name "*.JpEG" -or \
-name "*.JPeg" -or -name "*.JPeG" -or -name "*.JPEg" -or -name "*.JPEG"`; do
	name=`basename "${i}"`

	while [ -e "images/${name}" ]; do
		name="_${name}"
	done

	cp "${i}" "images/${name}"
done

I view all the images, and the found in the sources where those images where used.

Hope this helps.

Comment 24

[Daniel C]

(In reply to comment #23)

> chrome/classic/skin/classic/mozapps/update/extensionalert.png
> Dind't find where it is used, if at all.

http://mxr.mozilla.org/mozilla/source/toolkit/themes/winstripe/mozapps/update/update.css#75

Comment 25

[Andrés Delfino]

(In reply to comment #24)
> (In reply to comment #23)
> 
> > chrome/classic/skin/classic/mozapps/update/extensionalert.png
> > Dind't find where it is used, if at all.
> 
> http://mxr.mozilla.org/mozilla/source/toolkit/themes/winstripe/mozapps/update/update.css#75
> 

OK, but must do we care about winstripe?

Comment 26

[Daniel C]

(In reply to comment #25)

> OK, but must do we care about winstripe?

No, but this is different from the other 3 icons you mentioned in that it is used somewhere, so needs platform specific tests if it is to be removed from the tar.bz2. Error.png and Question.png got updated yesterday but there's no code to use them yet. I think warning.gif can be removed.

Comment 27

[Andrés Delfino]

(In reply to comment #26)
> (In reply to comment #25)
> 
> > OK, but must do we care about winstripe?
> 
> No, but this is different from the other 3 icons you mentioned in that it is
> used somewhere, so needs platform specific tests if it is to be removed from
> the tar.bz2. Error.png and Question.png got updated yesterday but there's no
> code to use them yet. I think warning.gif can be removed.
> 

Ah, I see.

Comment 28

[Michael Monreal [:micmon]]

Hmm... this bug was marked blocking bug 418868, which has now been resolved? The Error/Warning/Information/Question are currently the most visible non-tango spots in the Linux builds, will this still make it into 3.0?

Comment 30

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

Can we split the changes to the error console off to a separate bug, using the Windows XP icons here really sticks out from all of the other tango icons.

Comment 31

[Reed Loden [:reed]]

Comment on attachment 305249 [details] [diff] [review]
Adds the use of Error, warning

However, until you get a new patch ready for just the question stuff, I'll leave this r?Ryan. :)

Comment 32

[Ian Spence]

I just assumed a review- since it really shouldn't go in with those #ifdefs

For those interested, split off for comment 30 is bug 427717

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304991 [details]
Console icon for error console

I don't understand what this has to do with this bug. Can someone please explain (and renom for approval?)

Comment 34

[Reed Loden [:reed]]

Comment on attachment 304991 [details]
Console icon for error console

Icon needed for bug 427717.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304991 [details]
Console icon for error console

Great, put it on that bug and ask for approval there.

Comment 36

[Reed Loden [:reed]]

Why? This bug needs the icon, too... I don't see a point in having multiple copies of the icon that might just become confusing later. The other bug clearly points to this bug for the icon and includes proper dependencies.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304991 [details]
Console icon for error console

whatever

Comment 38

[Reed Loden [:reed]]

Comment on attachment 305249 [details] [diff] [review]
Adds the use of Error, warning

Gavin, can you take a look at this, please?

Comment 39

[Reed Loden [:reed]]

Attachment: Updated patch

Updated patch removing the console changes that were done in another, simplying the config change, removing obsolete safebrowsing change, remove unneeded update change, and add another ifdef for extensions.js

Comment 40

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

Comment on attachment 320089 [details] [diff] [review]
Updated patch

I assume that you've made sure that every single one of these works correctly and looks OK?

Comment 41

[Reed Loden [:reed]]

(In reply to comment #40)
> (From update of attachment 320089 [details] [diff] [review])
> I assume that you've made sure that every single one of these works correctly
> and looks OK?

I've gone through each icon replacement in the patch and made sure that the replacement icon is the correct one and the correct size.

Comment 42

[Reed Loden [:reed]]

Comment on attachment 320089 [details] [diff] [review]
Updated patch

Replaces lots of Windows icons with correct Linux Tango variants.

Comment 43

[Mike Schroepfer]

Comment on attachment 320089 [details] [diff] [review]
Updated patch

Go Go GTK theme!

Comment 44

[Reed Loden [:reed]]

Checking in toolkit/mozapps/extensions/content/extensions.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/content/extensions.js,v  <--  extensions.js
new revision: 1.176; previous revision: 1.175
done
Checking in toolkit/themes/gnomestripe/global/findBar.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/findBar.css,v  <--  findBar.css
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/themes/gnomestripe/mozapps/update/updates.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/mozapps/update/updates.css,v  <--  updates.css
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/themes/winstripe/global/config.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/config.css,v  <--  config.css
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/themes/winstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.59; previous revision: 1.58
done

Comment 45

[Kai Liu]

This patch broke config.css for Vista; you forgot to update the aero config.css entry in jar.mn.  ;)

Comment 46

[(no longer active)]

Attachment: Fix bustage of config.css on Vista

Comment 47

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

Comment on attachment 320255 [details] [diff] [review]
Fix bustage of config.css on Vista

Kai filed bug 433069.

Comment 48

[Kai Liu]

Two mid-air collisions in a row, hehe...