Closed Bug 968056 Opened 11 years ago Closed 6 years ago

keypress event shouldn't be fired for non-printable keys

Categories

(Core :: DOM: Events, defect, P3)

Product:
Core
Component:
DOM: Events
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: masayuki, Assigned: masayuki)

References

(Depends on 3 open bugs)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [webcompat:p1][geckoview:klar:p2])

User Story

Business driver: Achieve tier-1 Google Search experience for Gecko on Android
In D3E, keypress event is defined as that it's fired only if the key produces a character value. https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#event-type-keypress However, Gecko dispatches keypress event for all keys except modifier keys. Before, this bug, we need to change all keypress event handlers which handles non-printable keys, sigh...
Assignee: nobody → masayuki
Changing this stuff is super risky. We probably want to be able to have this in Nightlies for couple of cycles and only later in Aurora/Beta/Release
Yeah, I agree with you. Anyway, this needs huge amount of work. I'm not sure *when* we can fix this.
FYI: Anyway, we should use keydown event as far as possible because keypress may not be dispatched if preceding keydown event is consumed. The change (bug 501496) has a lot of regression and I suppose there are still undiscovered　bugs. Making our code use keydown event fixes such risk. NOTE: On other browsers, Enter key causes keypress event even though it's not a printable key. And also Esc key causes keypress event only on IE. I think that we need to keep dispatching keypress event for Enter key.
Ctrl or Alt + printable key doesn't cause keypress event on Chrome and IE. Win + printable key and Ctrl or Shift + Enter causes keypress event on Chrome and IE.
Nice inconsistency everywhere :/
masayuki: when you say the Enter key is not a printable key, are you thinking of <input/>'s? Because in textareas Enter prints a newline, and therefore *should* fire keypress. Also, on a Mac, Alt and many keys print characters, for example, Alt+v = √, and hence should and do fire keypress in Chrome. The bigger problem to me is that if a textarea has 'x' selected, and a user types an 'x' vs. pressing e.g. Alt-Right (on a Mac), I can't think of any robust way to tell those apart. As I pointed out above, .altKey could be true even when typing something printable, though I suppose I could guess when, based on the .which/.keyCode/.charCode of the keydown and keypress, those don't really match up (Alt-x probably doesn't type an 'x', after all); I could also hardcode every key-combo that isn't typing ({Cmd,Alt}-{Left,Right,Up,Down}, Ctrl-{A,E,F,B,N,P}, I'm sure there's more I don't know about). Those ideas are houses of cards glued together by guesses and hope, though. (It is important to me to know whether the user typed something or not because my textarea is hidden and just a source of keyboard events to type and edit math.)
Enter is an input key of control character. Most text editors which can edit multiple line text usually handle as inserting line break, though. keypress event is legacy event in D3E. Therefore, I'll try the best for compatibility with other browsers. So, don't mind. The goal of this bug is, making similar behavior as other browsers'.
We're beginning to see compat issues caused by this behaviour. I know it's a lot of work (I've been involved with this exact rewrite at Opera, and Opera's code was probably simpler than Firefox's) but it should happen..
Flags: webcompat?
See Also: → https://github.com/webcompat/web-bugs/issues/8699
Whiteboard: [webcompat]
Note that this issue is part of an effort between Google and Mozilla to get Google Search working consistently across Chrome and Firefox on Android: https://docs.google.com/document/d/1FIUk5Y5_VmZ8rqHEsFEoXaysJUCRzf3RbvcyuYpim9c/edit# The correct fix for https://github.com/webcompat/web-bugs/issues/7327 is for Firefox to stop sending keypress events for non-printable characters. Once we have the necessary automation support, I will contribute a web-platform-test that Firefox fails but other browsers pass.
Any update on this?
First of all, developers of Core/Firefox usually use keypress event to handle any keyboard events. Therefore, all of them stop using keypress event in new code. Then, we need to rewrite all keypress event handlers which handles non-printable keys as keydown event handler. I guess that it's really terrible that we need to rewrite too many automated tests. Finally, we can stop dispatching keypress event in TextEventDispatcher simply. I think that for reducing the above jobs, we should stop dispatching keypress event only in web contents.
Once changed this should be documented at https://developer.mozilla.org/en-US/docs/Web/Events/keypress. Sebastian
Keywords: dev-doc-needed
Keywords: site-compat
I strongly believe that when we fix this bug, bug 354358 should also be fixed same time. Web apps may need to change their code when we fix this bug. Then, they can fix something for bug 354358 at same time. Fixing those bugs separately is too risky for our marketing at least with current our market share.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=354358
Flags: webcompat? → webcompat+
Whiteboard: [webcompat] → [webcompat:p1]
Depends on: 1442528
User Story: (updated)
Priority: -- → P3
Whiteboard: [webcompat:p1] → [webcompat:p1] [geckoview:klar:p2]
Depends on: 1478559
Whiteboard: [webcompat:p1] [geckoview:klar:p2] → [geckoview:klar:p2]
Setting to webcompat:p1 again per bug 1479964 comment 19, which is also a p1.
Whiteboard: [geckoview:klar:p2] → [webcompat:p1][geckoview:klar:p2]
Depends on: 1496288
Will be fixed by bug 1496288 soon.
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Kohei Yoshino [:kohei] (Bugzilla UX) (FxSiteCompat) from comment #23) > Posted site compatibility note: > https://www.fxsitecompat.com/en-CA/docs/2018/non-printable-keys-no-longer- > fire-keypress-event/ > Update 4: This change has been enabled on all the channels as of Firefox 64 No, starting from 65.
Flags: needinfo?(kohei.yoshino)
Oops, typo. Fixed. Thanks!
Flags: needinfo?(kohei.yoshino)

Hi Chris, how do we nominate changes for the developer-facing release notes? i.e. https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65 ?

I assume the relnote Bugzilla flag is for the user-facing release notes?

I guess this (and maybe bug 354358) should be included there?

Flags: needinfo?(cmills)

(In reply to Brian Birtles (:birtles) from comment #26)

Hi Chris, how do we nominate changes for the developer-facing release notes? i.e. https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65 ?

I assume the relnote Bugzilla flag is for the user-facing release notes?

I guess this (and maybe bug 354358) should be included there?

This is done via the dev-doc-needed keyword, which is aleready present on this bug, and the other one you reference. I check these regularly, so I'm not sure how I missed this one. I'm about to do the documentation for miscellaneous DOM updates for Fx65, so I'll make sure these two are documented as part of that work.

Flags: needinfo?(cmills)

Great, thanks Chris!

I've documented this:

Added a note to the Fx65 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#APIs (see the Removals section)

Added a note to the KeyPress interface and keypress event browser compat:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/Events/keypress#Browser_compatibility

Let me know if this is OK. Thanks!

Flags: needinfo?(masayuki)
Keywords: dev-doc-needed → dev-doc-complete

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #29)

I've documented this:

Added a note to the Fx65 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#APIs (see the Removals section)

Thanks, but keep dispatching Enter, Shift + Enter and Ctrl + Enter for the compatibility with the other browsers. Here is spec issue: https://github.com/w3c/uievents/issues/183

Added a note to the KeyPress interface and keypress event browser compat:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/Events/keypress#Browser_compatibility

Let me know if this is OK. Thanks!

Thank you for your work.

And also, could you document bug 354358 too? It's important change for some web apps which don't care IME with special path. Especially, it fixes a lot of web-compat issue in Korea.

Flags: needinfo?(masayuki)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #30)

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #29)

I've documented this:

Added a note to the Fx65 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#APIs (see the Removals section)

Thanks, but keep dispatching Enter, Shift + Enter and Ctrl + Enter for the compatibility with the other browsers. Here is spec issue: https://github.com/w3c/uievents/issues/183

Ah, thanks for reminding me about that. I've updated all the notes I added to make this clear.

Added a note to the KeyPress interface and keypress event browser compat:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/Events/keypress#Browser_compatibility

Let me know if this is OK. Thanks!

Thank you for your work.

And also, could you document bug 354358 too? It's important change for some web apps which don't care IME with special path. Especially, it fixes a lot of web-compat issue in Korea.

I have that one on my list too — I'll do it next.

You need to log in before you can comment on or make changes to this bug.