1479964 - Tracking event.keyCode issue due to the implementation of window.event

Status: RESOLVED FIXED

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

Description

[Karl Dubost💡 :karlcow]

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

Comment 1

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

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.

Comment 2

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

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

Comment 3

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

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?

Comment 4

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

That sounds reasonable.

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

Comment 5

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

(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.)

Comment 6

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

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?

Comment 7

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

(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.

Comment 8

[Karl Dubost💡 :karlcow]

(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.

Comment 9

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

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

Comment 10

[Kohei Yoshino]

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/

Comment 11

[Karl Dubost💡 :karlcow]

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
}

Comment 12

[Karl Dubost💡 :karlcow]

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.

Comment 13

[Mike Taylor [:miketaylr]]

(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...).

Comment 14

[Mike Taylor [:miketaylr]]

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?

Comment 15

[Mike Taylor [:miketaylr]]

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

Comment 16

[Mike Taylor [:miketaylr]]

(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

Comment 17

[Mike Taylor [:miketaylr]]

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

Comment 18

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

(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.

Comment 19

[Brian Birtles (:birtles)]

(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.

Comment 20

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

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

Comment 21

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

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

Comment 22

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

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

Comment 23

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

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

Comment 24

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

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

Comment 25

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

Attachment: Bug 1479964 - Set KeyboardEvent.keyCode and KeyboardEvent.charCode to same value if the event is "keypress" event

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.

Comment 26

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

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a9db3033d9cb

Comment 29

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

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

Comment 30

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

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.

Comment 31

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

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

Comment 32

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

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

Comment 33

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

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.)

Comment 34

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

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.

Comment 35

[Kohei Yoshino]

(Correcting flags as per Bug 1497546)

Comment 36

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

https://groups.google.com/forum/#!topic/mozilla.dev.platform/IWLLJmoGroA