Clay: Replace event.keyCode usages with event.key

[diegonvs]

What is your proposal?

While reviewing a PR, @pat270 brings an important point: .keyCode for identifying a pressed key is no longer recommended.

See: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode

This feature is no longer recommended. Though some browsers might still support it, it may have already been removed from the relevant web standards, may be in the process of being dropped, or may only be kept for compatibility purposes. Avoid using it, and update existing code if possible; see the compatibility table at the bottom of this page to guide your decision. Be aware that this feature may cease to work at any time.

The idea is replace these usages, @pat270 recommended using event.key due to compatibility with IE11, but according https://caniuse.com/#feat=keyboardevent-key: Has non-standard key identifiers and incorrect behaviour with AltGraph..

When grepping our Clay codebase, I noticed We're using on the Modal, MultiSelect and on FocusScope component.

Why would adopting this proposal be beneficial?

Avoid future problems with Keyboard Events.

What are the alternatives to this proposal?

Use .code(but no support for IE11)

[wincent]

I'm not sure what keys we need to support, but the list of non-standard event names that we'd need to cover might be pretty short; ie. this list from MDN might get us pretty far:

  switch (event.key) {
    case "Down": // IE/Edge specific value
    case "ArrowDown":
      // Do something for "down arrow" key press.
      break;
    case "Up": // IE/Edge specific value
    case "ArrowUp":
      // Do something for "up arrow" key press.
      break;
    case "Left": // IE/Edge specific value
    case "ArrowLeft":
      // Do something for "left arrow" key press.
      break;
    case "Right": // IE/Edge specific value
    case "ArrowRight":
      // Do something for "right arrow" key press.
      break;
    case "Enter":
      // Do something for "enter" or "return" key press.
      break;
    case "Esc": // IE/Edge specific value
    case "Escape":
      // Do something for "esc" key press.
      break;

[bryceosterhaus]

For the react based instances where we use keyCode, do you think that will be an issue since it is coming through react's synthetic events?

[wincent]

Ah good point. I can think of two ways to find out: test it in the browser or look in the React source.

I am too lazy to test but not lazy enough not to look at the source:

/**
 * Normalization of deprecated HTML5 `key` values
 * @see https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent#Key_names
 */
const normalizeKey = {
  Esc: 'Escape',
  Spacebar: ' ',
  Left: 'ArrowLeft',
  Up: 'ArrowUp',
  Right: 'ArrowRight',
  Down: 'ArrowDown',
  Del: 'Delete',
  Win: 'OS',
  Menu: 'ContextMenu',
  Apps: 'ContextMenu',
  Scroll: 'ScrollLock',
  MozPrintableKey: 'Unidentified',
};

That's the list of keys that they normalize, so coverage seems pretty good.

[diegonvs]

I'm planning to replace the usages on Clay, Should I add this map on @clayui/shared and reuse it or creating a map with the current key usages on each component that will need these values? 🤔

How Do you guys measure if something could be added to @clayui/shared or not?

[bryceosterhaus]

How Do you guys measure if something could be added to @clayui/shared or not?

We don't have anything clear cut for this. For adding to each each of the use cases, I would say lets just start with copy/paste for each map and then we can evaluate down the road if we want to abstract to @clayui/shared