404530 - Make use of GTK stock icons in more places

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/2007112004 Minefield/3.0b2pre

Firefox 3 nightlies are now using some GTK stock icons for buttons. There are however a few more places were GTK stock icons can be used. I assembled a list:

Places Organizer
# Back: moz-icon://stock/gtk-go-back?size=toolbar
# Forward: moz-icon://stock/gtk-go-forward?size=toolbar
# Clear Search (right side inside search entry)
- when active: moz-icon://stock/gtk-clear?size=toolbar
- when inactive (currently no icon): moz-icon://stock/gtk-clear?size=toolbar&state=disabled

Places Search Builder
# -:  moz-icon://stock/gtk-remove?size=menu (and remove label)
# +:  moz-icon://stock/gtk-add?size=menu (and remove label)

Find bar
# Close button: moz-icon://stock/gtk-close?size=menu (and turn into GTK button)

Help
# Back: moz-icon://stock/gtk-go-back?size=toolbar
# Back disabled: moz-icon://stock/gtk-go-back?size=toolbar&state=disabled
# Forward: moz-icon://stock/gtk-go-forward?size=toolbar
# Forward disabled: moz-icon://stock/gtk-go-forward?size=toolbar&state=disabled
# Home: moz-icon://stock/go-home?size=toolbar
# Print: moz-icon://stock/document-print?size=toolbar
# Weblink: moz-icon://stock/gtk-jump-to-ltr?size=menu

Print Preview
# .portrait-page: moz-icon://stock/gtk-orientation-portrait?size=toolbar"
# .landscape-page: moz-icon://stock/gtk-orientation-landscape?size=toolbar"


Reproducible: Always

Comment 1

[Michael Monreal [:micmon]]

Attachment: Search entry FF3 <> GTK

This shows the use of gtk-clear (here: using themed icon from GNOME) in a plain GTK app and compared to the current look in FF3. Note: the current icon is also used for "close" in FF3, so using a different "clear" icon makes sense for other platforms too!

Comment 2

[Michael Monreal [:micmon]]

Attachment: Search +/- mockup

To better illustrate what I meant...

Comment 3

[Michael Monreal [:micmon]]

Attachment: CSS Hack

I got all of this implemented in my local userChrome.css. The search builder is dynamic, so it probably needs code changes to use the icon for all search criteria. Otherwise, only simple changes.

Comment 4

[Michael Ventnor]

I've fixed the Places Organizer ones as part of a patch in bug 401279.

Comment 5

[Reed Loden [:reed]]

Attachment: Remove images from jar.mn - v1

Once the cvs copy in bug 406476 is complete, we'll need to remove the images from the jar.mn file because they aren't being copied over. This non-CVS-based patch does exactly that, plus it removes the unneeded MINIMO #ifndef (see bug 405705). Whoever reviews it, please remove the r? for the other person. I'm not sure whose queue is smaller, so I just picked both of you! :)

Comment 6

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

Comment on attachment 291128 [details] [diff] [review]
Remove images from jar.mn - v1

> % skin help classic/1.0 %skin/classic/help/
The skin package is already defined by winstripe.

>   skin/classic/help/help.css
>   skin/classic/help/helpFileLayout.css
These will have already been packaged into the jar by winstripe, so you need to instruct the build system to replace them by prefixing these lines with '+ '.

I'm also not sure why these files are being copied anyway; there's no useful history (half the move's made up of build files), the css files are going to be pretty well gutted when the icons are replaced and moves screw with the Hg import script - I'd just create new files for these.

Comment 7

[Reed Loden [:reed]]

(In reply to comment #6)
> I'm also not sure why these files are being copied anyway; there's no useful
> history (half the move's made up of build files), the css files are going to be
> pretty well gutted when the icons are replaced and moves screw with the Hg
> import script - I'd just create new files for these.

Sounds good to me. I'll resolve the copy bug as invalid.

Comment 8

[Michael Ventnor]

Attachment: Places native +/- [checked in]

This allows Linux to use native +/- images on the places organizer advanced search buttons, as well as a save icon on the save search button.

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 292337 [details] [diff] [review]
Places native +/- [checked in]

r=me, thanks

Comment 10

[Reed Loden [:reed]]

Checking in browser/components/places/content/advancedSearch.inc;
/cvsroot/mozilla/browser/components/places/content/advancedSearch.inc,v  <--  advancedSearch.inc
new revision: 1.9; previous revision: 1.8
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.5; previous revision: 1.4
done

Leaving open, as I'm not sure what else is wanted from this bug.

Comment 11

[Michael Ventnor]

Attachment: Error pages and error console

Two more highly used features that need the stock icon treatment.

"Forget about losing your work, look how native this icon is!"

Comment 12

[Michael Monreal [:micmon]]

Using stock icon in the error console is probably a bad idea: GTK only ships the warning/error/etc icons in dialog size (48x48) but we need 32x32 for the header and 16x16 (?) for the list. We will have a complete set of tango icons for this ready, soon.

Comment 13

[Michael Ventnor]

(In reply to comment #12)
> Using stock icon in the error console is probably a bad idea: GTK only ships
> the warning/error/etc icons in dialog size (48x48) but we need 32x32 for the
> header and 16x16 (?) for the list. We will have a complete set of tango icons
> for this ready, soon.
> 

Hm, here the icons come out fine but thats probably because I have a GNOME icon theme...

Comment 14

[Michael Ventnor]

Attachment: Just error pages [checked in]

Error pages only use dialog-sized icons.

Comment 15

[Michael Ventnor]

Attachment: Close button on notification bar [checked in]

Luckily reed deals with conflicts for me when he checks in. Thanks very much reed! :-)

Comment 16

[Michael Monreal [:micmon]]

Michael, yes, gnome-icon-theme ships warning/error/info in all sizes, but sadly we cannot rely on that :(

There's a bug against GTK to ship icons in all sizes, but I don't know when that will be fixed and FF3 cannot depend on this fixed version anyway. Perhaps FF4...  

Comment 17

[Michael Ventnor]

Attachment: Print preview and page setup

Comment 18

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

Comment on attachment 292703 [details] [diff] [review]
Print preview and page setup

Aren't the toolbar-portrait/landscape-page classes in printPreview.css still used?

Comment 19

[Michael Ventnor]

(In reply to comment #18)
> (From update of attachment 292703 [details] [diff] [review])
> Aren't the toolbar-portrait/landscape-page classes in printPreview.css still
> used?
> 

I don't see the buttons in print preview anymore, I just see the page navigation controls. Is it different in your print preview toolbar?

Comment 20

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

Comment on attachment 292703 [details] [diff] [review]
Print preview and page setup

(In reply to comment #19)
> I don't see the buttons in print preview anymore, I just see the page
> navigation controls. Is it different in your print preview toolbar?
> 

They're still in the DOM (and look like they're used on windows), not sure why they're hidden on linux - but I'd stick 'em in there just in case :)

Comment 21

[Michael Ventnor]

Attachment: Print preview patch 2 [checked in]

Comment 22

[Mike Schroepfer]

Comment on attachment 292728 [details] [diff] [review]
Print preview patch 2 [checked in]

Nice work on this folks!

Comment 23

[Reed Loden [:reed]]

Comment on attachment 292658 [details] [diff] [review]
Just error pages [checked in]


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

Comment 24

[Reed Loden [:reed]]

Comment on attachment 292661 [details] [diff] [review]
Close button on notification bar [checked in]

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

Comment 25

[Reed Loden [:reed]]

Comment on attachment 292728 [details] [diff] [review]
Print preview patch 2 [checked in]

Checking in toolkit/themes/gnomestripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/jar.mn,v  <--  jar.mn
new revision: 1.24; previous revision: 1.23
done
RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/global/printPageSetup.css,v
done
Checking in toolkit/themes/gnomestripe/global/printPageSetup.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/printPageSetup.css,v  <--  printPageSetup.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/themes/gnomestripe/global/printPreview.css,v
done
Checking in toolkit/themes/gnomestripe/global/printPreview.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/printPreview.css,v  <--  printPreview.css
initial revision: 1.1
done

Comment 26

[Michael Monreal [:micmon]]

I think we can close this?

Comment 27

[Reed Loden [:reed]]

Sounds good to me.