405165 - Menu icons for Gnomestripe

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Michael Monreal [:micmon]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071022 Ubuntu/7.10 (gutsy) Firefox/2.0.0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112304 Minefield/3.0b2pre

As part of the integration effort for Firefox 3, we want to add icons to menus, as this is common on Linux (see GNOME, KDE...)

I prepared some CSS code to do this as part of my userChrome. I don't really know how to properly integrate this into Gnomestripe, but if anyone would be willing to work on a patch, that would be great.

I themed all menus and popups that I have found. As usual in GNOME, not all menuitems have icons, only those which profit from it. With my changes, Firefox get icons in those places where they would be in a native GTK application. This means 95% of the icons used are themed icons from GTK. Besides those, I only used a few toolbar icons, this part actually depends on the new tango style toolbar icons, see bug 405163.

Reproducible: Always

Comment 1

[Michael Monreal [:micmon]]

Attachment: CSS Hack to add icons to the menu items

Comment 2

[Michael Monreal [:micmon]]

Note: adding menu icons also means we need a few adjustments to the menu spacing: larger padding at the left side of the menu and also some extra padding after the icon, if possible (see Bug 404751)

Comment 3

[Michael Monreal [:micmon]]

Attachment: CSS Hack to add icons to the menu items , Updated

Now that the toolbar icons are upstream, I updated the CSS file to load the images from chrome. After thinking a bit, I think it doesn't make much sense to integrate this into all the different CSS files. Why not use a new CSS file just for menu icons? This way, people who don't like them could easily disbale the CSS and prevent icons from showing up? In this case, the file here could nearly be used 1:1 (just removing a few comments and fixing the first part)

Comment 4

[Michael Ventnor]

Attachment: Part 1 - Just the browser bits first [checked in]

This does it for the main browser window's main menu and context menu. I'll work on the others in separate patches once some other patches of mine land so I can avoid conflicts.

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 290296 [details] [diff] [review]
Part 1 - Just the browser bits first [checked in]

Looks good.

We should file a followup on giving the import menuitem a proper id/command so we can stick a revert-to-saved icon on there.

Comment 6

[Michael Ventnor]

Attachment: Part 2 - Places Organizer [checked in]

The advantage of checking by command rather than ID is that the main menu items and context menu items share those attributes, which means less CSS.

Comment 7

[Reed Loden [:reed]]

Part 1:

Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.140; previous revision: 1.139
done


Part 2:

Checking in browser/themes/gnomestripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.3; previous revision: 1.2
done

Comment 8

[Boris Zbarsky [:bzbarsky]]

For what it's worth, it might be a good idea to add some ids to the XUL instead of using the (slower) attribute selectors this patch uses....

Comment 9

[Michael Monreal [:micmon]]

Still missing are icons for most context menus (on tabs, on bookmark toolbar, in text entries such as the url- or search entry) and also context menus for the Help and Addons window. Places Organizer already has icons in context menus and web pages also have.

Comment 10

[Reed Loden [:reed]]

(In reply to comment #9)
> Still missing are icons for most context menus (on tabs, on bookmark toolbar,
> in text entries such as the url- or search entry) and also context menus for
> the Help and Addons window. Places Organizer already has icons in context menus
> and web pages also have.

Then you should REOPEN the bug, not change the resolution to INCOMPLETE.

Comment 11

[Reed Loden [:reed]]

Tools->Add-ons and Help->Report Broken Web Site... could both use menu icons.

Comment 12

[Reed Loden [:reed]]

File->Page Setup..., too.

Comment 13

[Michael Monreal [:micmon]]

Let's not start sticking icons everywhere... Too many icons hurts usability, mostly if they are not even commonly used. So my take was to use GTK stock icons, which is simplest anyway, plus a few selected others.

## File->Page Setup: This icon is in the freedesktop spec (and with this, in the GNOME theme), but not accessible using plain GTK. The solution would be to support the freedesktop spec, but I guess this won't happen for 3.0. Perhaps there's a way to use the spec icon (document-page-setup) conditionally?

## Tools->Add-ons: There's no icon for "addons" currently. We could use the extensions icon but I don't know if using that for the whole dialog would make sense.

## Help->Report Broken Web Site: is this the same as the toolbar icon "Report broken"? If yes, we can just use the small toolbar version of this (as soon as the tango version is done)

Comment 14

[Reed Loden [:reed]]

(In reply to comment #13)
> ## File->Page Setup: This icon is in the freedesktop spec (and with this, in
> the GNOME theme), but not accessible using plain GTK. The solution would be to
> support the freedesktop spec, but I guess this won't happen for 3.0. Perhaps
> there's a way to use the spec icon (document-page-setup) conditionally?

Hmm, not sure. Ventron, is there a way to use moz-icon:// for something conditionally and not return the "unknown" icon if it's not there (so we just show nothing instead). If I go to moz-icon://stock/blahblah?size=menu, I see some unknown icon, so we can't just do that. :(

> ## Tools->Add-ons: There's no icon for "addons" currently. We could use the
> extensions icon but I don't know if using that for the whole dialog would make
> sense.

I think it would work for now, as that's what we use for other places to describe add-ons in general.

> ## Help->Report Broken Web Site: is this the same as the toolbar icon "Report
> broken"? If yes, we can just use the small toolbar version of this (as soon as
> the tango version is done)

Yes, it's the same icon.

Comment 15

[Michael Monreal [:micmon]]

(In reply to comment #14)
> Hmm, not sure. Ventron, is there a way to use moz-icon:// for something
> conditionally and not return the "unknown" icon if it's not there (so we just
> show nothing instead). If I go to moz-icon://stock/blahblah?size=menu, I see
> some unknown icon, so we can't just do that. :(
What is being done in Preferences->Applications? If the user doesn't have a fdo compliant theme isntalled, I think no mime type icons show up there. Is this ok (in that case we could just pull the document-page-setup and end up with no icon if it cannot be found) or is this a bug, too?

> I think it would work for now, as that's what we use for other places to
> describe add-ons in general.
Fine with me..

> > ## Help->Report Broken Web Site
> Yes, it's the same icon.
Ok, I'll try to get the tango version done ASAP. But where is this icon stored? I cannot find it in classic.jar. Can we perhaps just add it to Toolbar.png and Toolbar-small.png?

Comment 16

[Michael Ventnor]

(In reply to comment #14)
> (In reply to comment #13)
> > ## File->Page Setup: This icon is in the freedesktop spec (and with this, in
> > the GNOME theme), but not accessible using plain GTK. The solution would be to
> > support the freedesktop spec, but I guess this won't happen for 3.0. Perhaps
> > there's a way to use the spec icon (document-page-setup) conditionally?
> 
> Hmm, not sure. Ventron, is there a way to use moz-icon:// for something
> conditionally and not return the "unknown" icon if it's not there (so we just
> show nothing instead). If I go to moz-icon://stock/blahblah?size=menu, I see
> some unknown icon, so we can't just do that. :(

No, which is exactly why I want to stick with GTK icons because those are almost guaranteed to be implemented in every theme.

Comment 17

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

(In reply to comment #8)
> For what it's worth, it might be a good idea to add some ids to the XUL instead
> of using the (slower) attribute selectors this patch uses....

I think we should measure and see whether that will noticeably improve perf, and then weigh that against the argument in comment 6. Michael/Michael/Ryan, do you think you could do that?

Comment 18

[Reed Loden [:reed]]

(In reply to comment #15)
> > > ## Help->Report Broken Web Site
> > Yes, it's the same icon.
> Ok, I'll try to get the tango version done ASAP. But where is this icon stored?
> I cannot find it in classic.jar. Can we perhaps just add it to Toolbar.png and
> Toolbar-small.png?

Reporter is an extension, so the icon needs to be separate from Toolbar.png, etc.

See http://mxr.mozilla.org/firefox/source/extensions/reporter/resources/skin/classic/reporter/ for the images it has. This is extensions/reporter/resources/skin/classic/reporter/ in the tree.

Probably need to utilize Ventron's patch from bug 405368 for this, maybe?

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> Probably need to utilize Ventron's patch from bug 405368 for this, maybe?
>
... or just ifdef it ;)

Comment 20

[Michael Ventnor]

Attachment: Part 1 followup - Fix reload icon [checked in]

There is, in fact, a case where the reload menu items are disabled; right after you open a new blank tab. This fixes that so the disabled reload icon is used.

Comment 21

[Michael Monreal [:micmon]]

Ah, very well. I noticed this too, but could never really pin it down to one action. Glad this will be fixed.

Comment 22

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #17)
> I think we should measure and see whether that will noticeably improve perf,
> and then weigh that against the argument in comment 6. Michael/Michael/Ryan, do
> you think you could do that?
> 

Should be fairly minor, but I'll take a look at it.

Comment 23

[Michael Ventnor]

Attachment: Part 2 followup - Fix select all icon [checked in]

Sometimes the select all option in Places can be disabled. We need to fix the CSS rule there too.

Comment 24

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Checked in followups 1 and 2.

mozilla/browser/themes/gnomestripe/browser/places/organizer.css 1.4
mozilla/browser/themes/gnomestripe/browser/browser.css 1.143

Comment 25

[Michael Ventnor]

Attachment: Textbox context menu [checked in]

UL textboxes have context menus that are ripe for stock icons.

Comment 26

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 292505 [details] [diff] [review]
Textbox context menu [checked in]

Note that this'll need to be updated on checkin due to bug 399820.

Comment 27

[Reed Loden [:reed]]

Comment on attachment 292505 [details] [diff] [review]
Textbox context menu [checked in]

Checking in toolkit/themes/gnomestripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/jar.mn,v  <--  jar.mn
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/global/textbox.css,v
done
Checking in toolkit/themes/gnomestripe/global/textbox.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/textbox.css,v  <--  textbox.css
initial revision: 1.1
done

Comment 28

[Matt]

Apologies for conditional bugspam if this has already been pointed out somewhere else. I use the Gentoo overlay (which is currently 20071207).

The menu icons should ideally be integrated with Gnome preferences, or GTK preferences if it's on the toolkit level, of which I'm unsure.

Menu icons can be disabled or enabled on a cross-application basis, at least on Gnome. It looks very out of place if the rest of one's applications have menu icons disabled.

Comment 29

[Michael Monreal [:micmon]]

Still missing menu icons I have found:

- all the actions in context menu of the bookmark "name" entry filed in places organizer

- places organizer sidebar: "open in new tab" and "open in new tab"

- bookmarks toolbar: "open in new tab" and "open in new tab"

- bookmarks toolbar: Properties (gtk-properties)

- bookmarks sidebar: all actions in right-click context menu

- main search entry context menu: "Clear search history" (gtk-clear)

- actions in help window page context menu (when right-clicking the page in help)

- actions help window link context menu (when right-clicking a link in help)

Comment 30

[u294409]

File: "Send link" should use appropriate icon (document-send).

Comment 31

[Michael Monreal [:micmon]]

Jakub: document-send is not a gtk stock icon, so it cannot be used.

Comment 32

[Michael Monreal [:micmon]]

Just re-tested with latest nightly, the places listed in comment #29 are still left to do.

Comment 33

[Michael Ventnor]

Attachment: Remainder of B4 icons

This should fix everything mentioned in comment 29 and also fixes bug 416654.

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 305311 [details] [diff] [review]
Remainder of B4 icons

a=mconnor on behalf of 1.9 drivers

Comment 35

[Asaf Romano (gone)]

Comment on attachment 305311 [details] [diff] [review]
Remainder of B4 icons

mpa=mano

Comment 36

[Reed Loden [:reed]]

Closing this bug out. If others are noticed, please file a new bug.

Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.191; previous revision: 1.190
done
Checking in browser/themes/gnomestripe/browser/searchbar.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/searchbar.css,v  <--  searchbar.css
new revision: 1.27; previous revision: 1.26
done
Checking in browser/themes/gnomestripe/browser/places/organizer.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/places/organizer.css,v  <--  organizer.css
new revision: 1.11; previous revision: 1.10
done
Checking in browser/themes/gnomestripe/browser/places/places.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/places/places.css,v  <--  places.css
new revision: 1.25; previous revision: 1.24
done
Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.262; previous revision: 1.261
done
Checking in toolkit/themes/gnomestripe/help/help.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/help/help.css,v  <--  help.css
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/help/content/helpContextOverlay.xul;
/cvsroot/mozilla/toolkit/components/help/content/helpContextOverlay.xul,v  <--  helpContextOverlay.xul
new revision: 1.14; previous revision: 1.13
done

Comment 37

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022422 Minefield/3.0b4pre ID:2008022422

For the record I suspect this bug broke Tab Mix Plus extension 0.3.6.1.080223, which then interfered with browser chrome and other extensions. No bug has been filed, and onemen (TMP author) has been informed.

Comment 38

[Andrés Delfino]

Please reopen per bug 417476 and bug 415664.

Comment 39

[Reed Loden [:reed]]

monreal, would the new icon in notifyBadges.png (http://mxr.mozilla.org/firefox/source/toolkit/themes/gnomestripe/mozapps/extensions/notifyBadges.png) work as a menu icon for Help -> Check for Updates..., or would that be abusing it too much considering that the Add-ons Manager uses it?

Comment 40

[Michael Monreal [:micmon]]

(In reply to comment #39)
> monreal, would the new icon in notifyBadges.png
> work as a menu icon for Help -> Check for Updates..., or would that be abusing
> it too much considering that the Add-ons Manager uses it?

For me this would indicate "updates available", so, no... anyway I think this menu entry is better off without an icon.

Comment 41

[Reed Loden [:reed]]

(In reply to comment #40)
> (In reply to comment #39)
> For me this would indicate "updates available", so, no... anyway I think this
> menu entry is better off without an icon.

Ok. Just thought I'd check. :)

Comment 42

[Andrés Delfino]

Could anyone please bug 431572 as a dependency? We are so close :P