Event.keyCode is deprecated

[zepumph]

While working on phetsims/ratio-and-proportion#44, I found over at https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode that keyCode is deprecated. We use this a ton, and it didn't seem totally clear how we should go about handling this. Looking at https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent is not uplifting, as there doesn't seem to be an obvious replacement. @jessegreenberg, let's talk about this tomorrow.

[jessegreenberg]

Thanks for bringing up. keycode has been deprecated for a long time, but the last time we looked into it none of the recommended alternatives had wide support. Support looks a bit better these days though.

MDN recommends event.code, but it isn't supported on IE11. https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code

But it looks like event.key now has support on all of PhET's supported platforms:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key

[jessegreenberg]

We discussed this today and both agreed that it is the right thing to do to move away from deprecated keyCode. We should be using key instead since it has support on all of our platforms.

event.key differentiates capitalization, but our code does not. To get around this, at usages we are going to convert all event.key to lower case, then use functions like KeyboardDragListener.shiftKeyDown to determine if shift is down so that those usages don't have to change.

It isn't yet clear how this will impact internationalization since event.key is a string representation of the key pressed. It would be great to learn more about this, maybe testing output from a non-english keyboard.

We feel that this can be addressed at medium priority.

[zepumph]

Note that KeyboardFuzzer currently runs on keyCode, I was using Event.key for RatioInteractionListener.js to try it out, and it isn't set up to work right now.

[zepumph]

I think of above as a workaround until we add support for Event.key in KeyboardFuzzer. @jessegreenberg do you want this in its own issue?

[jessegreenberg]

Thats OK, thanks! Ill take a look as part of this issue.

[jessegreenberg]

PhET is about to drop support for IE11, and so I wondered if event.code should be used instead of event.key. But there is another reason not to use event.code, from MDN:

Warning: This ignores the user's keyboard layout, so that if the user presses the key at the "Y" position in a QWERTY keyboard layout (near the middle of the row above the home row), this will always return "KeyY", even if the user has a QWERTZ keyboard (which would mean the user expects a "Z" and all the other properties would indicate a "Z") or a Dvorak keyboard layout (where the user would expect an "F").

[jessegreenberg]

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key Indicated that certain keys might be different for Edge, so I was wondering if we would have to so something different on that platform. But https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values shows that the differences were for versions older than 16, so we are still good to go.

[zepumph]

I think we should raise the priority on this because of #1145. I'll assign myself.

[zepumph]

I see 95 usages of cased .keyCode in out project. #1053 (comment) makes me feel like we should flip the switch and see how it goes!

[zepumph]

Today during a11y dev meeting, @jessegreenberg and I decided that we want to see Event.key as the replacement, and to for now likely just convert all to lowercase, since keyStateTracker is keeping the shiftKey state in memory. In the future we may be able to utilize the capitalization of the key easier.

Furthermore, we realized that for i18n, we can have translators translate the key strings that we use, and then it will reflect the layout that they use in their keyboards (assumption here that the layout is based/typical of a locale). But that is for future us to handle.

Onward!!

[zepumph]

keyCode has been almost entirely removed from the project in exchange with Event.key.

Summary of work:

• Convert KeyboardUtils to lowercase versions of the Event.key strings found at https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values.
• Replace keyCode usages with key, especially when they compare against KeyboardUtils' constants.
• Create a typeDef called KeyDef, used in type doc, which is the value of Event.key but lowercase.
• Updates to state objects in KeyboardDragListener and KeyboardFuzzer to store key instead of keyCode
• Some wonkyness in tests to get them to run.
• Some rewriting of KeyboardFuzzer, especially for random, see question below.
• Some non-a11y usages of keyCode refactored to use key for consistency.
  @jessegreenberg please review this work.

Main questions from this work:

• Who is responsible for calling toLowerCase(). I found that we needed all constants in KeyboardUtils to be lowercase, because we can't assume that we can treat "w" differently from "Enter"
• KeyDef is being used for clarity and organization, but it defines itself as being lower case, such that even assertions can be called on it when that is the parameter. Is this the right direction? Or should I be more graceful in general. I foresee awkward bugs in the future when you expect something like this to work: event.key === KeyboardUtils.KEY_ENTER, but it needs to be lowercase.
• There were some "clever" uses of keyCode ranges to accomplish tasks, isRangeKey as well as KeyboardFuzzer.triggerRandomKeyDownUpEvents. I created KeyboardUtils.ALL_KEYS to accomplish the random key gen. This is a new feature built into a large refactor, please take a look. What do you think? One of the byproducts of this was adding a bunch more keys to KeyboardUtils.
• Please focus review especially on KeyboardUtils, KeyboardDragListener, KeyboardFuzzer, KeyStateTracker. Sim-specific cases are also good to poke around through and test by running and using the expected interaction.

I tested by making sure that keyboard input worked for a variety of types, including common code and sim-specific implementations that utilized certain keys. Aqua phet brand and fuzzBoard were both passing, so I went for it! Note that I couldn't really test PhET-iO playback support for keyboard and focus because of https://github.com/phetsims/phet-io/issues/1693

Over to @jessegreenberg with a review that should block publication.

[zepumph]

I went through some more usages of Event.key, and took a look at the above commits. I found quite a few spots where Event.code wasn't being used. Many were outside of the sim (like in tools/PhET-iO wrappers/etc). I feel like we should have a consistent approach across the project, so I tried to change all I could.

• In phetsims/sun@3ee0c17 you are hard coding to just look at the left shift. Is that appropriate? Can we support both? I also see this heavily done in KeyStateTracker for all modifiers. Can we support either modifier key, and not just the left keys? I am worried about this regression.
• Please review my above commits, especially how I am using KeyboardUtils.getNumberFromCode.
• I like that we got rid of KeyDef, but also want to note that getEventCode can still return null, and not always a string.

Thanks for doing this conversion.

[samreid]

We saw something in phetsims/geometric-optics#68 that may be related? @zepumph and @jessegreenberg can you please have a look?

[jessegreenberg]

OK thanks - I am pretty sure it is caused by something else and opened #1249 for that issue.

[jessegreenberg]

In phetsims/sun@3ee0c17 you are hard coding to just look at the left shift

I updated the usage in sun and tested, good catch.

I also see this heavily done in KeyStateTracker for all modifiers

In #1249 I updated the portion of correctModifierKeys that removes both left/right modifiers. But this portion still exists

scenery/js/accessibility/KeyStateTracker.js

Lines 162 to 183 in 2660c9e

	// add modifier keys if they aren't down
	if ( domEvent.shiftKey && !this.shiftKeyDown ) {
	this.keyState[ KeyboardUtils.KEY_SHIFT_LEFT ] = {
	keyDown: true,
	key: key,
	timeDown: 0 // in ms
	};
	}
	if ( domEvent.altKey && !this.altKeyDown ) {
	this.keyState[ KeyboardUtils.KEY_ALT_LEFT ] = {
	keyDown: true,
	key: key,
	timeDown: 0 // in ms
	};
	}
	if ( domEvent.ctrlKey && !this.ctrlKeyDown ) {
	this.keyState[ KeyboardUtils.KEY_CONTROL_LEFT ] = {
	keyDown: true,
	key: key,
	timeDown: 0 // in ms
	};
	}

and I don't know how to make it better. From domEvent.altKey there is no way to determine which key is down. But maybe we can convince ourselves that correctModifierKeys will still work correctly? For example, consider a case where you press a key with AltRight but the browser misses the event for AltRight. correctModifierKeys will then add an AltLeft to the keystate. Eventually AltRight is released by user and in the next correctModifierKeys, domEvent.altKey is false so both AltRight and AltLeft are removed from the keystate. That seems to work out.

Please review my above commits, especially how I am using KeyboardUtils.getNumberFromCode.'

Looks very nice, thanks.

@zepumph reassigning to you to review this comment, but the remaining work in KeyStateTracker may be for a dev meeting.

[zepumph]

But maybe we can convince ourselves that correctModifierKeys will still work correctly? . . .

Yes, that makes plenty of sense. Thanks for explaining.

2660c9e

This is the stuff of legends. Thanks for doing that.

• For

  scenery/js/accessibility/KeyStateTracker.js

  Lines 293 to 315 in 2660c9e

  	/**
  	* @returns {boolean} - true if the keyState indicates that the shift key is currently down.
  	* @public
  	*/
  	get shiftKeyDown() {
  	return this.isAnyKeyInListDown( [ KeyboardUtils.KEY_SHIFT_LEFT, KeyboardUtils.KEY_SHIFT_RIGHT ] );
  	}
  	/**
  	* @returns {boolean} - true if the keyState indicates that the alt key is currently down.
  	* @public
  	*/
  	get altKeyDown() {
  	return this.isAnyKeyInListDown( [ KeyboardUtils.KEY_ALT_LEFT, KeyboardUtils.KEY_ALT_RIGHT ] );
  	}
  	/**
  	* @returns {boolean} - true if the keyState indicates that the ctrl key is currently down.
  	* @public
  	*/
  	get ctrlKeyDown() {
  	return this.isAnyKeyInListDown( [ KeyboardUtils.KEY_CONTROL_LEFT, KeyboardUtils.KEY_CONTROL_RIGHT ] );
  	}

  , do you think if would be better to export these publicly for use anywhere on the project?

  scenery/js/accessibility/KeyboardUtils.js

  Lines 42 to 44 in 7f759c0

  	const SHIFT_KEYS = [ KEY_SHIFT_LEFT, KEY_SHIFT_RIGHT ];
  	const CONTROL_KEYS = [ KEY_CONTROL_LEFT, KEY_CONTROL_RIGHT ];
  	const ALT_KEYS = [ KEY_ALT_LEFT, KEY_ALT_RIGHT ];

[jessegreenberg]

Good idea, done above. Back to you to review again.

[zepumph]

Looks good. Anything else here?

[jessegreenberg]

Thanks for reviewing again, I think we are back on track for this issue.