Scrolling fixes

[ontzic]

Summary

Scrolling improvements in ReactAndroid:

Issue: With current ReactHorizontalScrollView behavior, it treats all views as focusable, regardless of if they are in view or not. This is fine for non-paged horizontal scroll view, but when paged this allows focus on elements that are not within the current page. Combined with logic to scroll to the focused view, this breaks the paging for ReactHorizontalScrollView.

Fix: limit the focusable elements to only elements that are currently in view when ReactHorizontalScrollView has paging enabled

Issue: When keyboard is attached and user tries to navigate through Tab key, Scroll views do not scroll to the focused child.
Since ReactScrollView handles layout changes on JS side, it does not call super.onlayout due to which mIsLayoutDirty flag in android ScrollView remains true and prevents scrolling to child when requestChildFocus is called.

Fix: To fix the focus navigation, we are overriding requestChildFocus method in ReactScrollView. We are not checking any dirty layout flag and scrolling to child directly. This will fix focus navigation issue for KeyEvents which are not handled by android's ScrollView, for example: KEYCODE_TAB. Same applies to ReactHorizontalScrollView.

Set Android ScrollView to be non-focusable when scroll is disabled. Prior to this change, non-scrollable Scrollview would still be focusable, causing a poor keyboarding experience

Changelog

[Android] [Fixed] Scrolling improvements in ReactAndroid

Test Plan

Changes tested on Nexus 7 (Android 6.0.1, API 23), with a Bluetooth keyboard attached.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[sahrens]

Awesome, thanks for improving accessibility in RN!

Is there a github issue this is associated with? Would be nice to include that in the summary.

Can you expand the test plan a bit? How could this potentially break existing behavior, and how did you verify that those cases still work as expected?

Someone with more Android expertise should probably take a look too - @mdvacca or @janicduplessis?

[sahrens]

Also, have you looked into ReactViewPager.java? I think that has similar issues and is commonly used for critical flows like new user experiences, onboarding wizards, etc.

[ontzic]

Also, have you looked into ReactViewPager.java? I think that has similar issues and is commonly used for critical flows like new user experiences, onboarding wizards, etc.

We don't have any contributions yet to ReactViewPager, but I will include our Android experts and maybe we could consider it for future improvements.

[ontzic]

Awesome, thanks for improving accessibility in RN!

Is there a github issue this is associated with? Would be nice to include that in the summary.

Can you expand the test plan a bit? How could this potentially break existing behavior, and how did you verify that those cases still work as expected?

Someone with more Android expertise should probably take a look too - @mdvacca or @janicduplessis?

Thanks a lot! These changes have existed and been used in our internal branch for a while, this being the first PR from a series of PRs we are planning for the near future, for bringing all the Android improvements we have, into RN.

For this particular PR, we did a sanity check on a physical device with with a Bluetooth keyboard attached, in RN Tester ScrollView, by using Tab and arrow keys to move the focus through the items of the list. One of the things that this PR is changing/improving is that using Tab will make the view scroll to the focused element, which will naturally scroll the view while going up and down the list (or left + right).

I will include shortly our Android experts, for more comments if needed.

[mdvacca]

LGTM

[facebook-github-bot]

@mdvacca is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @ontzic in ae231c8.

^{When will my fix make it into a release? | Upcoming Releases}