Closed Bug 1479964 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
DOM: Events
Version:
63 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 --- disabled

People

(Reporter: karlcow, Assigned: masayuki)

References

(
URL
)

Details

(Keywords: regression, site-compat, Whiteboard: [webcompat:p1])

Attachments

(1 file)

Bug 1479964 - Set KeyboardEvent.keyCode and KeyboardEvent.charCode to same value if the event is "keypress" event
46 bytes, text/x-phabricator-request
Details | Review 
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
Flags: webcompat?
Component: Event Handling → DOM: Events
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?
Flags: needinfo?(bugs)
That sounds reasonable.

But I'm not sure what to do with this issue currently. Perhaps we should disable window.event for now.
Flags: needinfo?(bugs)
(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.
Priority: -- → P2
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
}
URL: https://webcompat.com/issues/18573
status-firefox64: --- → affected
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.
Flags: needinfo?(miket)
Flags: webcompat?
Flags: webcompat+
Flags: needinfo?(miket)
Whiteboard: [webcompat] → [webcompat:p1]
(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...).
Blocks: 1493098
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?
Flags: needinfo?(masayuki)
See Also: → 1493869
(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)
status-firefox63: affected → disabled
(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.
Flags: needinfo?(masayuki)
(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.
QA Contact: overholt
QA Contact: overholt
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
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/a9db3033d9cb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1497685
No longer depends on: 1497685
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.
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
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.)
No longer depends on: 1478102
Depends on: 1478102
Depends on: 1502736
Depends on: 1541566
Regressions: 1545410
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: