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

Add explicit Tab support to keyboarding #723

Merged
merged 16 commits into from
Feb 26, 2021

Conversation

HeyImChris
Copy link

@HeyImChris HeyImChris commented Feb 12, 2021

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

Tab is an interesting character to receive a keyboard event for. Tab alone provides an OS event of character @" " (the width of a tab). However, when paired with the shift modifier key, the Tab event character becomes an empty string modified by the shift bar.

This is likely due to the multiple purposes of the tab key given that tab is used by the OS to traverse between many accessible views.

The easiest fix here is to slot the Tab key along with Escape as a special character that we need to explicitly check the key code for. This also aligns with the W3 standard section 3.3 Whitespace.

After this gets into master I'll make PR's checking it into the 0.62-stable and 0.63-stable branches.

Changelog

[macOS] [Bug] - Tab keyboard support

Test Plan

Added tab to the keyboard events test app. Both tab and shift tab can now propagate to our JS when listening for tab events.

Microsoft Reviewers: Open in CodeFlow

@anandrajeswaran
Copy link

Can/should usage of this be added to RNTester - both as a test bed for future changes and an illustrative guide on how/when clients should consider listening for tab keys? I am kinda curious on the scenario that needs this and how a client would be using the event.

Probably related, I'm curious what you mean by "multiple purposes of the tab key" in the description. I had thought tab was a pretty consistent and opinionated key on macOS so I'm curious why someone would be overriding its behavior.

@HeyImChris
Copy link
Author

Can/should usage of this be added to RNTester - both as a test bed for future changes and an illustrative guide on how/when clients should consider listening for tab keys? I am kinda curious on the scenario that needs this and how a client would be using the event.

Probably related, I'm curious what you mean by "multiple purposes of the tab key" in the description. I had thought tab was a pretty consistent and opinionated key on macOS so I'm curious why someone would be overriding its behavior.

Tab key has the add whitespace behavior and the toggle around views behavior which might be why the event char associated with tab is different from that of shift+tab as tab alone has a character representation but shift+tab doesn't.

An internal scenario for this is in Excel's CardView on macOS. I'm waiting to hear back specifics but believe it'll be used in their expanded matrix view for navigation.

@anandrajeswaran
Copy link

Can/should usage of this be added to RNTester - both as a test bed for future changes and an illustrative guide on how/when clients should consider listening for tab keys? I am kinda curious on the scenario that needs this and how a client would be using the event.
Probably related, I'm curious what you mean by "multiple purposes of the tab key" in the description. I had thought tab was a pretty consistent and opinionated key on macOS so I'm curious why someone would be overriding its behavior.

Tab key has the add whitespace behavior and the toggle around views behavior which might be why the event char associated with tab is different from that of shift+tab as tab alone has a character representation but shift+tab doesn't.

An internal scenario for this is in Excel's CardView on macOS. I'm waiting to hear back specifics but believe it'll be used in their expanded matrix view for navigation.

The tough part is that doesn't seem like a realistic distinction for me. In react-native I don't think we're ever going to want to recreate NSTextView (which has the special behavior), so other than that it should just follow the key view loop. If someone is using an rn control that translates to an NSTextView it should just work. So the question still is what customization we want to do and whether that is a good thing to do on the platform. Adding the scenario to the test app is a good way to illustrate that

@HeyImChris
Copy link
Author

Can/should usage of this be added to RNTester - both as a test bed for future changes and an illustrative guide on how/when clients should consider listening for tab keys? I am kinda curious on the scenario that needs this and how a client would be using the event.
Probably related, I'm curious what you mean by "multiple purposes of the tab key" in the description. I had thought tab was a pretty consistent and opinionated key on macOS so I'm curious why someone would be overriding its behavior.

Tab key has the add whitespace behavior and the toggle around views behavior which might be why the event char associated with tab is different from that of shift+tab as tab alone has a character representation but shift+tab doesn't.

An internal scenario for this is in Excel's CardView on macOS. I'm waiting to hear back specifics but believe it'll be used in their expanded matrix view for navigation.

Okay so alternative solution here... we scrap this review and add NSView props to RCTView for key-view loop management (e.g. canBecomeKeyView, nextKeyView, previousKeyView, etc.) and clients can use the system defined tab behavior to navigate around controls without actually overriding what the system tab behavior does

@anandrajeswaran
Copy link

Can/should usage of this be added to RNTester - both as a test bed for future changes and an illustrative guide on how/when clients should consider listening for tab keys? I am kinda curious on the scenario that needs this and how a client would be using the event.
Probably related, I'm curious what you mean by "multiple purposes of the tab key" in the description. I had thought tab was a pretty consistent and opinionated key on macOS so I'm curious why someone would be overriding its behavior.

Tab key has the add whitespace behavior and the toggle around views behavior which might be why the event char associated with tab is different from that of shift+tab as tab alone has a character representation but shift+tab doesn't.
An internal scenario for this is in Excel's CardView on macOS. I'm waiting to hear back specifics but believe it'll be used in their expanded matrix view for navigation.

Okay so alternative solution here... we scrap this review and add NSView props to RCTView for key-view loop management (e.g. canBecomeKeyView, nextKeyView, previousKeyView, etc.) and clients can use the system defined tab behavior to navigate around controls without actually overriding what the system tab behavior does

To me that seems like it would make it easier for clients as it would decrease the amount they would have to recreate, but I still think seeing an example of recommmended usage in RNTester would help make that clear

@HeyImChris
Copy link
Author

Can/should usage of this be added to RNTester - both as a test bed for future changes and an illustrative guide on how/when clients should consider listening for tab keys? I am kinda curious on the scenario that needs this and how a client would be using the event.
Probably related, I'm curious what you mean by "multiple purposes of the tab key" in the description. I had thought tab was a pretty consistent and opinionated key on macOS so I'm curious why someone would be overriding its behavior.

Tab key has the add whitespace behavior and the toggle around views behavior which might be why the event char associated with tab is different from that of shift+tab as tab alone has a character representation but shift+tab doesn't.
An internal scenario for this is in Excel's CardView on macOS. I'm waiting to hear back specifics but believe it'll be used in their expanded matrix view for navigation.

Okay so alternative solution here... we scrap this review and add NSView props to RCTView for key-view loop management (e.g. canBecomeKeyView, nextKeyView, previousKeyView, etc.) and clients can use the system defined tab behavior to navigate around controls without actually overriding what the system tab behavior does

To me that seems like it would make it easier for clients as it would decrease the amount they would have to recreate, but I still think seeing an example of recommended usage in RNTester would help make that clear

Cool yea I like that plan more too. When I add key-view loop support I can add a new test page for it

@HeyImChris
Copy link
Author

Closing this PR to make a new one that instead of just exposing tab, will allow clients to set props like nextKeyView and previousKeyView on their views and just use the default tabbing as the OS allows

@HeyImChris HeyImChris closed this Feb 25, 2021
@HeyImChris
Copy link
Author

There have been some valid client scenarios brought up since closing this that there is still value in tab support. I'll add this but then still add key-view looping support as well.

@HeyImChris HeyImChris reopened this Feb 26, 2021
@HeyImChris HeyImChris merged commit 39fdd68 into microsoft:master Feb 26, 2021
HeyImChris added a commit to HeyImChris/react-native-macos that referenced this pull request Mar 2, 2021
* Update RCTCxxBridge.mm

* Update RCTCxxBridge.mm

* add explicit tab support

* missing tab validity check
HeyImChris added a commit that referenced this pull request Mar 17, 2021
* Update RCTCxxBridge.mm

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* Update RCTCxxBridge.mm

* add quotes

* switch to catalina

* update pods to 10.15

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* add quotes

* switch to catalina

* update pods to 10.15

* upgrade version requirements to n-2 for osx

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* add quotes

* switch to catalina

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* add quotes

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* add quotes

* CI should run on PRs to future stable branches (#718)

* Pull in header's C declaration to stop symbol mangling  (#728)

* Update RCTCxxBridge.mm

* Update RCTCxxBridge.mm

* fix undefined extern c symbol

* scroll content with animation (#725)

Summary: please see test plan

Test Plan:
|Before|After|
|{F371503806}|{F371499708}|



Reviewers: skyle

Reviewed By: skyle

Subscribers: zackargyle

Differential Revision: https://phabricator.intern.facebook.com/D26354172

Tasks: T84165504

Signature: 26354172:1612920735:2cd8455b1bae06ee555bd98cfd41c4dfb29c288e

Co-authored-by: Mo Wang <mowang@fb.com>

* Fix missing props on secure text input (#719)

* Enable animated gif playback on Mac (#724)

* enable gif for mac

Summary:
I verify that it works for Zeratul as well. 

It also works regardless turbo module is enabled or not.

Test Plan: 
|{F369886201}|{F369889196}|



Reviewers: skyle

Reviewed By: skyle

Subscribers: zackargyle

Differential Revision: https://phabricator.intern.facebook.com/D26272145

Tasks: T82742678

Signature: 26272145:1612513052:520a8b0b3ab4b211c953b3225c6c96ffb8a29fc5

* bypass setImage logic when it is Mac

Summary: as titled

Test Plan:

{F370138978}

{F370139033}


Reviewers: skyle

Reviewed By: skyle

Subscribers: zackargyle

Differential Revision: https://phabricator.intern.facebook.com/D26288169

Signature: 26288169:1612565769:8f779fe01614e3399ac3e484853bb61246210ff4

* address PR comments

* address PR comments 1

Co-authored-by: Mo Wang <mowang@fb.com>

* Add explicit Tab support to keyboarding (#723)

* Update RCTCxxBridge.mm

* Update RCTCxxBridge.mm

* add explicit tab support

* missing tab validity check

* Fix Deadlock in RCTi18nUtil (iOS) (#733)

* Initial Commit

* Fix typo

* Fix default initialization of isRTLAllowed and documentation

* use BOOL, remove superfluous readwrite decorator

* Fix another typo

* Add TODO Marker

* update to macOS 10.15

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* add quotes

* switch to catalina

* update pods to 10.15

* upgrade version requirements to n-2 for osx

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* add quotes

* switch to catalina

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* add quotes

* make CI xcode versioning more robust to xcode12

* update to xcode 12.4

* use apple variables file

* add quotes

* update to macOS 10.15

* update to xcode 12.4

* use apple variables file

* switch to catalina

* 10.14

* CI has to run on 10.15.4 or later

* fix up pod errors

* upgrade iphone simulator

* iphone 13

* remove old post install script

* iphone 8

* Restore Podfile.lock to see if it fixes CI

* Restore compiler flags in TurboModuleCxx-WinRTPort

* Remove macOS arm64 build divergence

* Remove building all ARCHS temporarily

* Override ONLY_ACTIVE_ARCH in release builds too

* Push iOS 14 snapshots

Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>
Co-authored-by: Mo <mo.hy.wang@gmail.com>
Co-authored-by: Mo Wang <mowang@fb.com>
Co-authored-by: Scott Kyle <scott@appden.com>
Co-authored-by: Saad Najmi <saadnajmi2@gmail.com>
Co-authored-by: Anand Rajeswaran <anand.rajeswaran@outlook.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.

3 participants