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

Fix focus engine issue with list views on Apple TV #12944

Closed
wants to merge 3 commits into from

Conversation

douglowder
Copy link
Contributor

Motivation: Fix issue where Apple TV focus engine overscrolls when a list or scroll view is set to remove clipped subviews. See #12793 .

Test plan: Manual testing using modified source for UIExplorer's ListViewGridLayoutExample.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Mar 14, 2017
@douglowder douglowder changed the title Tvos listview fix 2 Fix focus engine issue with list views on Apple TV Mar 14, 2017
@shergin
Copy link
Contributor

shergin commented Mar 15, 2017

Thank you so much for pointing this out!
Therefore, I believe it should be fixed on native side because it is actually related to implementation details of very internal clipping feature. And, moreover, UIFocus​Environment feature family is available not only on tvOS actually, it is iOS 9.0+ and tvOS 9.0+, so turning clipping off for tvOS only does not always help.

I hope I will fix this soon.

@shergin shergin added the Platform: iOS iOS applications. label Mar 15, 2017
@douglowder
Copy link
Contributor Author

douglowder commented Mar 15, 2017

@shergin if you would prefer a native fix, I can have a look. I also realized that we probably need to make some doc changes so users know that removeClippedSubviews behaves differently on TV... http://facebook.github.io/react-native/docs/view.html#removeclippedsubviews

@shergin
Copy link
Contributor

shergin commented Mar 15, 2017

@dlowder-salesforce I will appreciate any help in researching this area, therefore:

  • I don't think that turning clipping off completely in some conditions is a good idea at all. It would be great if we can have more targeted and appropriate solution;
  • Even if right now it affects only tvOS (I have not tested yet), we have to support iOS as well. If not right now, but eventually iOS with attached keyboard will utilize this feature;
  • I would love to fix it with having accessibility issue ([iOS] VoiceOver focus does not automatically move to first unrendered element in ListView #12374) in mind, because it is essentially same (and, I have to admit, more important) problem.

@douglowder
Copy link
Contributor Author

@shergin yes it would be great if we could still do the clipping and not have this issue. Maybe there's some heuristic we can put in where we don't clip subviews that are near the edge of the viewable area, so that the bottom visible subview is not the bottommost view in the hierarchy, and this focus engine scrolling problem won't be triggered.

@douglowder
Copy link
Contributor Author

douglowder commented Mar 16, 2017

@shergin I tested out an idea where we create a buffer zone around the visible area of the view, and don't remove subviews that touch that zone. Setting it to 500 px seems to work well for the modified ListViewGridLayoutExample I've been testing with. See the most recent commit in https://github.com/dlowder-salesforce/react-native/tree/tvos-listview-native-fix . It seems to me that this might also resolve the related VoiceOver issue you mentioned, but I haven't tested that yet.

@shergin
Copy link
Contributor

shergin commented Mar 16, 2017

Yeah, I also thought about that. Probably increasing leeway here can help.
Therefore, personally, I am not satisfy with this solution because it is not universal and hacky. 😞

@douglowder
Copy link
Contributor Author

@shergin It's not clear to me what else we could do, other than increasing leeway or something similar -- I don't see any other general way of deciding which subviews to keep. Maybe the way to keep it non-hacky is to expose leeway as a view property, so that the application developer can decide how many hidden views to keep....

@douglowder
Copy link
Contributor Author

@shergin it does not appear to me that increasing leeway helps. I believe I have a reasonable fix that would make sense for both iOS and tvOS: extend the clipping rectangle bounds by one screen width/height in all four directions. See douglowder@df37fcc . I believe this should solve the VoiceOver issue as well.

@douglowder
Copy link
Contributor Author

@shergin I did some more testing of my native fix. It seems to work well as long as one doesn't navigate down the scrollview too fast. However, if you use the TV remote or arrow keys to go too fast, it seems like the code to add and remove and render subviews is having trouble keeping up, and occasionally the focus engine will get confused and pop the focus back up to the top. With removeClippedSubviews disabled, I don't seem to see any of those problems. Based on what I'm seeing right now, I believe that disabling removeClippedSubviews is the right solution for Apple TV.

@shergin
Copy link
Contributor

shergin commented Mar 23, 2017

@dlowder-salesforce Oh, that's interesting and super important observation! Thank you!!1
As a next step I would like to investigate/reevaluate the clipping mechanism completely. I have a filling that having this in RN is not worth it at all. If it is the case, we probably have to remove it completely. If my tests show promising results, I will share them with the community, and it can end up with removing this feature completely.

But, again, right now, it is just a feeling, no real proof. Give me some time.

@douglowder
Copy link
Contributor Author

Ok sounds good... since there is a workaround for the issue on Apple TV, I'll close this and wait for you to complete your work.

@douglowder douglowder closed this Mar 23, 2017
@douglowder douglowder deleted the tvos-listview-fix-2 branch March 28, 2017 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants