Alex Faaborg [:faaborg] (Firefox UX) Reporter
	Description • 17 years ago

Search field in the bookmarks organizer should have a search button inside the field.

See bug# 393514 for a mockup.

	Dão Gottwald [:dao]
	Comment 1 • 17 years ago

Why a search button if your search starts as you type?

	Alex Faaborg [:faaborg] (Firefox UX) Reporter
	Comment 2 • 17 years ago

Good point.  On the mac placing the search glyph on the left and making it non-interactive matches the behavior of their find as you type search fields.  On windows, they don't really have any find as you type fields that I know of (I need to check), but a search glyph on the right will make the field more descriptive, even if the control is functionally useless because it turns into a close button as soon as you start typing.

	Wesley Johnston (:wesj)
	Comment 3 • 17 years ago

Windows has plenty of FAYT search fields now, and the user experience guidelines are pretty much the same as those for Mac:

http://msdn2.microsoft.com/en-us/library/aa511489.aspx

	Mukunda
	Comment 4 • 17 years ago

On linux it isn't 100% obvious that it's even a search box. At least a label or an icon of some sort would make it more obvious

	Alex Faaborg [:faaborg] (Firefox UX) Reporter
	Comment 5 • 17 years ago

Linux doesn't have the self describing text "Search Bookmarks" in the field?

	Mukunda
	Comment 6 • 17 years ago

(In reply to comment #5)
> Linux doesn't have the self describing text "Search Bookmarks" in the field?
> 

No. I'm going to download the nightly build and see if this is still the case, I'm using the Beta 3 build right now and it doesn't have that text.  In the bookmarks sidebar the field has the text "Search:" next to it, though I like the idea of having the text inside the field which disappears once you focus on it.

	Mukunda
	Comment 7 • 17 years ago

Ok, correction: The latest nightly build does have the descriptive text in the search field. At least on Linux this seems like it's behaving properly.

 a "clear" button would be nice, as some other linux apps have that in their autocomplete fields, however, that's not very relevant to this bug.

 I'm starting to question how useful it is for me to be testing the betas since the bugs I see are usually either: 

A) never fixed 
b) already fixed ny the time I discover and report them ;)

	Mike Connor [:mconnor]
	Comment 8 • 17 years ago

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

	Klaas Heidstra Assignee
	Comment 9 • 16 years ago

This adds a search image to the search field in the same way as the cancel button. This is my first patch so I hope this is the right approach.

	Kai Liu
	Comment 11 • 16 years ago

When bug 431887 lands, a copy of the search icons will be added to toolkit and the ones in browser will be deprecated, so you'll probably want to keep an eye on the progress of that bug.  Since you're adding the button globally, you will also need to add styling for pinstripe.  Both gnomestripe and pinstripe already have search icons in toolkit (the ones used for the add-ons manager).

(Dão, I assume that bug 388811 is being pushed off to the next release?)

	(no longer active)
	Comment 12 • 16 years ago

klaas1988: could you please add the hover and depressed states like my patch in bug 431887 as well?  Otherwise we'll have to handle them in a separate bug after this one lands.  It should be quite similar to what I have done in that bug.

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

(In reply to comment #11)
> (Dão, I assume that bug 388811 is being pushed off to the next release?)

Yes.

(In reply to comment #12)
> klaas1988: could you please add the hover and depressed states

It doesn't have such states since it's actually not a submit button. The search starts on input.

	Klaas Heidstra Assignee
	Comment 14 • 16 years ago

This uses the icons from toolkit as soon as they get landed. Also I've changed the margins so that they match the search icon from the add-ons manager. For pinstripe I just added "display: none" because I think pinstripe already has a search icon there. As Dão Gottwald already pointed out this doesn't need hover and active states because "Find As You Type-fields" don't have a search button, but just an image so people can see it is a search field.

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

Comment on attachment 319967 [details] [diff] [review]
patch v2

>Index: browser/components/places/content/organizer.css

>+#searchFilter[filtered="true"] .textbox-input-searchimage {
>+  display: none;
>+}
>+
> .textbox-input-closebutton {
>   display: none;
> }
> 
> #searchFilter[filtered="true"] .textbox-input-closebutton {
>   display: -moz-box;
> }

better:

#searchFilter[filtered="true"] .textbox-input-searchimage ,
#searchFilter:not([filtered="true"]) .textbox-input-closebutton {
  display: none;
}

>Index: browser/themes/gnomestripe/browser/places/organizer.css
>Index: browser/themes/winstripe/browser/places/organizer.css

>+.textbox-input-searchimage {
>+  margin: 2px 0px 2px 0px;

better:

margin: 2px 0;

>+  min-width: 0;
>+  background-color: transparent;
>+  border: none;
>+  padding: 0;
...
>+  width: 16px;
>+  height: 16px;

What's that about?

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

You didn't add the chromedir attribute to the element.

	Klaas Heidstra Assignee
	Comment 17 • 16 years ago

This patch addreses the comments from Dao, it also adds the search icons for gnomestripe and pinstripe to toolkit (winstripe icons are added in bug 431887).

	Klaas Heidstra Assignee
	Comment 18 • 16 years ago

These are the icons that need to be added to toolkit.

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

Comment on attachment 320320 [details] [diff] [review]
patch v3

>Index: browser/components/places/content/organizer.css

>-.textbox-input-closebutton {
>+#searchFilter[filtered="true"] .textbox-input-searchimage ,
>+#searchFilter:not([filtered="true"]) .textbox-input-closebutton {
>   display: none;
> }

remove this:

> #searchFilter[filtered="true"] .textbox-input-closebutton {
>   display: -moz-box;
> }

	Klaas Heidstra Assignee
	Comment 20 • 16 years ago

Final patch with this part removed:
> #searchFilter[filtered="true"] .textbox-input-closebutton {
>   display: -moz-box;
> }
This can't be checked-in until bug 431887 is fixed. I think it is already too late for Firefox 3.0, so this could be something for 3.0.1.

	Mike Schroepfer
	Comment 22 • 16 years ago

Comment on attachment 320347 [details] [diff] [review]
patch v3.1

We'll have to get this in 3.0.1.  Thanks!

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

This will not block 3.0, no.

	Klaas Heidstra Assignee
	Comment 24 • 16 years ago

I think this can be checked-in in mozilla-central in advance so it can later be checked-in to 3.0.1. This can land before bug 431887 but isn't fixed on windows until that one is checked-in.

	Kai Liu
	Comment 25 • 16 years ago

Since this isn't going to make it into 3.0, should we hold off and wait for bug 388811 before doing anything with this?

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

Doesn't hurt to move on with this regardless of bug 388811.

	Klaas Heidstra Assignee
	Comment 27 • 16 years ago

This isn't going into 3.0 but this can go in 3.0.1, I think bug 388811 won't land in the near future. This patch also copies search icons to toolkit which currently are in a place they shouldn't be. Also landing this bug doesn't make it more difficult to fix bug 388811, so there is not a real reason to not land this.

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

Is this RTL-ready? If so, please nominate for approval1.9.0.1.

	Klaas Heidstra Assignee
	Comment 29 • 16 years ago

(In reply to comment #28)
> Is this RTL-ready? If so, please nominate for approval1.9.0.1.

Yes this is RTL-ready, but I can't set the approval1.9.0.1 flag (it is disabled).

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

http://hg.mozilla.org/index.cgi/mozilla-central/rev/25c5f7adddc0

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 31 • 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

	Klaas Heidstra Assignee
	Comment 32 • 16 years ago

This still needs to be fixed for Firefox 3.0.2, so I'm not sure the resolution already should be fixed.

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

(In reply to comment #32)
> This still needs to be fixed for Firefox 3.0.2, so I'm not sure the resolution
> already should be fixed.

There's the fixed1.9.0.2 keyword for that.

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

Comment on attachment 320347 [details] [diff] [review]
patch v3.1

Pushing approval request to 1.9.0.3.

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

Comment on attachment 320347 [details] [diff] [review]
patch v3.1

We're opting not to take this in a maintenance release.

	Gervase Markham [:gerv]
	Comment 36 • 15 years ago

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv