344536 - search bar form autocomplete entries can conceal suggestions

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Dylan McCall]

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

Comment 1

[Dylan McCall]

> I believe this bug is similar to bug #342980
> 

Never mind that similarity guess there.
Don't know what I was thinking, really...

Comment 2

[Ria Klaassen (not reading all bugmail)]

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. 

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 6

[Matthew N. [:MattN]]

Attachment: Screenshot showing search history before suggestions

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

Comment 7

[kcarchana77]

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.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Thanks kcharchana77 - I've needinfo's gavin, who is assigned as the mentor for this bug.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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

Comment 10

[Willian Gustavo Veiga]

Attachment: limits-history-items.patch

History items are limited to 7.

Comment 11

[Willian Gustavo Veiga]

Attachment: limits-history-items.patch

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 13

[Willian Gustavo Veiga]

Attachment: limits-history-items.patch

Comment 14

[:Gavin Sharp [email: [e-mail]]]

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!

Comment 15

[Willian Gustavo Veiga]

Attachment: search.xml.patch

Comment 16

[Willian Gustavo Veiga]

Attachment: single-file-patch.patch

Merging both in a single file.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 19

[Willian Gustavo Veiga]

Attachment: 344536-limit-the-number-of-history-items-displayed.patch

I hope it's alright now. Thank you very much Gavin.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Looks mostly good, but the commit comment in that patch is missing the "r=gavin" part, which is important :)

Comment 21

[Willian Gustavo Veiga]

Attachment: 344536-limit-the-number-of-history-items-displayed.patch

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/76bb456c9106

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/76bb456c9106