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

[Android] Convert event to local coordinates inside the target view. #3426

Closed
wants to merge 1 commit into from
Closed

Conversation

cypheon
Copy link

@cypheon cypheon commented Oct 14, 2015

This change makes sure nativeEvent.locationX and .locationYare coordinates relative to the element, as described in the documentation. And as is current behavior on iOS. Should fix #3201.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek and @andreicoman11 to be potential reviewers.

@facebook-github-bot facebook-github-bot 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 Oct 14, 2015
@@ -53,6 +53,18 @@ public static int findTargetTagForTouch(
return targetTag;
}

public static View findTargetViewForTouch(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a simpler version of findTargetTagForTouch. Could share code instead of copy&pasting.

Missing documentation.

@mkonicek
Copy link
Contributor

How would you test this doesn't break any existing Android core components? Can you come up with an example use case in JS this would break?

@mkonicek
Copy link
Contributor

We'll open source all the Android integration tests so we'd see on Travis if this breaks anything.

@mkonicek
Copy link
Contributor

Can you link to the place in the docs where this is defined and the iOS implementation?

@mkonicek
Copy link
Contributor

Talked to @andreicoman11, this issue is valid, he said he'd likely look into fixing this himself.

@cypheon
Copy link
Author

cypheon commented Oct 15, 2015

Alright then, I'm glad someone more familiar with the code base will have a look at this. If @andreicoman11 is working on this as well, I don't need to bother preparing an updated patch.

The change shouldn't break any core components, because as of now, the only JS code using nativeEvent.locationX is in Libraries/Inspector/InspectorOverlay.js.
Anyway, thanks for the feedback, being part of the event handling, this really has to be low-overhead. So it makes sense to get rid of all unecessary allocations and calls.

@PhilippKrone
Copy link
Contributor

@mkonicek Do you have an idea when this issue will be solved? Is kind of blocking for me at the moment :)

PhilippKrone added a commit to PhilippKrone/react-native-slider that referenced this pull request Oct 22, 2015
Until the PR facebook/react-native#3426 is merged, we need to always return "true" in the method _handleStartShouldSetPanResponder, as discussed in jeanregisser#5 (comment).
@mkonicek
Copy link
Contributor

Checked with @andreicoman11, no ETA yet as there are many issues to prioritize. He has a task for it in the internal fb task tool.

Closing this PR as Andrei will take care of the implementation.

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.

PanResponder's nativeEvent Touch Location Differs on Android/iOS
6 participants