use zPosition in RCTViewHitTest to properly route touches #875
Conversation
facebook-github-bot
added
the
CLA Signed
|
1st impression 👍 👍 👍 2nd, any measurable impact with the 2nd loop? |
|
Haven't done any performance tests, not sure how expensive hit tests are but this could potentially add a fair number of them. Reading up a little more on iOS hit testing it looks like by default view layer zposition is not taken into account (haven't seen too many references to this, but this book claims so). Not sure if that is something we want to honor. If it is, or performance is an issue an alternative solution could be implementing a custom hitTest for RCTScrollView and only check for zposition there. Maybe that would be better since I haven't actually seen any other instances of zposition being used. |
|
My previous code was not properly dealing with children of zPositioned views, changed it so zIndex is compared at the sibling level. This allows nesting buttons / touchable elements in sticky section headers on listViews. |
|
@nicklockwood - I have run into this issue on my own as well - it's common for sticky headers to include touchable components, eg: for navigation, and this isn't possible currently. I tested this fix and it worked well for me. cc @tadeuzagallo @a2 |
|
Sorry this has been in limbo for a while. The issue we have with the fix is that it's very expensive to do the search like this. Because the loop is no longer exiting early, it basically increases the algorithm from O(log n) to O(n), which is troubling. A possible alternative would be to pre-sort subviews by z-index, but since the JS-side of React isn't aware of the z-index property, it's not entirely clear how best to do that. |
|
@nicklockwood - Thanks for the response! That does indeed sound troubling - what about the option of implementing a custom hitTest for RCTScrollView? It might be a good stop-gap solution until we can come up with a more comprehensive one. |
|
@brentvatne that sounds like a solid approach, especially since that's currently the only place we use z-index. |
|
@badfortrains - are you up for modifying this PR based on the above conversation or would you like me to take care of it? |
|
It should be pretty straightforward - just loop through the sticky header indexed views first and if none of them return a hit, call [super hitTest]. There are certainly some optimisations that could be made, such as extracting some of the dockClosestSectionHeader logic so that you only need to check the docked headers (the others all have the same z-index as the normal views, so don't need special treatment), but I reckon that's a reasonable starting point. |
|
Closing in favour of new PR, thanks! |
|
Thanks @brentvatne for following up on this, the new pr looks like a good solution. |
Summary: As per discussion with @nicklockwood in facebook#875, make `RCTScrollView` check its sticky headers for hitTests first. Closes facebook#1415 Github Author: Brent Vatne <brent.vatne@madriska.com> Test Plan: Have a sticky header in a ScrollView with a Touchable onPress action, scroll a bit after it docks and try tapping, should respond to tap.
Summary: As per discussion with @nicklockwood in facebook#875, make `RCTScrollView` check its sticky headers for hitTests first. Closes facebook#1415 Github Author: Brent Vatne <brent.vatne@madriska.com> Test Plan: Have a sticky header in a ScrollView with a Touchable onPress action, scroll a bit after it docks and try tapping, should respond to tap.
Fixes #696. Previously the hit test would return the first suitable, fully nested, view it found. Changed the test to store all hits and chose the one with the greatest zPosition.
The change can be seen in the UIExplorer - Paging example. Previously presses would fall through the sticky section headers, now the headers correctly catch the presses and the row animation underneath is not triggered.