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

VERIFIED FIXED in Firefox 3.1a1

Status

()

Firefox
Bookmarks & History
P4
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: faaborg, Assigned: Klaas Heidstra)

Tracking

({polish})

Trunk
Firefox 3.1a1
x86
Windows Vista
polish
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [431887 must land first] [RC2-])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

See bug# 393514 for a mockup.
Flags: blocking-firefox3?

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11

Comment 1

11 years ago
Why a search button if your search starts as you type?
(Reporter)

Comment 2

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

Updated

11 years ago
Depends on: 388811
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
Target Milestone: Firefox 3 beta3 → ---

Comment 4

11 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
(Reporter)

Comment 5

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

Comment 6

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

Comment 7

11 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 ;)

Updated

11 years ago
Priority: P3 → P4
Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
(Reporter)

Updated

10 years ago
Blocks: 425582
(Assignee)

Comment 9

10 years ago
Created attachment 319900 [details] [diff] [review]
patch

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.
Attachment #319900 - Flags: review?(mano)
(Assignee)

Comment 10

10 years ago
Created attachment 319901 [details]
screenshot of patch applied on vista
Attachment #319901 - Flags: ui-review?(faaborg)

Comment 11

10 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?)
Assignee: nobody → klaas1988
Depends on: 431887
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.
(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.
(Reporter)

Updated

10 years ago
Attachment #319901 - Flags: ui-review?(faaborg) → ui-review+
(Assignee)

Comment 14

10 years ago
Created attachment 319967 [details] [diff] [review]
patch v2

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.
Attachment #319900 - Attachment is obsolete: true
Attachment #319967 - Flags: ui-review?(faaborg)
Attachment #319967 - Flags: review?(mano)
Attachment #319900 - Flags: review?(mano)
(Assignee)

Comment 15

10 years ago
Created attachment 319968 [details]
screenshot with updated margins
Attachment #319901 - Attachment is obsolete: true
Attachment #319968 - Flags: ui-review?(faaborg)
(Reporter)

Updated

10 years ago
Attachment #319968 - Flags: ui-review?(faaborg) → ui-review+
(Reporter)

Updated

10 years ago
Attachment #319967 - Flags: ui-review?(faaborg) → ui-review+
(Assignee)

Updated

10 years ago
Attachment #319967 - Flags: review?(mano) → review?(dao)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch] [needs review dao]
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.
Attachment #319967 - Flags: review?(dao) → review-
(Assignee)

Comment 17

10 years ago
Created attachment 320320 [details] [diff] [review]
patch v3

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).
Attachment #319967 - Attachment is obsolete: true
Attachment #320320 - Flags: review?(dao)
(Assignee)

Comment 18

10 years ago
Created attachment 320321 [details]
search icons to add to toolkit

These are the icons that need to be added to toolkit.
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;
> }
Attachment #320320 - Flags: review?(dao) → review+
(Assignee)

Comment 20

10 years ago
Created attachment 320347 [details] [diff] [review]
patch v3.1

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.
Attachment #320320 - Attachment is obsolete: true
Attachment #320347 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Whiteboard: [has patch] [needs review dao] → [has patch] [has review] [needs approval]

Updated

10 years ago
Duplicate of this bug: 433143

Comment 22

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

We'll have to get this in 3.0.1.  Thanks!
Attachment #320347 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Whiteboard: [has patch] [has review] [needs approval] → [has patch] [has review] [needs approval][RC2?]
(Assignee)

Updated

10 years ago
Flags: blocking-firefox3- → blocking-firefox3?
This will not block 3.0, no.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [has patch] [has review] [needs approval][RC2?] → [has patch] [has review] [needs approval][RC2-]
(Assignee)

Comment 24

10 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.
Keywords: checkin-needed
Whiteboard: [has patch] [has review] [needs approval][RC2-] → [has patch] [has review] [RC2-]

Comment 25

10 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?
Doesn't hurt to move on with this regardless of bug 388811.
(Assignee)

Comment 27

10 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.
Is this RTL-ready? If so, please nominate for approval1.9.0.1.
(Assignee)

Comment 29

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

Updated

10 years ago
Attachment #320347 - Flags: approval1.9.0.1?
(Assignee)

Updated

10 years ago
Whiteboard: [has patch] [has review] [RC2-] → [has patch] [has review] [needs approval] [RC2-]

Updated

10 years ago
Whiteboard: [has patch] [has review] [needs approval] [RC2-] → [431887 must land first] [has patch] [has review] [needs approval] [RC2-]
Attachment #320347 - Flags: approval1.9.0.1? → approval1.9.0.2?
http://hg.mozilla.org/index.cgi/mozilla-central/rev/25c5f7adddc0
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [431887 must land first] [has patch] [has review] [needs approval] [RC2-] → [431887 must land first] [RC2-]
Target Milestone: --- → Firefox 3.1a1
Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a1pre) Gecko/2008071303 Minefield/3.1a1pre ID:2008071303
Status: RESOLVED → VERIFIED
(Assignee)

Comment 32

10 years ago
This still needs to be fixed for Firefox 3.0.2, so I'm not sure the resolution already should be fixed.
(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.

Updated

10 years ago
No longer depends on: 388811
Comment on attachment 320347 [details] [diff] [review]
patch v3.1

Pushing approval request to 1.9.0.3.
Attachment #320347 - Flags: approval1.9.0.2? → approval1.9.0.3?
Comment on attachment 320347 [details] [diff] [review]
patch v3.1

We're opting not to take this in a maintenance release.
Attachment #320347 - Flags: approval1.9.0.4? → approval1.9.0.4-
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.