Closed Bug 344536 Opened 13 years ago Closed 6 years ago
search bar form autocomplete entries can conceal suggestions
14.30 KB, image/png
3.33 KB, patch
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1 I would consider this a bit of a useability bug. If I have autocomplete and Search As You Type turned on, typing in the search box will cause search suggestions to be concealed by the autocomplete, which is located on top. Perhaps the two should be flipped around? Reproducible: Always Steps to Reproduce: 1. In the search box, type the first letters to something which you have searched for before. 2. Scroll down through the autocomplete options to see the search suggestions, hidden away at the bottom. Actual Results: Search suggestions, hidden away at the bottom of the list. Expected Results: Search suggestions should have been located at the top of the list. Sorry if I have called anything by the wrong name. I'm not actually sure if these search suggestions were querying a search engine or a dictionary. I'm going to assume that there will eventually be a Getting Started guide, so nothing to worry about. I believe this bug is similar to bug #342980
Component: Find Toolbar / FastFind → Search
QA Contact: general → search
> I believe this bug is similar to bug #342980 > Never mind that similarity guess there. Don't know what I was thinking, really...
A poll would be useful here I think :). How many people actually use the search history and how many would prefer the best place for the suggestions? Personally I never use the search history, not because I never do the same search twice, but because I rarely remember exactly the first letter of the search term, which is necessary for the autocomplete.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: Searchbox: Autocomplete conceals Search Suggestions → search bar form autocomplete entries can conceal suggestions
As Tony mentioned in bug 398455, another possible solution would be to cap form history entries so that some suggestions are always visible. It shouldn't be too hard to tweak the order and number of history and "suggest" results in the search suggest nsIAutocompleteSearch implementation. The total size of the popup is configurable as well, although that's configured in the search bar binding itself (search.xml, "maxrows").
Cannot reproduce this -- it may be behaving differently than described here now. Autocomplete results show up in the url text entry area, while suggestions from history are below in a dropdown.
(In reply to Liz Henry :lizzard from comment #5) > Cannot reproduce this -- it may be behaving differently than described here > now. Autocomplete results show up in the url text entry area, while > suggestions from history are below in a dropdown. Search history still shows up first in the dropdown. See the attached screenshot.
Whiteboard: [good first bug][mentor=gavin][lang=xul]
I am interested to work on this bug and I would I like to more details about this bug. I would also like to know where I should get started.
Thanks kcharchana77 - I've needinfo's gavin, who is assigned as the mentor for this bug.
(In reply to kcarchana77 from comment #7) > I am interested to work on this bug and I would I like to more details about > this bug. I would also like to know where I should get started. The best way to move forward is for you to check out the code, get a build set up (https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions), and start trying to find the code involved. In this case, that's toolkit/components/search/nsSearchSuggestions.js and browser/components/search/content/search.xml. What we probably want to experiment at first is limiting the number of form history results added to the results in nsSearchSuggestions.js's onReadyStateChange method. If you have more specific questions, feel free to ask here in the bug or on IRC (you can find me in #fx-team or #introduction).
History items are limited to 7.
Attachment #787688 - Flags: review+
Attachment #787688 - Flags: review+ → review?(gavin.sharp)
Attachment #787735 - Flags: review?(gavin.sharp)
Comment on attachment 787735 [details] [diff] [review] limits-history-items.patch Thanks for patching this, Willian! Looks good in general, but there are a few comments that need addressing: >Bug #344536 - Limits history items. A better checkin comment might be: Bug 344536: Limit the number of history items displayed the search bar history dropdown, so that some suggestions are always visible >diff --git a/toolkit/components/search/nsSearchSuggestions.js b/toolkit/components/search/nsSearchSuggestions.js > /** >+ * Number of history items displayed. >+ */ >+ _historyLimit: 7, The comment should probably say "Maximum number of history items displayed", and it would also be useful to mention how we came up with this number. You could add a reference to search.xml's maxrows attribute, too. "Maximum number of history items displayed. This is capped at 7 because the primary consumer (Firefox search bar) displays 10 rows by default, and so we want to leave some space for suggestions to be visible." >+ var historyItemsNumber = Math.max(this._formHistoryResult.matchCount, this._historyLimit); I misled you on IRC - you actually want Math.min here :) I might also suggest renaming the variable to something like "maxHistoryItems". Have you tested that this patch produces the correct results? Testing with a search term that produces fewer than 7 results as well as one that produces more than 7 history results is a good idea - with the patch as is the former case would probably produce an exception in the Browser Console.
Attachment #787735 - Flags: review?(gavin.sharp) → review-
Comment on attachment 788282 [details] [diff] [review] limits-history-items.patch Sorry for the delayed response here, Willian! This is good work. One thing I forgot to mention: since we're introducing somewhat of a dependency between this code and the maxrows attribute in search.xml, we should probably add a comment there saying that if we're going to change that number (http://hg.mozilla.org/mozilla-central/annotate/a8daa428ccbc/browser/components/search/content/search.xml#l36), we should update this code too. r=me with that change, let me know on IRC if you need help making that change or getting this landed. Thanks again!
Attachment #788282 - Flags: review?(gavin.sharp) → review+
Merging both in a single file.
Comment on attachment 795755 [details] [diff] [review] single-file-patch.patch Looks good! For future reference, you should ideally follow the steps at https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in (or https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F) to generate a patch with the correct metadata (commit comment, author name, etc.). Running "./mach mercurial-setup" from your mozilla-central checkout is a handy way to have a lot of that done for you automagically.
Attachment #795755 - Flags: review?(gavin.sharp) → review+
I'm assigning this bug to you, since you're working on it. Once you've attached a patch with a commit comment, you should add the "checkin-needed" keyword to the keyword field, and someone will come along and commit your patch for you.
Assignee: nobody → contact
I hope it's alright now. Thank you very much Gavin.
Looks mostly good, but the commit comment in that patch is missing the "r=gavin" part, which is important :)
Whiteboard: [good first bug][mentor=gavin][lang=xul] → [good first bug][mentor=gavin][lang=xul][fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=gavin][lang=xul][fixed-in-fx-team] → [good first bug][mentor=gavin][lang=xul]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.