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

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

Split out the clicking part into bug 1116865 as the problems are fairly different.

Comment 2

[Dave Townsend [:mossop]]

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

Comment 3

[Neil Deakin]

Attachment: Don't close popup when the caret is moved

Comment 4

[Neil Deakin]

This builds on top of the patch in bug 1116865.

Comment 5

[Neil Deakin]

Attachment: Don't close popup when the caret is moved

Update uuid as well.

Comment 6

[Marco Bonardo [:mak]]

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?

Comment 7

[Neil Deakin]

Attachment: searchcaret-1116865

There were some test failures on Windows, caused by not waiting for the synthesized mouse events to occur.

Comment 8

[Neil Deakin]

I filed bug 1126772 on adding synthesizeNativeMouse to EventUtils so other tests don't make this same mistake.

Comment 9

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e5eb798931

Comment 10

[Neil Deakin]

I checked this in, since the latest patch review is just a test change.

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8e5eb798931

Comment 13

[Petruta Horea [:phorea]]

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.

Comment 14

[Florian Quèze [:florian]]

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.