Skip to content

keyUpEvents/keyDownEvents check code, not key #11049

Description

@Saadnajmi

Problem Description

Per the keyboard API spec, the props keyDownEvents and keyUpEvents are used to specify which events should be marked as handled natively (so that they are only handled in JS). The spec says you specify an array of IHandledKeyboardEvent objects, each of which contains a key field and booleans for the modifier keys:

  const handledNativeKeyboardEvents: IHandledKeyboardEvent[] = [
     { key: 'Esc' },
     { key: 'Enter', ctrlKey : true, eventPhase : EventPhase.Capturing }
  ];

However, if we look a the actual type and implementation , we aren't checking the key field, but instead the code field.

<Pressable
  keyDownEvents={[
    {code: 'KeyW', handledEventPhase: 3},
    {code: 'KeyE', handledEventPhase: 1},
  ]}
  ...
  >

These two differ quite heavily and make it harder to write cross platform code. Both rn-win32 and rn-macOS check the key field in their respective implementation.

This came up as an issue while I was working on FluentUI React Native, where I'm trying to write cross-platform keyboard handlers and having a very hard time :')

Steps To Reproduce

Look at PressableExample.windows.js and see that all the keyboarding examples have objects that look like [{code: 'keyW'}, {code: 'keyH'}]

Expected Results

Should be more like [{key:'W'},{key:'H'}]

CLI version

n/a

Environment

n/a

Target Platform Version

None

Target Device(s)

Desktop

Visual Studio Version

None

Build Configuration

None

Snack, code example, screenshot, or link to a repository

I guess, this is my keyboard handler. The windows version of it is wrong because it uses key :')

https://github.com/microsoft/fluentui-react-native/blob/main/packages/utils/interactive-hooks/src/useKeyProps.ts

Activity

  1. ghost added
    Needs: Triage 🔍New issue that needs to be reviewed by the issue management team (label applied by bot)
    on Dec 31, 2022
  2. added
    Breaking ChangeThis PR will break existing apps and should be part of the known breaking changes for the release
    and removed
    Needs: Triage 🔍New issue that needs to be reviewed by the issue management team (label applied by bot)
    on Jan 5, 2023
  3. added this to the 0.72 milestone on Jan 5, 2023
  4. rozele commented on Jan 11, 2023

    @rozele
    Contributor

    Should we support both?

    Code aligns to https://www.w3.org/TR/uievents-code/
    Key aligns to https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values

    It's not clear which is a better standard, but I would think we'd want to lean more on W3 standards than MDN specs.

  5. rozele commented on Jan 11, 2023

    @rozele
    Contributor

    Not sure if we already have another issue for this, but I think it's perhaps even more important to reconcile whether we want all key events on focused views to be emitted (as on Windows) or only those declared (as on macOS). Seems to me that emitting all events is perhaps more aligned with some of the PointerEvents APIs (e.g., onPointerDown / onPointerUp), but there are others that emit only when subscribed (e.g., onPointerEnter / onPointerMove). Perhaps we should get a bit more "creative" on Windows, and only emit the event if there's an actual subscriber in the path (though arguably apps that have things like shortcuts configured on the root would pretty much always have onKeyUp/onKeyDown subscribed).

    If we were to follow the pattern of always emitting key events (which I believe is a bit more difficult on macOS than it is on Windows), we may not even need keyDown/UpEvents, if we were to lean on synchronous events in Fabric - the idea being that if you wanted to mark an event as handled, we could subscribe onKeyDown/UpSync, set preventDefault, and have this prevent native behaviors...

  6. Saadnajmi commented on Jan 11, 2023

    @Saadnajmi
    ContributorAuthor

    I've been going with "long term goal is to emit all events on macOS" and have a draft to support that: microsoft/react-native-macos#1615 . I haven't considered how synchronous events would change the API as that seemed a ways away.

    I think both key and code are valid properties with semantically different meanings. We can have both and maybe even keyDownEvents can filter on either/or. The bug is simply that right now, it filters on "code" which is not obvious and disagrees with the spec I linked / makes it harder to write cross platform code.

  7. rozele commented on Jan 11, 2023

    @rozele
    Contributor

    Indeed - we have this pain today in Messenger, and use this ugly little trick:

    export function makeWindowsKeyEventData(data: KeyEventData) {
      let code = data.key;
    
      // See https://www.w3.org/TR/uievents/#code-examples
      const keyToCodeMap = {
        ',': 'Comma',
        '-': 'Minus',
        '/': 'Slash',
        '=': 'Equal',
        '[': 'BracketLeft',
        ']': 'BracketRight',
        '{': 'BracketLeft',
        '}': 'BracketRight',
      };
    
      if (code.length === 1) {
        if (code >= '0' && code <= '9') {
          code = 'Digit' + code;
        } else if (code >= 'A' && code <= 'z') {
          code = 'Key' + code.toUpperCase();
        } else if (keyToCodeMap.hasOwnProperty(code)) {
          code = keyToCodeMap[code];
        }
      }
    
      // For consistency with macOS, changing the handled event phase for
      // 'keyDownEvents' and 'keyUpEvents' to 'capturing' to prevent default
      // behavior.
      const CAPTURING_EVENT_PHASE = 1;
      return {...data, code, handledEventPhase: CAPTURING_EVENT_PHASE};
    }
    

    But I'm certain it's not a complete mapping, just maps the characters we care about in Messenger thus far 😬

  8. Saadnajmi commented on Jan 11, 2023

    @Saadnajmi
    ContributorAuthor

    Eric Rozell (@rozele) why does the event phase go to capturing for macOS consistency? I thought you would prevent defaults by virtue of having the event in your keyUp/DownEvents array?

  9. rozele commented on Jan 12, 2023

    @rozele
    Contributor

    Not sure, I assumed Capturing event phase was default as well. I think there was a time where setting the Bubbling phase did not prevent native behaviors in XAML Islands, but tried to repro recently and it was no longer the case.

    This is still open though:
    microsoft/microsoft-ui-xaml#4763

    🤷‍♂️

  10. Saadnajmi commented on Jan 12, 2023

    @Saadnajmi
    ContributorAuthor

    Eric Rozell (@rozele) In macOS natively, there is no capturing phase, only a bubbling phase. That's why I was confused.

    There's a way to capture events at the application level (https://developer.apple.com/documentation/appkit/nsevent/1534971-addlocalmonitorforeventsmatching?language=objc) so we could theoretically implement something close to a capturing phase.

  11. TatianaKapos commented on Dec 9, 2023

    @TatianaKapos
    Contributor

    Marking needs attention, this should either be assigned a dev for v74 or be put on the backlog!

  12. added and removed
    Needs: Attention 👋An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot)
    on Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Area: KeyboardBreaking ChangeThis PR will break existing apps and should be part of the known breaking changes for the releasePartner: FacebookWorkstream: Component ParityClose the parity gap between RNW and RN for core RN components and their supporting APIs.bug

    Type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions