Closed Bug 405165 Opened 12 years ago Closed 12 years ago

Menu icons for Gnomestripe

Categories

(Firefox :: Theme, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 beta3

People

(Reporter: micmon, Assigned: ventnor.bugzilla)

References

Details

Attachments

(7 files, 1 obsolete file)

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
Blocks: 405163
No longer blocks: 405163
Depends on: 405163
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)
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)
Attachment #289976 - Attachment is obsolete: true
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.
Assignee: nobody → ventnor.bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #290296 - Flags: review?(rflint)
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.
Attachment #290296 - Flags: review?(rflint) → review+
Attachment #290296 - Flags: approval1.9?
Attachment #290296 - Flags: approval1.9? → approval1.9+
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.
Attachment #290769 - Flags: review?(rflint)
Attachment #290769 - Flags: review?(rflint) → review+
Attachment #290769 - Flags: approval1.9?
Attachment #290769 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M10
Version: unspecified → Trunk
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....
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.
Resolution: FIXED → INCOMPLETE
(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.
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Tools->Add-ons and Help->Report Broken Web Site... could both use menu icons.
File->Page Setup..., too.
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)
(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.
(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?
(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.
(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?
(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?
(In reply to comment #18)
> Probably need to utilize Ventron's patch from bug 405368 for this, maybe?
>
... or just ifdef it ;)
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.
Attachment #291815 - Flags: review?(rflint)
Ah, very well. I noticed this too, but could never really pin it down to one action. Glad this will be fixed.
Attachment #291815 - Flags: review?(rflint)
Attachment #291815 - Flags: review+
Attachment #291815 - Flags: approval1.9?
(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.
Attachment #291815 - Flags: approval1.9? → approval1.9+
Sometimes the select all option in Places can be disabled. We need to fix the CSS rule there too.
Attachment #290296 - Attachment is obsolete: true
Attachment #290769 - Attachment is obsolete: true
Attachment #292473 - Flags: review?(rflint)
Attachment #292473 - Flags: review?(rflint)
Attachment #292473 - Flags: review+
Attachment #292473 - Flags: approval1.9?
Attachment #292473 - Flags: approval1.9? → approval1.9+
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
Target Milestone: Firefox 3 M10 → Firefox 3 M11
UL textboxes have context menus that are ripe for stock icons.
Attachment #291815 - Attachment is obsolete: true
Attachment #292473 - Attachment is obsolete: true
Attachment #292505 - Flags: review?(rflint)
Attachment #291815 - Attachment description: Part 1 followup - Fix reload icon → Part 1 followup - Fix reload icon [checked in]
Attachment #291815 - Attachment is obsolete: false
Attachment #292473 - Attachment description: Part 2 followup - Fix select all icon → Part 2 followup - Fix select all icon [checked in]
Attachment #292473 - Attachment is obsolete: false
Attachment #290296 - Attachment description: Part 1 - Just the browser bits first → Part 1 - Just the browser bits first [checked in]
Attachment #290296 - Attachment is obsolete: false
Attachment #290769 - Attachment description: Part 2 - Places Organizer → Part 2 - Places Organizer [checked in]
Attachment #290769 - Attachment is obsolete: false
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.
Attachment #292505 - Flags: review?(rflint) → review+
Attachment #292505 - Flags: approval1.9?
Attachment #292505 - Flags: approval1.9? → approval1.9+
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
Attachment #292505 - Attachment description: Textbox context menu → Textbox context menu [checked in]
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.
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)
File: "Send link" should use appropriate icon (document-send).
Jakub: document-send is not a gtk stock icon, so it cannot be used.
Depends on: 414260
Depends on: 416654
Just re-tested with latest nightly, the places listed in comment #29 are still left to do.
This should fix everything mentioned in comment 29 and also fixes bug 416654.
Attachment #305311 - Flags: review?(rflint)
Component: OS Integration → Theme
QA Contact: os.integration → theme
Attachment #305311 - Flags: review?(rflint) → review+
Attachment #305311 - Flags: approval1.9?
Comment on attachment 305311 [details] [diff] [review]
Remainder of B4 icons

a=mconnor on behalf of 1.9 drivers
Attachment #305311 - Flags: approval1.9? → approval1.9+
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Please reopen per bug 417476 and bug 415664.
No longer blocks: 418218
Depends on: 418218
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?
(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.
(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. :)
Could anyone please bug 431572 as a dependency? We are so close :P
Duplicate of this bug: 439929
Blocks: 417667
You need to log in before you can comment on or make changes to this bug.