1407085 - [autocomplete=on] keydown listener cannot set input element value if event.key == "Escape"

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect, P3)

Description

[Col]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170918122929

Steps to reproduce:

Version set as 57, but bug in at least 55, 57, 58.

Added a keydown listener to an input and tried to set input.value = "" if keyevent.key === "Escape".

See worked example:

https://jsfiddle.net/d7xugxen/


Actual results:

Pressing escape does not clear the input


Expected results:

Pressing escape should have caused the event handler to clear the input

Comment 1

[Andrew Overholt [:overholt]]

smaug theorized that maybe we trap Escape to get out of fullscreen. Do you know, Xidorn?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Hmm, maybe I'm wrong here. Do we use esc for something else too.
Masayuki, do you recall.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If you do |keyboardevent.preventDefault()| in the event handler, it works as your expectation. So, autocomplete or something restores the value, probably.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Andrew Overholt [:overholt] from comment #1)
> smaug theorized that maybe we trap Escape to get out of fullscreen. Do you
> know, Xidorn?

If fullscreen traps that button, the handler should not be triggered at all, so sounds like it isn't that case.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3)
> If you do |keyboardevent.preventDefault()| in the event handler, it works as
> your expectation. So, autocomplete or something restores the value, probably.

It seems you're right that autocomplete may be where the issue is.

If you set autocomplete="off" on that input, escape works as expected, too.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc9f81498554221d10b4907d609cc6352560a7f4

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71a5de8f2afa01497d2c15e115f757f4359bc4be

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b928bf587309ba4fe586130cce1d37dd127ed34

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1407085 - nsAutoCompleteController shouldn't restore original value after somebody changes the input value even when Escape key is pressed

When Escape key is pressed, nsAutoCompleteController needs to restore last
string which was default value of the input or typed by the user.  However,
somebody may change the value, e.g., an event listener which handles
Escape key.  In this case, nsAutoCompleteController shouldn't restore the
last string.

Unfortunately, when JS sets input value, DOM "input" event won't be fired.
Therefore, nsAutoCompleteController doesn't have a chance to modify
mSearchString in this case.  Therefore, nsAutoCompleteController needs to
store expected input string for checking if somebody modified the input value.
For solving this issue, this patch adds a new member, mSetValue which is
modified when the input value is modified by nsAutoCompleteController itself
or mSearchString is modified.

Even with this patch, if user temporarily selects an item of the popup and
JS sets same value as the selected item from JS, nsAutoCompleteController
restores the input value with mSearchString.  However, this must be rare
case and I don't have idea to fix this issue with simple patches.

Review commit: https://reviewboard.mozilla.org/r/208402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208402/

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8939745 [details]
Bug 1407085 - nsAutoCompleteController shouldn't restore original value after somebody changes the input value even when Escape key is pressed

https://reviewboard.mozilla.org/r/208402/#review216138

thank you

::: toolkit/components/autocomplete/nsAutoCompleteController.h:55
(Diff revision 1)
> +
> +  /**
> +   * SetSearchStringInternal() sets both mSearchString and mSetValue to
> +   * aSearchString.
> +   */
> +  void SetSearchStringInternal(const nsAString& aSearchString)

Is there a reason to not just change SetSearchString and document above mSearchString that it should only be modified through SetSearchString?
The only difference of this internal method is that it doesn't return NS_OK.

::: toolkit/components/autocomplete/nsAutoCompleteController.h:160
(Diff revision 1)
>  
> +  // mSearchString stores value which is the original value of the input or
> +  // typed by the user.  When user is choosing an item from the popup, this
> +  // is NOT modified by the item because this is used for reverting the input
> +  // value when user cancels choosing an item from the popup.
>    nsString mSearchString;

Worth documenting that changing mSearchString should only happen through the Set method (whatever it ends up being).

::: toolkit/components/satchel/test/mochitest.ini:11
(Diff revision 1)
>    subtst_privbrowsing.html
>    parent_utils.js
>  
>  [test_bug_511615.html]
>  [test_bug_787624.html]
> +[test_bug_1407085.html]

why is the test in satchel, rather than in toolkit/content/tests/widgets where the other autocomplete tests are?
Satchel is only about form fill, not generic autocomplete.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8939745 [details]
Bug 1407085 - nsAutoCompleteController shouldn't restore original value after somebody changes the input value even when Escape key is pressed

https://reviewboard.mozilla.org/r/208402/#review216138

> Is there a reason to not just change SetSearchString and document above mSearchString that it should only be modified through SetSearchString?
> The only difference of this internal method is that it doesn't return NS_OK.

Yes, SetSearchString() is a virtual method which is derived from nsIAutoCompleteController. For keeping Quantum Flow, we should not use virtual call as far as possible.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8939745 [details]
Bug 1407085 - nsAutoCompleteController shouldn't restore original value after somebody changes the input value even when Escape key is pressed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208402/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5323dfb26361
nsAutoCompleteController shouldn't restore original value after somebody changes the input value even when Escape key is pressed r=mak

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/5323dfb26361

Comment 15

[Julien Cristau [:jcristau]]

Not a new bug, probably too late for 58 at this stage.