1055085 - Clear <input type="search"> value on ESC key

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Florens Verschelde :fvsch]

Following a contemporary usability convention, <input type="search"> elements should be cleared of all text (value set to "") when the user hits the ESC key.

This is already the case for <input type=search> in:
- Chrome (tested: 36)
- Safari (tested: 7.0)
- Internet Explorer (tested: 10.0)

In Firefox, this behavior is applied somewhat consistently in the UI:
- Awesomebar ✓
- Bookmarks sidebar ✓
- Library ✓
- about:addons ✓
- about:config ✓
- about:preferences#applications ✓
- etc.

… with a few issues:
- Search box ✗ (not working)
- Find in page ✗ (ESC closes the Find toolbar)

It's also a convention that is widely used in OSX, and apparently in Windows as well (tested briefly in Win7).

If implemented, this behavior should be preventable with `event.preventDefault()` (KeyboardEvent.prototype.preventDefault).

Note that Bug 558594 set out to add a default style to search inputs. It has some discussion of behavior when it comes to adding a Clear button, but I reckon the keyboard control aspect can be handled separately (and sooner).

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix+test

This works for <input type=search> but the Search Bar use type=text
so it needs to be fixed separately (bug 1055030).

Comment 2

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

Do other browsers clear the value as a default action to ESC keydown or keyup or keypress?

Comment 3

[Florens Verschelde :fvsch]

Attachment: Testing which event browsers use for clearing on ESC

(In reply to Olli Pettay [:smaug] from comment #2)
> Do other browsers clear the value as a default action to ESC keydown or
> keyup or keypress?

Chrome and Safari: on keydown.
Internet Explorer: don’t know, but you can test with the above test page.

Tested using event.preventDefault() on either keyup, keydown or keypress.

Comment 4

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

The test is testing with which event preventDefault works to prevent ESC to clear the value, but which event actually clears to value?

Comment 5

[Florens Verschelde :fvsch]

Could cancelling one event prevent behavior attached to a different event? (When cancelling the keydown event, theKeyupEvent.defaultPrevented remains false, so it seems unlikely.)

I also did some empirical tests:

If you hold on the ESC key for 5 seconds, the field is cleared instantly but the keyup event will happen 5 seconds later.

Chrome and Safari follow the DOM3 spec which says:
> If supported by a user agent, this event [keypress] MUST
> be dispatched when a key is pressed down, if and only if
> that key normally produces a character value.

Safari and Chrome don’t dispatch the "keypress" event for the ESC key. Hence our only two candidates are "keydown" and "keyup", and we already ruled out "keyup". So keydown it is.

Comment 6

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

Comment on attachment 8480432 [details] [diff] [review]
fix+test

Ok, thanks, then we should check NS_KEY_DOWN and not NS_KEY_PRESS

Comment 7

[Mats Palmgren (inactive)]

Attachment: KEY_DOWN version

After switching to KEY_DOWN there were problems with form-fill
which restores the value in KEY_PRESS (thus negating our clear):

(gdb) bt
#0  mozilla::dom::HTMLInputElement::SetValueInternal (this=0x7fffaec2d400, aValue=..., aUserInput=true, aSetValueChanged=true) at content/html/content/src/HTMLInputElement.cpp:2793
#1  mozilla::dom::HTMLInputElement::SetUserInput (this=0x7fffaec2d400, aValue=...) at content/html/content/src/HTMLInputElement.cpp:2396
#2  non-virtual thunk to mozilla::dom::HTMLInputElement::SetUserInput(nsAString_internal const&) () at Unified_cpp_html_content_src1.cpp:2403
#3  nsFormFillController::SetTextValue (this=0x7fffba0c62f0, aTextValue=...) at toolkit/components/satchel/nsFormFillController.cpp:490
#4  non-virtual thunk to nsFormFillController::SetTextValue(nsAString_internal const&) () at toolkit/components/satchel/nsFormFillController.cpp:494
#5  nsAutoCompleteController::RevertTextValue (this=0x7fffb9b4d6a0) at toolkit/components/autocomplete/nsAutoCompleteController.cpp:1288
#6  nsAutoCompleteController::HandleEscape (this=0x7fffb9b4d6a0, _retval=0x7fffffff9a03) at toolkit/components/autocomplete/nsAutoCompleteController.cpp:300
#7  nsFormFillController::KeyPress (this=0x7fffba0c62f0, aEvent=0x7fffaedc8ab0) at toolkit/components/satchel/nsFormFillController.cpp:942

I switched form-fill to KEY_DOWN as well (this patch) but that caused
test failures related to closing of popups:
toolkit/components/satchel/test/test_bug_511615.html 
toolkit/content/tests/chrome/test_autocomplete_with_composition_on_input.html

https://tbpl.mozilla.org/?tree=Try&rev=fda2b8d6fd7d

Comment 8

[Mats Palmgren (inactive)]

Attachment: KEY_DOWN + split HandleEscape

Splitting nsAutoCompleteController::HandleEscape into a KEY_DOWN part that
clears the text value, and a KEY_PRESS part that closes the popup fixed
the auto-complete related test failures above.  There are still some
browser/devtools/ test failures though.  I don't know if that's a bug
in my changes or just devtools that expect ESC have no built-in
behavior for type=search.

Here's an example regression:
1. open Devtools Inspector
2. click the search box
3. type "h" => a popup menu appears (containing html, head etc)
4. type ESC => the popup is closed
5. type ESC again => the text field is cleared
6. type ESC again => the text field is minimized and a console panel
   appears at the bottom of the window, taking focus

with this patch, ESC in 5 triggers the action in 5 *and* 6.

https://tbpl.mozilla.org/?tree=Try&rev=0f03fa536584

Comment 9

[Mats Palmgren (inactive)]

This was way more complicated than I have time for so I'm not going
to work further on this bug for now - feel free to take it.

Comment 10

[Florens Verschelde :fvsch]

> I don't know if that's a bug in my changes or just devtools
> that expect ESC have no built-in behavior for type=search.

I think the devtools code has used <input type="search"> quite a bit, and added their own event management and custom implementations of "ESC clears text and/or does other stuff we deem useful". This is exemplified by Bug 902990 for instance.

Perhaps someone familiar with this kind of devtools code could be asked for feedback?

Comment 11

[Mats Palmgren (inactive)]

See last comment.  Can someone in Devtools take it from here?

Comment 12

[Paul Rouget [:paul]]

Brian, you worked on the ESC key behavior in the devtools. Can you help?

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: devtools-toolbox-search-esc.patch

Rather knock this down case by case in devtools, we could just ignore the split console ESC handling from the toolbox when it is cleared in this manner.  One problem I'm seeing here is that by the time the keydown event fires, it seems like the value has already been emptied (so I don't see how to distinguish between the ESC in #5 and the ESC in #6.

Is this expected behavior?  Usually during keydown callbacks, e.target.value has not yet changed from the value before the keydown.

Comment 14

[Mats Palmgren (inactive)]

Attachment: 1055085-toolbox-search-esc-2.patch

(In reply to Brian Grinstead [:bgrins] from comment #13)
> [...] One problem I'm seeing here is that by the time the keydown event
> fires, it seems like the value has already been emptied (so I don't see how
> to distinguish between the ESC in #5 and the ESC in #6.

The event handler you patched is for keypress, not keydown.
If I change it to keydown then your patch seems to work fine
for the STR in comment 8.

> Usually during keydown callbacks, e.target.value
> has not yet changed from the value before the keydown.

Yes.

typing ESC in IE:
keydown, value is intact
keypress, value is empty
keyup, value is empty

typing ESC in Chrome (on Linux):
keydown, value is intact
keyup, value is empty
(no keypress event were fired for ESC)

With the patches here, we're compatible with IE.

Comment 15

[Mats Palmgren (inactive)]

With the additional browser/devtools/ patches:
https://tbpl.mozilla.org/?tree=Try&rev=3041113f55fd

Comment 16

[Mats Palmgren (inactive)]

It still breaks a few devtools tests it seems.

Comment 17

[Mats Palmgren (inactive)]

Attachment: fix+test

Unbitrotted rollup patch.

Comment 18

[Brian Grinstead [:bgrins]]

New try push: https://tbpl.mozilla.org/?tree=Try&rev=3897a7676b0a.  There are a bunch of failing tests - some of them (like the inspector tests) I'm not seeing locally unfortunately.

One that I am seeing and is interesting is that the debugger textbox does not clear when the debugger is paused with this patch applied.  It can be seen in browser/devtools/debugger/test/browser_dbg_search-global-06.js - in this case there is an escape key sent to the window after focusing the debugger textbox [0].  However, this does not clear the textbox if the debugger is paused (I can confirm that this doesn't work just running the browser with the patch applied).  I tried to figure out why, but nothing was forthcoming and I don't have the time right now to track it down.  Maybe Victor would have a better idea of what is going on?

Comment 19

[Victor Porof [:vporof][:vp]]

I don't know. ESC should clear a textbox if it's focused.

Comment 20

[sjw]

I *love* this behavior, especially for password fields. Would be great to have this in Firefox too.
I think it's easier to implement this for password fields that for search fields, because they are not used in devtools.
Can we use this bug for the general behavior or is it just for input[type=search]?

Comment 21

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 22

[sjw]

This would also be great to have for <input type="password">. This works at least in IE, OS login screens and some desktop apps.

Comment 23

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1055085 - Clear input search on esc keydown. r=smaug!,masayuki!,ayeddi!

This matches Blink and WebKit, with the difference that this preserves
the undo stack (which I think is useful and mitigates a bit the
double-esc concerns Masayuki had in bug 1936648).

Keydown because it matches both other engines, though I think keypress
would've been more consistent...

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a3456a90a9c4
https://hg.mozilla.org/integration/autoland/rev/1adee7e0809c
Clear input search on esc keydown. r=smaug,masayuki

Comment 25

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/1adee7e0809c