Michael Monreal [:micmon] Reporter
	Description • 17 years ago

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

	Michael Monreal [:micmon] Reporter
	Comment 1 • 17 years ago

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!

	Michael Monreal [:micmon] Reporter
	Comment 2 • 17 years ago

To better illustrate what I meant...

	Michael Monreal [:micmon] Reporter
	Comment 3 • 17 years ago

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.

	Michael Ventnor Assignee
	Comment 4 • 17 years ago

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

	Reed Loden [:reed]
	Comment 5 • 17 years ago

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! :)

	Ryan Flint [:rflint] (ping via IRC for reviews)
	Comment 6 • 17 years ago

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.

	Reed Loden [:reed]
	Comment 7 • 17 years ago

(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.

	Michael Ventnor Assignee
	Comment 8 • 17 years ago

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.

	Dietrich Ayala (:dietrich)
	Comment 9 • 17 years ago

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

r=me, thanks

	Reed Loden [:reed]
	Comment 10 • 17 years ago

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.

	Michael Ventnor Assignee
	Comment 11 • 17 years ago

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

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

	Michael Monreal [:micmon] Reporter
	Comment 12 • 17 years ago

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.

	Michael Ventnor Assignee
	Comment 13 • 17 years ago

(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...

	Michael Ventnor Assignee
	Comment 14 • 17 years ago

Error pages only use dialog-sized icons.

	Michael Ventnor Assignee
	Comment 15 • 17 years ago

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

	Michael Monreal [:micmon] Reporter
	Comment 16 • 17 years ago

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...  

	Ryan Flint [:rflint] (ping via IRC for reviews)
	Comment 18 • 17 years ago

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?

	Michael Ventnor Assignee
	Comment 19 • 17 years ago

(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?

	Ryan Flint [:rflint] (ping via IRC for reviews)
	Comment 20 • 17 years ago

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 :)

	Mike Schroepfer
	Comment 22 • 17 years ago

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

Nice work on this folks!

	Reed Loden [:reed]
	Comment 23 • 17 years ago

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

	Reed Loden [:reed]
	Comment 24 • 17 years ago

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

	Reed Loden [:reed]
	Comment 25 • 17 years ago

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

	Michael Monreal [:micmon] Reporter
	Comment 26 • 17 years ago

I think we can close this?

	Reed Loden [:reed]
	Comment 27 • 17 years ago

Sounds good to me.