Recently, Firefox landed Bug 218415 on supported window.event We start to see regression in websites (through webcompat.com reports) which uses window.event to determine if the browser should use event.keyCode or event.which for getting the key reference on keypress events. This bug is here to track the regressions for now and determine eventually what will be the best course (curse) of actions. It seems to revolve on a variation of var num; var char; if (window.event) { num = e.keyCode } else if (e.which) { num = e.which } char = String.fromCharCode(num) or var keyCode = window.event ? event.keyCode : event.which; event.keyCode, event.which and event.charCode are antiquated constructs. https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/which https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/charCode event.charCode has a slightly larger support across browsers. event.keyCode returns 0 in Gecko and the character reference in other browsers event.which returns the character reference in Gecko (Netscapism) The modern way to get the key is event.key https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

If we'd set KeyboardEvent.keyCode to the which value, we'd break following implementations: foo.addEventListener("keypress", (aEvent) => { if (!aEvent.keyCode) { // do something for character input. } else { // do something for non-printable keys like Arrow keys, Fn keys, etc. } }); So, I don't think that there are something which we can do for the odd apps since non-undefined window.event does NOT mean specific web browser now.

Ah, but my example will be broken by fix of bug 354358.

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/events/keyboard_event.cc?l=117-120&rcl=9eb9a910e8f9354912a72a8f2e54fe0584ed7bf7 > if (type() == EventTypeNames::keydown || type() == EventTypeNames::keyup) > key_code_ = key.windows_key_code; > else > key_code_ = char_code_; Chromium always sets keyCode value to charCode value when it's keypress event representing native input. Perhaps, we should follow the same behavior after (or at same time) fixing bug 354358?

That sounds reasonable. But I'm not sure what to do with this issue currently. Perhaps we should disable window.event for now.

(In reply to Olli Pettay [:smaug] from comment #4) > That sounds reasonable. Sure, I'll try to create an experimental patch. > But I'm not sure what to do with this issue currently. Perhaps we should > disable window.event for now. Hmm, looks like that it is not controlled by pref. https://reviewboard.mozilla.org/r/233638/diff/17#index_header Will you back it out completely? (Although the assignee, hsivonen is away until 8/6.)

No particular hurry, next merge will be early September, so there is still time to figure out other approaches. Would it be bad to so change keyCode handling before bug 354358?

(In reply to Olli Pettay [:smaug] from comment #6) > Would it be bad to so change keyCode handling before bug 354358? I'm still not sure. However, according to the broken websites of our keypress event dispatching changes, that indicate that such websites refer keyCode value of keypress event on Firefox. So, I guess that this behavior change has similar impact as bug 354358.

(In reply to Olli Pettay [:smaug] from comment #4) > But I'm not sure what to do with this issue currently. Perhaps we should > disable window.event for now. My impression for now is that we benefit more of the addition of window.event than the new issues being created (and reported so far). I would like to keep it for now. We didn't have any top100 site breaking yet. And keeping it might help us discover new patterns.

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/window-event-has-been-added-for-compatibility-but-some-browser-detections-are-broken/

function digitos(n) { return (window.event ? key = n.keyCode : n.which && (key = n.which), key != 8 || key != 13 || key < 48 || key > 57) ? key > 47 && key < 58 || key == 8 || key == 13 : !0 }

This latest report https://webcompat.com/issues/18684 is for a government website in India. People in India can't login and do their tax with Firefox. I guess this raises a bit the priority for a fix.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (offline: 9/21-9/30) from comment #3) > https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/events/ > keyboard_event.cc?l=117-120&rcl=9eb9a910e8f9354912a72a8f2e54fe0584ed7bf7 > > if (type() == EventTypeNames::keydown || type() == EventTypeNames::keyup) > > key_code_ = key.windows_key_code; > > else > > key_code_ = char_code_; > > Chromium always sets keyCode value to charCode value when it's keypress > event representing native input. Perhaps, we should follow the same behavior > after (or at same time) fixing bug 354358? Following Chrome, assuming it's safe enough, and speccing this seems like a good path forward (unfortunately...).

Bug 1493098 breaks wikipedia donation. We can try outreach there, but it seems like we just need to implement and spec event.keyCode mirroring charCode for keypress event. What do you think, Masayuki?

Filed https://github.com/whatwg/dom/issues/701.

(In reply to Mike Taylor [:miketaylr] (62 Regression Engineering Owner) from comment #15) > Filed https://github.com/whatwg/dom/issues/701. ...in the wrong place. New bug: https://github.com/w3c/uievents/issues/213

(marking 63 as disabled, because this will only be seen in Nightly, given 1493869)

(In reply to Mike Taylor [:miketaylr] (62 Regression Engineering Owner) from comment #14) > Bug 1493098 breaks wikipedia donation. We can try outreach there, but it > seems like we just need to implement and spec event.keyCode mirroring > charCode for keypress event. > > What do you think, Masayuki? Yeah, I think so. However, 0 keyCode/charCode value is currently important since we're dispatching non-printable keypress events until fixing bug 354358 and web apps need to distinguish whether coming keypress events are printable or not. After we stop dispatching keypress events for non-printable keys, web apps become not necessary to check keypress events. On the other hand, I think that there is only one risk. If web apps ignore keypress events whose keyCode value is non-zero only when Gecko, we'll meet such another web-compat issue.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (offline: 9/21-9/30) from comment #18) > Yeah, I think so. However, 0 keyCode/charCode value is currently important > since we're dispatching non-printable keypress events until fixing bug > 354358 and web apps need to distinguish whether coming keypress events are > printable or not. After discussing with Masayuki, the references to bug 354358 here should actually refer to bug 968056. That is, Masayuki is concerned that we should ship bug 968056 before or at the same time as changing this, otherwise code such as the following will break: foo.addEventListener("keypress", aEvent => { if (aEvent.charCode) { // Do something for char input } else { // Ignore non-printable key. } }); Regarding the test failures in the try run from comment 9, it looks like at least some are from DevTools where it's not yet clear what DevTools is doing.

I realize that if we do this hack only in web content, we can minimize the regression risk in our source code.

Chrome sets both KeyboardEvent.keyCode and KeyboardEvent.charCode of "keypress" event to same value. On the other hand, our traditional behavior is, sets one of them to 0. Therefore, we need to set keyCode value to charCode value if the keypress event is caused by a non-function key, i.e., it may be a printable key with specific modifier state and/or different keyboard layout for compatibility with Chrome. Similarly, we need to set charCode value to keyCode value if the keypress event is caused by a function key which is not mapped to producing a character. Note that this hack is for compatibility with Chrome. So, for now, it's enough to change the behavior only for "keypress" event handlers in web content. If we completely change the behavior, we need to fix a lot of default handlers and mochitests too. However, it's really difficult because default handlers check whether keypress events are printable or not with following code: > if (event.charCode && > !event.altKey && !event.ctrlKey && !event.metaKey) { or > if (!event.keyCode && > !event.altKey && !event.ctrlKey && !event.metaKey) { So, until we stop dispatching "keypress" events for non-printable keys, we need complicated check in each of them. And also note that this patch changes the behavior of KeyboardEvent::KeyCode() when spoofing is enabled and the instance is initialized by initKeyEvent() or initKeyboardEvent(). That was changed by bug 1222285 unexpectedly and keeping the behavior makes patched code really ugly. Therefore, this takes back the old behavior even if spoofing is enabled.

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/a9db3033d9cb Set KeyboardEvent.keyCode and KeyboardEvent.charCode to same value if the event is "keypress" event r=smaug

Oh... The reports in web-compat do not reference whether keyCode or charCode is referred wrongly by the web app...

If we stop mirroring charCode to keyCode, the broken web apps (probably based on Google Closure) work fine. However, according to comment 0, we need to do it. This is really hacky approach though, we might be able to switch behavior with checking whether window.event is referred or not before first keyCode/charCode value access on the document? Of course, I don't like this approach, but as far as I've heard, this is important bug and should be fixed ASAP.

Ah, no. Cannot fix this case: https://webcompat.com/issues/18073 > function onlyNumbersCharWithDot(e) { > > var keynum; > > if (e.keyCode == 13 ||e.which == 13 ) { > return true; > } > > if (e.keyCode == 9 ||e.which == 9 ) { > return true; > } > if (e.which == 0 ) { > return true; > } > > if (e.keyCode == 8 ||e.which == 8 ) { > return true; > } > > if (e.keyCode == 32 ||e.which == 32) { > return false; > } > > if (window.event) // IE > { > keynum = e.keyCode; > } else if (e.which) // Netscape/Firefox/Opera

Tricky. Do we know why exactly Google Closure is broken?

I've still not found exact point. However, key handling code in rememberthemilk (I assume that is generated by Closure) checks UA string around checking keyCode. I wonder, if we release bug 968056 before changing this behavior, any web apps can fix them easier since they don't need to check whether keypress event is caused by printable key or non-printable key anymore. (Although bug 968056 is still blocked by medium.com.)

https://github.com/google/closure-library/blob/719529feafc9d60eecd2f620756d3e5cc84a9250/closure/goog/events/keyhandler.js#L400-L413 https://github.com/google/closure-library/blob/719529feafc9d60eecd2f620756d3e5cc84a9250/closure/goog/events/keyhandler.js#L439-L449 https://github.com/google/closure-library/blob/719529feafc9d60eecd2f620756d3e5cc84a9250/closure/goog/events/keycodes.js#L413-L415 At least those code checks both UA string and keyCode value.

(Correcting flags as per Bug 1497546)