Closed Bug 344536 Opened 13 years ago Closed 6 years ago

search bar form autocomplete entries can conceal suggestions

Categories

(Firefox :: Search, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: dylanmccall+1, Assigned: beberveiga)

References

Details

(Whiteboard: [good first bug][mentor=gavin][lang=xul])

Attachments

(2 files, 6 obsolete files)

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: General → Find Toolbar / FastFind
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
OS: All → Windows XP
Hardware: All → PC
OS: Windows XP → All
Hardware: PC → All
Duplicate of this bug: 398455
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.
Whiteboard: [wontfix?]
(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.
Flags: needinfo?(gavin.sharp)
(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).
Flags: needinfo?(gavin.sharp)
Attached patch limits-history-items.patch (obsolete) — Splinter Review
History items are limited to 7.
Attachment #787688 - Flags: review+
Attachment #787688 - Flags: review+ → review?(gavin.sharp)
Attached patch limits-history-items.patch (obsolete) — Splinter Review
Attachment #787688 - Attachment is obsolete: true
Attachment #787688 - Flags: 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-
Attached patch limits-history-items.patch (obsolete) — Splinter Review
Attachment #787735 - Attachment is obsolete: true
Attachment #788282 - Flags: review?(gavin.sharp)
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+
Attached patch search.xml.patch (obsolete) — Splinter Review
Attachment #795709 - Flags: review?(gavin.sharp)
Attached patch single-file-patch.patch (obsolete) — Splinter Review
Merging both in a single file.
Attachment #788282 - Attachment is obsolete: true
Attachment #795709 - Attachment is obsolete: true
Attachment #795709 - Flags: review?(gavin.sharp)
Attachment #795755 - Flags: review?(gavin.sharp)
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.
Attachment #795755 - Attachment is obsolete: true
Attachment #806277 - Flags: checkin+
Looks mostly good, but the commit comment in that patch is missing the "r=gavin" part, which is important :)
Attachment #806277 - Attachment is obsolete: true
Attachment #806909 - Flags: checkin+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/76bb456c9106
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=gavin][lang=xul] → [good first bug][mentor=gavin][lang=xul][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/76bb456c9106
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.