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] Change onTouchEvent to dispatchTouchEvent in NativeViewGestureHandler #3244

Merged
merged 8 commits into from
Dec 4, 2024

Conversation

m-bert
Copy link
Contributor

@m-bert m-bert commented Nov 28, 2024

Description

Currently wrapping WebView into NativeViewGestureHandler doesn't work as expected - events from native gesture do not reach WebView. This happens because inside NativeViewGestureHandler we call onTouchEvent method. In native hierarchy WebView is nested inside ViewGroup, which means that calling onTouchEvent instead of dispatchTouchEvent won't do anything to the WebView.

Should fix #2454

Fixes #3196

Test plan

Tested on example app and the code below:

Test code
import * as React from 'react';
import { View, StyleSheet } from 'react-native';
import { WebView } from 'react-native-webview';
import {
  Gesture,
  GestureDetector,
  GestureHandlerRootView,
} from 'react-native-gesture-handler';

function WebViewScreen() {
  const native = Gesture.Native()
    .shouldActivateOnStart(true)
    .disallowInterruption(true);
  const pan = Gesture.Pan().onChange(console.log);

  const g = Gesture.Simultaneous(native, pan);

  return (
    <View style={styles.webViewContainer}>
      <GestureDetector gesture={g}>
        <WebView
          source={{ uri: 'https://templates.tiptap.dev/nw6Cmz6HfD' }}
          style={styles.webView}
          javaScriptEnabled={true}
          domStorageEnabled={true}
          startInLoadingState={true}
          onError={(syntheticEvent) => {
            const { nativeEvent } = syntheticEvent;
            console.warn('WebView error: ', nativeEvent);
          }}
        />
      </GestureDetector>
    </View>
  );
}

export default function BuggyApp() {
  return (
    <GestureHandlerRootView>
      <WebViewScreen />
    </GestureHandlerRootView>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    alignItems: 'center',
    justifyContent: 'center',
  },
  text: {
    fontSize: 18,
  },
  webView: {
    flex: 1,
  },
  webViewContainer: {
    flex: 1,
    width: '100%',
  },
});

@m-bert m-bert requested a review from j-piasecki November 28, 2024 11:23
@@ -99,7 +99,7 @@ class NativeViewGestureHandler : GestureHandler<NativeViewGestureHandler>() {
if (state == STATE_UNDETERMINED && !hook.canBegin(event)) {
cancel()
} else {
view.onTouchEvent(event)
view.dispatchTouchEvent(event)
Copy link
Member

Choose a reason for hiding this comment

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

Does this impact any other views? dispatchTouchEvent sounds like it does something significantly different than onTouchEvent.

Copy link
Contributor Author

@m-bert m-bert Nov 28, 2024

Choose a reason for hiding this comment

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

From my long journey through Android source code I can say that it is not entirely different. In short ViewGroup.dispatchTouchEvent calls View.dispatchTouchEvent, which calls View. performOnTouchCallback, which calls View.onTouchEvent. So in the end we still call this function. The thing is, beside calling onTouchEvent indirectly, android also checks for interceptions and extracts touch target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go through example app one more time and focus more on the examples that use NativeViewGestureHandler.

Also, what exactly do you mean by

Does this impact any other views?

What do you think could be affected?

@Nantris
Copy link

Nantris commented Nov 29, 2024

I just wanted to comment to confirm this does appear to resolve #3196! I detailed a finding of my testing there which may be worth reading through for anyone reviewing this

#3196 (comment)

A big thanks to @m-bert for the great work! I hope this can be merged without breaking anything!

@Nantris
Copy link

Nantris commented Nov 29, 2024

Also, I won't pretend I have the expertise to know much here, but I noticed there's one place that onTouchEvent is still used. I assume that's purposeful?

@m-bert
Copy link
Contributor Author

m-bert commented Dec 2, 2024

Also, I won't pretend I have the expertise to know much here, but I noticed there's one place that onTouchEvent is still used. I assume that's purposeful?

Yes, it is 😅

@m-bert m-bert requested a review from j-piasecki December 2, 2024 08:49
Comment on lines 86 to 94
fun triggerEvent(event: MotionEvent) {
val view = view!!

if (view is ReactViewGroup) {
view.dispatchTouchEvent(event)
} else {
view.onTouchEvent(event)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We may want this to be part of the native gesture hook, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Done in ac88178

@m-bert m-bert requested a review from j-piasecki December 2, 2024 09:44
Comment on lines 205 to 208
/**
* Decides whether or not target view should pass event to children.
*/
fun triggerEvent(view: View?, event: MotionEvent) = view?.onTouchEvent(event)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* Decides whether or not target view should pass event to children.
*/
fun triggerEvent(view: View?, event: MotionEvent) = view?.onTouchEvent(event)
/**
* Passes the event down to the underlying view using the correct method.
*/
fun sendTouchEvent(view: View?, event: MotionEvent) = view?.onTouchEvent(event)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4ca8726

@m-bert m-bert merged commit f0604b0 into main Dec 4, 2024
3 checks passed
@m-bert m-bert deleted the @mbert/native-interactions branch December 4, 2024 16:47
@lovegaoshi
Copy link

just want to leave a comment that I started seeing ScrollView E Invalid pointerId=-1 in onTouchEvent with this commit only on Android 15 (both emulator and pixel 7; Android 14/S21 is fine), when using a RNGH scrollview as flashlist's renderScrollComponent. ScrollView wont scroll anymore. I reverted exactly this commit and can scroll fine then.

@m-bert
Copy link
Contributor Author

m-bert commented Feb 5, 2025

Hi @lovegaoshi! Do you have any reproducible example? As far as I can see this package is not actively supported and depends on outdated versions of libraries, especially Gesture Handler.

@lovegaoshi
Copy link

lovegaoshi commented Feb 5, 2025

i think I'll just open an issue avoid polluting the PR tracker.

thx for the attention! Let me clarify that the problem doesnt necessarily has to do with flashdraglist being unmaintained - at its core, its just an Animated wrapped flashlist with RNGH's ScrollView used as its renderScrollComponent. You can reproduce with simply that.

while this is not exactly a minimum reproducer, I removed everything except the exact bug in index.js's entry point. You can build via git clone, yarn, yarn build, then build via Android Studio. Scrolling this list specifically on android 15 (emulator or real device) will yield this.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants