Gabriele Best [:Gabri] Reporter
	Description • 16 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050201 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050201 Minefield/3.0pre

If I hover or click the searchbox icon

Reproducible: Always

	Gabriele Best [:Gabri] Reporter
	Comment 1 • 16 years ago

If I hover or click the searchbox icon on 'Get add-ons" window it doesn't change.

	Gabriele Best [:Gabri] Reporter
	Comment 3 • 16 years ago

The icon that should be changed is this chrome://mozapps/skin/extensions/searchIcons.png

with something like that http://mxr.mozilla.org/mozilla/source/browser/themes/winstripe/browser/Search-glass.png

	Gabriele Best [:Gabri] Reporter
	Comment 4 • 16 years ago

The css file is chrome://mozapps/skin/extensions/extensions.css

	(no longer active) Assignee
	Comment 5 • 16 years ago

Toolkit can't be made dependent on browser, so we should need a new icon in the toolkit theme, unless there's one already.  CCing Alex to see what he thinks here.

	Kai Liu
	Comment 6 • 16 years ago

But browser could be made dependent on toolkit.  I was thinking that we could just move the current search glass used for the browser searchbar into toolkit, and then the searchbar CSS could reference the icon in toolkit instead of browser.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 7 • 16 years ago

Let's add:

Search-glass-aero.png
Search-glass.png
Search-glass-rtl-aero.png
Search-glass-rtl.png

to:

/toolkit/themes/winstripe/global/icons

We can't remove the existing files in browser since extensions might be relying on them.

	Kai Liu
	Comment 8 • 16 years ago

(In reply to comment #7)
> We can't remove the existing files in browser since extensions might be relying
> on them.
> 

They haven't existed very long in browser, have they?  The 3-state search glass in browser had only recently landed, so any extension that was using that would've had to adjust anyway.  Plus, extensions should not be relying on theme files in the first place because there is no guarantee that custom Firefox themes would use the same filenames.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 9 • 16 years ago

It's true that we updated the states, but the files themselves were there in Firefox 2.  While extensions probably shouldn't rely on theme files, in practice they actually do, so we want to avoid moving any files this close to RC1 out of courtesy.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 10 • 16 years ago

These are the 4 search icons to add to /toolkit/themes/winstripe/global/icons

	Gabriele Best [:Gabri] Reporter
	Comment 11 • 16 years ago

Now, these icons are used.
XP: http://mxr.mozilla.org/mozilla/source/toolkit/themes/winstripe/mozapps/extensions/searchIcons.png
VISTA: http://mxr.mozilla.org/mozilla/source/toolkit/themes/winstripe/mozapps/extensions/searchIcons-aero.png

	Gabriele Best [:Gabri] Reporter
	Comment 12 • 16 years ago

The bug is present an all the OS.

	(no longer active) Assignee
	Comment 13 • 16 years ago

Alex: we need additional icons for other pinstripe and gnomestripe as well.

	(no longer active) Assignee
	Comment 14 • 16 years ago

(In reply to comment #13)
> Alex: we need additional icons for other pinstripe and gnomestripe as well.

On further investigation, pinstripe and gnomestripe don't seem to have equivalent hover effects for the search bar.  Therefore I'm making this bug Windows-specific.

	(no longer active) Assignee
	Comment 15 • 16 years ago

Simple patch, which should be quick to review.  This patch should land with the icons in attachment 319499 [details].

	(no longer active) Assignee
	Comment 17 • 16 years ago

Comment on attachment 319778 [details] [diff] [review]
Patch (v2): it also adds ':hover' and ':hover:active' property at cancel button

(In reply to comment #16)
> Created an attachment (id=319778) [details]
> Patch (v2): it also adds ':hover' and ':hover:active' property at cancel button

This is what bug 431002 is about.  I'm marking this as obsolete, because bug 431002 has a more extensive patch waiting in Gavin's queue.

	Gabriele Best [:Gabri] Reporter
	Comment 18 • 16 years ago

Ah ok, I didn't see bug 431002 before.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 19 • 16 years ago

Comment on attachment 319499 [details]
search icons to add to toolkit

Renom for approval once the patch gets in. No point landing these until then.

	(no longer active) Assignee
	Comment 20 • 16 years ago

(In reply to comment #19)
> (From update of attachment 319499 [details])
> Renom for approval once the patch gets in. No point landing these until then.

Actually I wasn't going to request checkin-needed until my patch gets r,a as well, but it's not a big deal.  I'll ask for approval then.

	Dão Gottwald [:dao]
	Comment 21 • 16 years ago

Comment on attachment 319769 [details] [diff] [review]
Patch (v1)

>Index: toolkit/themes/winstripe/mozapps/extensions/extensions.css

>-  list-style-image: url("chrome://mozapps/skin/extensions/searchIcons.png");

This makes searchIcons.png unused, so you can remove it.

>+.searchbox-search[chromedir="rtl"] {
>+  list-style-image: url("chrome://global/skin/icons/Search-glass-rtl.png");
>+}

.searchbox-search doesn't have a chromedir attribute.

I think we should add and use the search glass as a global toolkit image in all themes consistently.

	Klaas Heidstra
	Comment 22 • 16 years ago

(In reply to comment #14)
> (In reply to comment #13)
> > Alex: we need additional icons for other pinstripe and gnomestripe as well.
> 
> On further investigation, pinstripe and gnomestripe don't seem to have
> equivalent hover effects for the search bar.  Therefore I'm making this bug
> Windows-specific.

These icons are also needed for gnomestripe, therefore
"Search-glass.png" and "Search-glass-rtl.png" (and the appropriate styling)
should also be added for gnomestripe to:
/toolkit/themes/gnomestripe/global/icons

In pinstripe this icon in toolkit is named search-textbox.png (there is no rtl search icon for pinstripe), so that one maybe should be renamed to Search-glass.png also.

This is also needed for bug 403154 because my patch in that bug is referring to
these files.

	(no longer active) Assignee
	Comment 23 • 16 years ago

(In reply to comment #21)
> (From update of attachment 319769 [details] [diff] [review])
> >Index: toolkit/themes/winstripe/mozapps/extensions/extensions.css
> 
> >-  list-style-image: url("chrome://mozapps/skin/extensions/searchIcons.png");
> 
> This makes searchIcons.png unused, so you can remove it.

I guess Alex was opposed to remove images from the theme at this point of the release cycle.  Alex: can you confirm, please?

> >+.searchbox-search[chromedir="rtl"] {
> >+  list-style-image: url("chrome://global/skin/icons/Search-glass-rtl.png");
> >+}
> 
> .searchbox-search doesn't have a chromedir attribute.

I'll address this in the new patch coming along.

> I think we should add and use the search glass as a global toolkit image in all
> themes consistently.

Alex: what do you think?

Dão: can you file a followup on this issue and CC me?  Thanks!

	(no longer active) Assignee
	Comment 24 • 16 years ago

(In reply to comment #22)
> These icons are also needed for gnomestripe, therefore
> "Search-glass.png" and "Search-glass-rtl.png" (and the appropriate styling)
> should also be added for gnomestripe to:
> /toolkit/themes/gnomestripe/global/icons
> 
> In pinstripe this icon in toolkit is named search-textbox.png (there is no rtl
> search icon for pinstripe), so that one maybe should be renamed to
> Search-glass.png also.
> 
> This is also needed for bug 403154 because my patch in that bug is referring to
> these files.

I guess this deserves a new bug...

	(no longer active) Assignee
	Comment 25 • 16 years ago

Added the chromedir attribute to .searchbox-search.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 26 • 16 years ago

>I guess Alex was opposed to remove images from the theme at this point of the
>release cycle.  Alex: can you confirm, please?

Yeah, beltzner very clearly requested that we not remove images late in the cycle since various extensions could be depending on them.

>> I think we should add and use the search glass as a global toolkit image in all
>> themes consistently.
>
>Alex: what do you think?

I would like us to do a massive overhaul of how we organize icons at some point to reduce all of the redundancy and have much cleaner organization and file names.

	Dão Gottwald [:dao]
	Comment 27 • 16 years ago

If we add chrome://global/skin/icons/Search-glass.png to winstripe only, we create a worse dependency scenario, where the image would be present on Windows but missing on Mac and Linux, making it less likely that the author realizes that there's something wrong.

	Dão Gottwald [:dao]
	Comment 28 • 16 years ago

Comment on attachment 320249 [details] [diff] [review]
Patch (v1.1)

Looks like bug 403154 is going to add Search-glass.png for the other platforms.

	(no longer active) Assignee
	Comment 29 • 16 years ago

Comment on attachment 320249 [details] [diff] [review]
Patch (v1.1)

Requesting approval for this theme change which adds a polish effect to the Add-on Manager window.

	(no longer active) Assignee
	Comment 30 • 16 years ago

Comment on attachment 320249 [details] [diff] [review]
Patch (v1.1)

Oops, I meant to set the approval1.9 flag...

	Mike Schroepfer
	Comment 31 • 16 years ago

Comment on attachment 320249 [details] [diff] [review]
Patch (v1.1)

Sorry we are closed for Firefox 3.0.  This may be a possibility for 3.0.1 or definitely 3.1.

	Darren Kalck [:D-Kalck]
	Comment 33 • 16 years ago

Does it still approval to land ?

	Darren Kalck [:D-Kalck]
	Comment 34 • 16 years ago

+need
Sorry for spam

	Dão Gottwald [:dao]
	Comment 35 • 16 years ago

(In reply to comment #33)
> Does it still approval to land ?

no

	Darren Kalck [:D-Kalck]
	Comment 36 • 16 years ago

Maybe someone should add the checkin-needed keyword ?

	(no longer active) Assignee
	Comment 37 • 16 years ago

Comment on attachment 320249 [details] [diff] [review]
Patch (v1.1)

I think it would be nice to consider it on the branch as well.  Bug 403154 needs this one to be checked in to the branch.

In the meantime, this can be pushed on mozilla-central.

	Dão Gottwald [:dao]
	Comment 38 • 16 years ago

http://hg.mozilla.org/index.cgi/mozilla-central/rev/916a8b289576

I also removed winstripe/mozapps/extensions/searchIcons.png

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 39 • 16 years ago

Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a1pre) Gecko/2008071303 Minefield/3.1a1pre ID:2008071303

	Samuel Sidler (old account; do not CC)
	Comment 40 • 16 years ago

Comment on attachment 320249 [details] [diff] [review]
Patch (v1.1)

Moving approval request out to 1.9.0.3.

	Samuel Sidler (old account; do not CC)
	Comment 41 • 16 years ago

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release). And likewise, doesn't meet the criteria for 1.9.0.4.