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

Deprecate onScrollKeyDown, refactor Flatlist selection logic #1365

Merged
merged 6 commits into from
Aug 25, 2022

Conversation

Saadnajmi
Copy link
Collaborator

@Saadnajmi Saadnajmi commented Aug 20, 2022

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

Closes #501

With #1258 , #1267, and (for posterity) #1270 , we "revived" the Flatlist prop enableSelectionOnKeyPress. This would add some custom keyboard handling to Flatlist allowing you to move through the list with arrow keys and keyboard shortcuts, and "select" a row. The prop was meant to be opt-in, meaning if it was off, then Flatlist would do no keyboard handling and let all keyboard events bubble as usual. We did this by setting a prop on the Flatlists' inner RCTScrollView: onScrollKeyDown. When that is set, RCTScrollView will handle some keyboard events (ArrowUp, ArrowDown, PageUp, PageDown, Home, End) instead of letting them bubble up through the native hierarchy. One thing to note: onScrollKeyDown has been marked for deprecation for a while now, since it doesn't conform to the agreed-upon RN keyboarding API that the desktop platforms set.

As it turns out, enableSelectionOnKeyPress wasn't truly opt-in. As it was written, RCTScrollView would always block those keyboard events natively, regardless of whether the prop was set or not. This caused some bugs where come keyboard events stopped getting handled for our partner teams.

How do we fix this? I chose to scrap the custom implementation of onScrollKeyDown altogether, and just implement that standard onKeyDown / validKeysDown API that React Native macOS has on RCTView. This is aided by the refactor done in #1338 , which made it much easier to share the keyboarding API :) With this new API, we can conditionally pass validKeysDown based on whether enableSelectionOnKeyPress is set, and make the prop truly opt in.

Changelog

[macOS] [Fixed] - Deprecate onScrollKeyDown

Test Plan

I have an (ugly..) example a View inside each FlatList row that captures ArrowUp and ArrowDown. With and without the enableSelectionOnKeyPress prop set, the view can still capture the arrow keys. I don't really want to commit this example, because it's ugly...

Screen.Recording.2022-08-24.at.1.14.09.AM.mov

@Saadnajmi Saadnajmi marked this pull request as ready for review August 23, 2022 20:45
@Saadnajmi Saadnajmi requested a review from a team as a code owner August 23, 2022 20:45
@Saadnajmi Saadnajmi force-pushed the deprecate-scrollkeydown branch 3 times, most recently from 065c195 to 51956aa Compare August 24, 2022 08:23
remove pressable diff

Remove JS handling for PageUp/Down, fix flow errors

Add back "autoscroll to focused view" behavior

remove commented code

remove change to pressable

Update documentation

fix flow error

fix lint issue

Fix 'selectRowAtIndex'

More simplification

lock
@Saadnajmi Saadnajmi changed the title Deprecate onScrollKeyDown Deprecate onScrollKeyDown, refactor Flatlist selection logic Aug 24, 2022
@Saadnajmi
Copy link
Collaborator Author

One more request from the feature team: they would like to not have the first row selected by default. I'll add an "initialSelectedIndex' prop like the existing 'initialScrollIndex' and let it be set to undefined to accomodate.

@Saadnajmi Saadnajmi merged commit 8a4ade2 into microsoft:main Aug 25, 2022
@Saadnajmi Saadnajmi deleted the deprecate-scrollkeydown branch August 25, 2022 04:53
Saadnajmi added a commit to Saadnajmi/react-native-macos that referenced this pull request Aug 25, 2022
…ft#1365)

* Deprecate onScrollKeyDown

remove pressable diff

Remove JS handling for PageUp/Down, fix flow errors

Add back "autoscroll to focused view" behavior

remove commented code

remove change to pressable

Update documentation

fix flow error

fix lint issue

Fix 'selectRowAtIndex'

More simplification

lock

* Make method public again

* Add initialSelectedIndex

* macOS tags

* fix lint
Saadnajmi added a commit to Saadnajmi/react-native-macos that referenced this pull request Aug 25, 2022
…ft#1365)

* Deprecate onScrollKeyDown

remove pressable diff

Remove JS handling for PageUp/Down, fix flow errors

Add back "autoscroll to focused view" behavior

remove commented code

remove change to pressable

Update documentation

fix flow error

fix lint issue

Fix 'selectRowAtIndex'

More simplification

lock

* Make method public again

* Add initialSelectedIndex

* macOS tags

* fix lint
Saadnajmi added a commit that referenced this pull request Aug 25, 2022
…1374)

* Refactor handling of keyDown/keyUp (#1338)

This refactors / simplifies certain keyUp|Down event handling.
It will make a later change (adding textInput handling for textInput fields) easier (to review)

Co-authored-by: Scott Kyle <skyle@fb.com>

* Deprecate onScrollKeyDown, refactor Flatlist selection logic (#1365)

* Deprecate onScrollKeyDown

remove pressable diff

Remove JS handling for PageUp/Down, fix flow errors

Add back "autoscroll to focused view" behavior

remove commented code

remove change to pressable

Update documentation

fix flow error

fix lint issue

Fix 'selectRowAtIndex'

More simplification

lock

* Make method public again

* Add initialSelectedIndex

* macOS tags

* fix lint

* RNTester: only show the Flatlist keyboard navigation switch on macOS

Co-authored-by: Christoph Purrer <christophpurrer@outlook.com>
Co-authored-by: Scott Kyle <skyle@fb.com>
Saadnajmi added a commit that referenced this pull request Aug 26, 2022
…1375)

* Revert "Revert Flatlist changes (#1306)"

This reverts commit 8ce9545.

* Refactor handling of keyDown/keyUp (#1338)

This refactors / simplifies certain keyUp|Down event handling.
It will make a later change (adding textInput handling for textInput fields) easier (to review)

Co-authored-by: Scott Kyle <skyle@fb.com>

* Deprecate onScrollKeyDown, refactor Flatlist selection logic (#1365)

* Deprecate onScrollKeyDown

remove pressable diff

Remove JS handling for PageUp/Down, fix flow errors

Add back "autoscroll to focused view" behavior

remove commented code

remove change to pressable

Update documentation

fix flow error

fix lint issue

Fix 'selectRowAtIndex'

More simplification

lock

* Make method public again

* Add initialSelectedIndex

* macOS tags

* fix lint

* remove one more reference

* yarn lint --fix

* RNTester: only show the Flatlist keyboard navigation switch on macOS

Co-authored-by: Christoph Purrer <christophpurrer@outlook.com>
Co-authored-by: Scott Kyle <skyle@fb.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
…ft#1365)

* Deprecate onScrollKeyDown

remove pressable diff

Remove JS handling for PageUp/Down, fix flow errors

Add back "autoscroll to focused view" behavior

remove commented code

remove change to pressable

Update documentation

fix flow error

fix lint issue

Fix 'selectRowAtIndex'

More simplification

lock

* Make method public again

* Add initialSelectedIndex

* macOS tags

* fix lint
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.

Deprecate onScrollKeyDown
2 participants