Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS Keyboard Support #657

Merged
merged 39 commits into from Dec 12, 2020
Merged

Conversation

HeyImChris
Copy link

@HeyImChris HeyImChris commented Nov 26, 2020

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

This adds macOS keyboarding support. This was also done in a way that it can be expanded to iOS hardware keyboards with a few minor additions in some followup work.

Changelog

[macOS][Added]- macOS keyboard support

Test Plan

In the RNTester I can tab focus over to Views and Buttons and print the key being pressed for both up and down presses.

Microsoft Reviewers: Open in CodeFlow

React/Views/RCTViewKeyboardEvent.m Show resolved Hide resolved
Comment on lines 1570 to 1579
- (void)keyDown:(NSEvent *)event {
if (!self.onKeyDown) {
[super keyDown:event];
return;
}

RCTViewKeyboardEvent *keyboardEvent = [RCTViewKeyboardEvent keyDownEventWithReactTag:self.reactTag characters:event.characters modifier:event.modifierFlags];
[_eventDispatcher sendEvent:keyboardEvent];
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common pattern with keyDown:/keyUp: events is for someone to override keyDown: check the type or keys pressed or other info on the event and optionally handle it, else pass the event up the responder chain by calling super. In this react implementation it looks like the presence of an onKeyDown is what determines whether the event gets handled, so how would someone implement a "conditional" key down handler, for example one that responds to arrow keys to move an object but lets other key events pass through to the responder chain to see if someone else handles it?

It looks like the current implementation would just drop all those events on the floor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm good point. We could let the modules initialize this with the specific keys they want passed up and then not post events for all the other keys

Copy link
Member

@alloy alloy Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In [React] JS this would typically be controlled by conditionally invoking Event.prototype.stopPropagation() or Event.prototype.preventDefault(), see e.g. https://reactjs.org/docs/events.html. I assume there’s precedent for this in the RN codebase, but haven’t had to implement this myself yet.

The only concern I’d have is that we can't get a synchronous response and, assuming that asynchronously responding to this event is ok, what the possible lag implications are when somebody's typing. For that reason your idea of upfront describing which keys a listener handles might make sense, but it would be a deviation from typical behaviour.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickGerleman I assume this is a similar concern on Windows, no? For reference, the doc you linked to mentions:

The properties in NativeSyntheticEvent like target, bubbles, cancelable etc., are also available for IKeyboardEvent and follow the same behaviors as other events in react-native today.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @acoates-ms or @FalseLobster have context into this for Windows.

Copy link
Collaborator

@Saadnajmi Saadnajmi Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: I had originally come back to this thread 1 year later to say that the validKeys(Up|Down) prop is non-standard and not well documented, and a rough proposal for how to not need it by doing all the event handling in JS. Then I discovered that the original Keyboarding proposal doc has a section about this exact issue, with a similar prop key(Up|Down)Events. I think it would be best if we could align with this prop / event model: It would certainly make my job writing components with custom keyboard events in FuentUI React Native easier (That's how I came back to this PR anyway).
Another helpful PR around this prop for reference: microsoft/react-native-windows#8077 .

@@ -445,6 +452,21 @@ export default class Pressability {
}
},
};

const keyEventHandlers = {
onKeyDown: (event: KeyEvent): void => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For extra context, react-native-windows did this a slightly different way. There, we translate key events seen by Pressability into press events, instead of having Pressability emit key events. This matches some legacy behavior, but it also means press event shape is unexpected when press happens by keyboard activation.

If you want raw pass through, it might be simpler to just avoid passing the handlers from Pressable/Touchable to Pressability.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we get state change visuals on keyboard with this? IIRC hooking into the Pressability state machine gave us those for free.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mimics the pattern we used for onFocus and onBlur for macOS. What do you mean by state change visuals on keyboard?

Copy link
Collaborator

@Saadnajmi Saadnajmi Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand properly, windows treats onKeyUp as onPressIn, and onKeyDown as onPressOut. Then, any visuals that might be associated with the pressed state of a perusable also happen on key press. This sounds like a desirable behavior that I think we'd want to incorporate...
I just checked and it looks like react-native-windows is still doing the same thing, so I guess the flow problems were worth the trade off?

React/Views/RCTView.m Outdated Show resolved Hide resolved
React/Views/RCTView.m Outdated Show resolved Hide resolved
Comment on lines 1570 to 1579
- (void)keyDown:(NSEvent *)event {
if (!self.onKeyDown) {
[super keyDown:event];
return;
}

RCTViewKeyboardEvent *keyboardEvent = [RCTViewKeyboardEvent keyDownEventWithReactTag:self.reactTag characters:event.characters modifier:event.modifierFlags];
[_eventDispatcher sendEvent:keyboardEvent];
}

Copy link
Member

@alloy alloy Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In [React] JS this would typically be controlled by conditionally invoking Event.prototype.stopPropagation() or Event.prototype.preventDefault(), see e.g. https://reactjs.org/docs/events.html. I assume there’s precedent for this in the RN codebase, but haven’t had to implement this myself yet.

The only concern I’d have is that we can't get a synchronous response and, assuming that asynchronously responding to this event is ok, what the possible lag implications are when somebody's typing. For that reason your idea of upfront describing which keys a listener handles might make sense, but it would be a deviation from typical behaviour.

React/Views/RCTViewKeyboardEvent.h Outdated Show resolved Hide resolved
React/Views/RCTViewKeyboardEvent.h Outdated Show resolved Hide resolved
React/Views/RCTViewKeyboardEvent.m Outdated Show resolved Hide resolved
React/Views/RCTViewKeyboardEvent.m Outdated Show resolved Hide resolved
React/Views/RCTViewManager.m Show resolved Hide resolved
@HeyImChris
Copy link
Author

Checking in as the only failure is a known Android CI failure unrelated to this

@HeyImChris HeyImChris merged commit c0ec10d into microsoft:master Dec 12, 2020
harrieshin pushed a commit that referenced this pull request Dec 16, 2020
* Update RCTCxxBridge.mm

* macOS keyboard support

* update comment

* add key value checks so we don't send all events

* macOS keyboard support

* update comment

* add key value checks so we don't send all events

* [ado] Workaround homebrew openssl issue (#669)

actions/runner-images#1811 (comment)

* Fix detox yarn error with Xcode 12 (#670)

* Update RCTCxxBridge.mm

* fix detox errors with xcode 12

* only use valid keys, bubble events to super

* macOS keyboard support

* update comment

* add key value checks so we don't send all events

* only use valid keys, bubble events to super

* macOS keyboard support

* add key value checks so we don't send all events

* resolve bad merge

* update valid key bug, api typo

* spacing fix

* fix flow errors

* fix snapshot tests for new APIs

* yarn lint --fix

* fix flipper

Co-authored-by: Eloy Durán <eloy.de.enige@gmail.com>
amgleitman pushed a commit to amgleitman/react-native-macos that referenced this pull request Dec 17, 2020
harrieshin pushed a commit that referenced this pull request Dec 17, 2020
* cherry pick c0ec10d: macOS Keyboard Support (#657)

* Bring back explicit null checks in TouchableOpacity.js

* Make yarn flow-check-ios happy

Co-authored-by: HeyImChris <48299693+HeyImChris@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants