Closed
Bug 403154
Opened 17 years ago
Closed 17 years ago
Search field in the bookmarks organizer should have a search button inside the field
Categories
(Firefox :: Bookmarks & History, defect, P4)
Tracking
()
VERIFIED
FIXED
Firefox 3.1a1
People
(Reporter: faaborg, Assigned: klaas1988)
References
Details
(Keywords: polish, Whiteboard: [431887 must land first] [RC2-])
Attachments
(3 files, 4 obsolete files)
24.00 KB,
image/png
|
faaborg
:
ui-review+
|
Details |
2.66 KB,
application/x-zip-compressed
|
Details | |
9.77 KB,
patch
|
samuel.sidler+old
:
approval1.9.0.4-
|
Details | Diff | Splinter Review |
Search field in the bookmarks organizer should have a search button inside the field.
See bug# 393514 for a mockup.
Flags: blocking-firefox3?
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
Comment 1•17 years ago
|
||
Why a search button if your search starts as you type?
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.
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
Updated•17 years ago
|
Target Milestone: Firefox 3 beta3 → ---
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•17 years ago
|
||
Linux doesn't have the self describing text "Search Bookmarks" in the field?
(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.
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•17 years ago
|
Priority: P3 → P4
Comment 8•17 years ago
|
||
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+
Assignee | ||
Comment 9•17 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.
Attachment #319900 -
Flags: review?(mano)
Assignee | ||
Comment 10•17 years ago
|
||
Attachment #319901 -
Flags: ui-review?(faaborg)
Comment 11•17 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
Comment 12•17 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.
Comment 13•17 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.
Reporter | ||
Updated•17 years ago
|
Attachment #319901 -
Flags: ui-review?(faaborg) → ui-review+
Assignee | ||
Comment 14•17 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.
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•17 years ago
|
||
Attachment #319901 -
Attachment is obsolete: true
Attachment #319968 -
Flags: ui-review?(faaborg)
Reporter | ||
Updated•17 years ago
|
Attachment #319968 -
Flags: ui-review?(faaborg) → ui-review+
Reporter | ||
Updated•17 years ago
|
Attachment #319967 -
Flags: ui-review?(faaborg) → ui-review+
Assignee | ||
Updated•17 years ago
|
Attachment #319967 -
Flags: review?(mano) → review?(dao)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch] [needs review dao]
Comment 16•17 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.
Attachment #319967 -
Flags: review?(dao) → review-
Assignee | ||
Comment 17•17 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).
Attachment #319967 -
Attachment is obsolete: true
Attachment #320320 -
Flags: review?(dao)
Assignee | ||
Comment 18•17 years ago
|
||
These are the icons that need to be added to toolkit.
Comment 19•17 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;
> }
Attachment #320320 -
Flags: review?(dao) → review+
Assignee | ||
Comment 20•17 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.
Attachment #320320 -
Attachment is obsolete: true
Attachment #320347 -
Flags: approval1.9?
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [has patch] [needs review dao] → [has patch] [has review] [needs approval]
Comment 22•17 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•17 years ago
|
Whiteboard: [has patch] [has review] [needs approval] → [has patch] [has review] [needs approval][RC2?]
Assignee | ||
Updated•17 years ago
|
Flags: blocking-firefox3- → blocking-firefox3?
Comment 23•17 years ago
|
||
This will not block 3.0, no.
Flags: blocking-firefox3? → blocking-firefox3-
Updated•17 years ago
|
Whiteboard: [has patch] [has review] [needs approval][RC2?] → [has patch] [has review] [needs approval][RC2-]
Assignee | ||
Comment 24•17 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•17 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?
Comment 26•17 years ago
|
||
Doesn't hurt to move on with this regardless of bug 388811.
Assignee | ||
Comment 27•17 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.
Comment 28•17 years ago
|
||
Is this RTL-ready? If so, please nominate for approval1.9.0.1.
Assignee | ||
Comment 29•17 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).
Attachment #320347 -
Flags: approval1.9.0.1?
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch] [has review] [RC2-] → [has patch] [has review] [needs approval] [RC2-]
Whiteboard: [has patch] [has review] [needs approval] [RC2-] → [431887 must land first] [has patch] [has review] [needs approval] [RC2-]
Updated•17 years ago
|
Attachment #320347 -
Flags: approval1.9.0.1? → approval1.9.0.2?
Comment 30•17 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 17 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
Comment 31•17 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
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 32•17 years ago
|
||
This still needs to be fixed for Firefox 3.0.2, so I'm not sure the resolution already should be fixed.
Comment 33•17 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.
Comment 34•16 years ago
|
||
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 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.
Attachment #320347 -
Flags: approval1.9.0.4? → approval1.9.0.4-
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•