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

[Touches] Let only the nearest ancestor root view process touches #1372

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented May 22, 2015

This is to support nested root views with independent bridges by preventing touches in the inner root view from being handled by the outer one.

Previously, touches were handled by both touch handlers. This was problematic because the outer touch handler would dispatch a JS touch event to the outer bridge with a react tag from the inner root's descendant view. So if you touched view 7 in the inner root view, then view 7 in the outer root view would also receive a touch event. Instead, we want touches in the inner root view to stay in the inner root view.

Performance-wise, I profiled this diff and didn't see an impact. On the start of each touch (when the finger goes down, not when it is dragged), the view hierarchy is traversed upwards until the first root view which is on the order of 10 property lookups for a typical view hierarchy.

Test Plan: Tapped around in an inner root view and saw that components in the outer root view like text fields, etc were no longer receiving mystery touches.

@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 May 22, 2015
@sahrens
Copy link
Contributor

sahrens commented Jun 11, 2015

@tadeuzagallo, @nicklockwood: what do you think about this?

@nicklockwood
Copy link
Contributor

What's the use case for nested root views?

@nicklockwood
Copy link
Contributor

(Not that I need to know that in order to approve the PR - which seems reasonable to me).

@ide
Copy link
Contributor Author

ide commented Jun 12, 2015

This is for the Exponent component that renders a React Native bundle inside of another React Native app ^_^

@satya164
Copy link
Contributor

Any updates on this?

@brentvatne
Copy link
Collaborator

What should we do with this? @ide @nicklockwood

@satya164
Copy link
Contributor

ping!

@nicklockwood
Copy link
Contributor

cc: @majak, who has a better understanding of touch handling than I do.

@majak majak assigned majak and unassigned tadeuzagallo Jan 27, 2016
@majak
Copy link
Contributor

majak commented Jan 27, 2016

I believe this works.
I see this as a subset of a larger problem, which is embedding RN root view in any other view, where the outer view can have its own gesture recognizers. Imagine RN rootviews in a native UIScrollView. When you touch down on the embedded RN and drag, the RN should get its touches cancelled.

So I'm thinking about a bit more generic solution, which would expose RCTTouchHandler as a property on the rootView, so anyone could set up dependency between RN and native GR as needed.
(There is some risk in this approach, since it requires a change in what states our RCTTouchHandler GR goes through, and I'm not entirely sure if there are any hidden implication of doing that yet.)

I'm hesitant to merge this fix. The main reason being is it's not unreasonable to think you could want to do the opposite thing - make the embedded RN not receive any touches, and we already internally have have the UIScrollView embed need.

@ide let me know what are your thoughts.

@ide
Copy link
Contributor Author

ide commented Jan 28, 2016

@majak thanks for your analysis. I agree that this diff is too inflexible and it would be much better if the RN RootView provided a way for its superview to customize how touches within the RN RootView are handled.

In our use case, we want to let a touch inside of a RootView get sent to the deepest subview that was directly touched (like the normal touch-handling behavior), and allow the touch to bubble up the view hierarchy.

The superview of the RootView wants to intercept and create a copy of the touch with some properties changed, and let the new touch bubble up. This is a little diagram of what we're trying to do:

+-----------------------------------------------------------+
|                    RCTRootView 1                          |
|                    managed by RCTBridge 1                 |
|  +----+-----------------------------------------------+   |
|  |    ^                                               |   |
|  |    |       Subview managed by RCTBridge 1          |   |
|  |    |                                               |   |
|  |    +---[TouchHandler receives UITouch A.     ]     |   |
|  |        [It stops propagation of UITouch A and]     |   |
|  |        [creates a new touch called UITouch B.]     |   |
|  |        [UITouch B is allowed to bubble up the]     |   |
|  |        [view hierarchy.                      ]     |   |
|  |                 ^                                  |   |
|  |            +-------------------------------------+ |   |
|  |            |    ^                                | |   |
|  |            |    |     RCTRootView 2              | |   |
|  |            |    |     managed by RCTBridge 2     | |   |
|  |            |    |                                | |   |
|  |            |    |                                | |   |
|  |            |    |                                | |   |
|  |            |    +----+[UITouch A arrives.  ]     | |   |
|  |            |          [The touch is handled]     | |   |
|  |            |          [as normal.          ]     | |   |
|  |            +-------------------------------------+ |   |
|  +----------------------------------------------------+   |
+-----------------------------------------------------------+

So an API that lets us write code that sits between the UIKit touch & gesture recognizer APIs and RN's touch handler would probably work for us.

@majak
Copy link
Contributor

majak commented Feb 1, 2016

This is interesting. Just to double check, you want the inner root view to handle the touch, but at the same time the outer one should get some transformed touch as well? (No matter if the inner root view actually did anything with the touch.)

@ide
Copy link
Contributor Author

ide commented Feb 1, 2016

@majak Yes, exactly.

@majak
Copy link
Contributor

majak commented Feb 2, 2016

@ide This diff would be a part of the solution for your case?
Also I'm curious what the transformation would look like if you can say.

@ide
Copy link
Contributor Author

ide commented Feb 2, 2016

I think this diff would not be part of the solution since we want the touch to bubble up past the inner root view, but allow its superview to intercept the touch.

The transformation of the UITouch would be just to change its view property. In our case we want the new value of touch.view to be the superview of the inner root view.

@facebook-github-bot
Copy link
Contributor

@ide updated the pull request.

@majak
Copy link
Contributor

majak commented Feb 5, 2016

So currently even the outer root view get touches with view property set to a view from the inner root view? (If yes) That sounds wrong, since the embedded one should be treated like an opaque view by the outer one.

My guess is this is happening because of this:

// Find closest React-managed touchable view
UIView *targetView = touch.view;
while (targetView) {
if (targetView.reactTag && targetView.userInteractionEnabled &&
[targetView reactRespondsToTouch:touch]) {
break;
}
targetView = targetView.superview;
}

I think you could try modify that logic to do continue climbing up the view hierarchy up even in case we found a react view, but its root view is not the same as the one the GR was set up on.

@ghost
Copy link

ghost commented Feb 29, 2016

@tadeuzagallo would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@mkonicek
Copy link
Contributor

@ide Do you plan to continue working on this? Any opinions on the comment by majak?

@ide
Copy link
Contributor Author

ide commented Mar 3, 2016

@mkonicek yep, I plan to look into this more eventually. Probably not super soon but I would like to build a solution!

@mkonicek
Copy link
Contributor

mkonicek commented Mar 4, 2016

OK sounds good :)

@mkonicek
Copy link
Contributor

Just going through all old PRs. Based on the comments above leaving this one open.

This is to support nested root views with independent bridges by preventing touches in the inner root view from being handled by the outer one.

Previously, touches were handled by both touch handlers. This was problematic because the outer touch handler would dispatch a JS touch event to the outer bridge with a react tag from the inner root's descendant view. So if you touched view 7 in the inner root view, then view 7 in the outer root view would also receive a touch event. Instead, we want touches in the inner root view to stay in the inner root view.

Performance-wise, I profiled this diff and didn't see an impact. On the start of each touch (when the finger goes down, not when it is dragged), the view hierarchy is traversed upwards until the first root view which is on the order of 10 property lookups for a typical view hierarchy.

Test Plan: Tapped around in an inner root view and saw that components in the outer root view like text fields, etc were no longer receiving mystery touches.
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2016
@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 31, 2016
@lacker
Copy link
Contributor

lacker commented Oct 25, 2016

I support the great cause of this pull request, but at this point it is kind of just a symbol of intending to do something about this that has just been a symbol for months. I think we should close it. If you want to start working on it again then that would be awesome! - and then I think just opening a new PR would be appropriate.

@lacker lacker closed this Oct 25, 2016
ayushjainrksh pushed a commit to MLH-Fellowship/react-native that referenced this pull request Jul 2, 2020
* ignore version 0.11

* lint
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants