Closed Bug 1114707 Opened 9 years ago Closed 9 years ago

Search suggestions should stay open when using cursor keys to move the caret in the search box

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb

People

(Reporter: mossop, Assigned: enndeakin)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

We want the search suggestions to stay open whenever the user is still interacting with the search box. Currently using cursor keys or clicking to move the caret closes the search suggestions and they reopen after typing more.
Flags: qe-verify+
Flags: firefox-backlog+
Split out the clicking part into bug 1116865 as the problems are fairly different.
Summary: Search suggestions should stay open when using cursor keys and clicking to move the caret in the search box → Search suggestions should stay open when using cursor keys to move the caret in the search box
The problem here is that the keypress handler in autocomplete.xml calls nsAutoCompleteController.handleKeyNavigation for the cursor keys that would move the caret and that forcibly closes the popup. I'm not sure why.

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#437
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#515
Assignee: dtownsend → enndeakin
This builds on top of the patch in bug 1116865.
Depends on: 1116865
Update uuid as well.
Attachment #8544003 - Attachment is obsolete: true
Iteration: --- → 37.3 - 12 Jan
Status: NEW → ASSIGNED
Attachment #8544006 - Flags: review?(mak77)
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
Comment on attachment 8544006 [details] [diff] [review]
Don't close popup when the caret is moved

Review of attachment 8544006 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/autocomplete/nsIAutoCompleteInput.idl
@@ +150,5 @@
> +
> +  /*
> +   * Don't rollup the popup when the caret is moved.
> +   */
> +  readonly attribute boolean noRollupOnCaretMove;

why bumping the uuid of nsIAutoCompleteController if you changed nsIAutoCompleteInput?
Is there some relationship/inheritance I'm missing?
Attachment #8544006 - Flags: review?(mak77) → review+
Iteration: 38.1 - 26 Jan → 38.2 - 9 Feb
There were some test failures on Windows, caused by not waiting for the synthesized mouse events to occur.
Attachment #8544006 - Attachment is obsolete: true
Attachment #8555920 - Flags: review?(mak77)
I filed bug 1126772 on adding synthesizeNativeMouse to EventUtils so other tests don't make this same mistake.
I checked this in, since the latest patch review is just a test change.
Comment on attachment 8555920 [details] [diff] [review]
searchcaret-1116865

Review of attachment 8555920 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/search/test/browser_searchbar_openpopup.js
@@ +30,2 @@
>  
> +    aElement.addEventListener("mouseup", eventOccurred, true);

well, the only comment I could have is about the exotic style of the function definition.
this would have been more compact and readable:
aElement.addEventListener("mouseup", function handle() {
  aElement.removeEventListener("mouseup", handle, true);
  resolve();
}, true);

but it's really a nit.

Just as an info, is this simply a more formal way to executeSoon, or is the event really dispatching asynchronously?
I was under the impression simply returning Promise.resolve() would have obtained the same result, in the next tick the event would have been dispatched already.
Attachment #8555920 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/e8e5eb798931
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
QA Contact: petruta.rasa
Verified as fixed using Nightly 38.0a1 2015-02-08 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
No longer blocks: 1131685
Comment on attachment 8555920 [details] [diff] [review]
searchcaret-1116865

Review of attachment 8555920 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/search/test/browser_searchbar_openpopup.js
@@ +383,5 @@
> +
> +// Moving the caret using the cursor keys should not close the popup.
> +add_task(function* dont_rollup_oncaretmove() {
> +  gURLBar.focus();
> +  textbox.value = "long text";

This should be cleaned up at the end of the test; the test I'm writing for bug 1126250 behaved differently when running as a single test or when running with the whole folder because of this.
Depends on: 1242764
You need to log in before you can comment on or make changes to this bug.