Closed Bug 1407085 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Autocomplete, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: cmcaine, Assigned: masayuki)

References

Details

Attachments

(1 file)

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
Component: Untriaged → DOM: Events
Product: Firefox → Core
Priority: -- → P3
smaug theorized that maybe we trap Escape to get out of fullscreen. Do you know, Xidorn?
Flags: needinfo?(xidorn+moz)
Hmm, maybe I'm wrong here. Do we use esc for something else too.
Masayuki, do you recall.
Flags: needinfo?(xidorn+moz) → needinfo?(masayuki)
If you do |keyboardevent.preventDefault()| in the event handler, it works as your expectation. So, autocomplete or something restores the value, probably.
Flags: needinfo?(masayuki)
(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.
(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.
Component: DOM: Events → Autocomplete
Product: Core → Toolkit
Summary: keydown listener cannot set input element value if event.key == "Escape" → [autocomplete=on] keydown listener cannot set input element value if event.key == "Escape"
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8939745 - Flags: review?(mak77) → review+
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.
Pushed by masayuki@d-toybox.com:
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
https://hg.mozilla.org/mozilla-central/rev/5323dfb26361
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Not a new bug, probably too late for 58 at this stage.
Blocks: 1431923
You need to log in before you can comment on or make changes to this bug.