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

Improve react-native-gesture-handler::pressable handling #1738

Open
sidferreira opened this issue Jan 21, 2025 · 17 comments · May be fixed by #1741
Open

Improve react-native-gesture-handler::pressable handling #1738

sidferreira opened this issue Jan 21, 2025 · 17 comments · May be fixed by #1741

Comments

@sidferreira
Copy link

sidferreira commented Jan 21, 2025

Describe the Feature

I want to start acknowledging that's not RNTL responsibility, but asking for help to improve it.

As customer (and fan) I can offer some help but I believe https://github.com/callstack/react-native-testing-library and https://github.com/software-mansion/react-native-gesture-handler would solve these issues WAY faster.

At this moment, while the app is running, the RNGH::Pressable will deliver a very similar experience to RN::Pressable, but if we run tests, many things start to differ.

Some items are very easy to solve (like missing a11y states) but some are trickier ( RN::Pressable can respond to fireEvent(el, 'click') but RNGH::Pressable can't).

I'm creating this issue to get some help on this.

Known Differences on Testing:

  • RNGH::Pressable won't work with userEvent.press
  • RNGH::Pressable won't work with fireEvent(el, 'click')

Possible Implementations

#1741
software-mansion/react-native-gesture-handler#3357

Related Issues

software-mansion/react-native-gesture-handler#2385

@mdjastrzebski
Copy link
Member

I've checked the current state. These are the host components that RNGH Pressable renders in test (with Jest include they recommend):

test('GestureHandler Pressable', () => {
  const onPress = jest.fn();

  render(
    <Pressable onPress={onPress}>
      <Text>Hello</Text>
    </Pressable>,
  );

  expect(screen.toJSON()).toMatchInlineSnapshot(`
    <RNGestureHandlerButton
      collapsable={false}
      enabled={true}
      hitSlop={
        {
          "bottom": 0,
          "left": 0,
          "right": 0,
          "top": 0,
        }
      }
      rippleColor={0}
      style={
        [
          {},
          undefined,
        ]
      }
    >
      <Text>
        Hello
      </Text>
    </RNGestureHandlerButton>
  `);
});

Source: https://github.com/callstack/react-native-testing-library/blob/chore/gesture-handler/src/__tests__/react-native-gesture-handler.test.tsx

It definitely lacks any event handler exposed on the host component level.

@latekvo
Copy link

latekvo commented Jan 22, 2025

Hey, to clarify, Pressable doesn't directly pass the onPress callback to any of the host components. Instead, it intercepts the events coming to RNGestureHandlerButton using GestureDetector (also not a host view), and calls the provided onPress and other similar callbacks manually. The RNGestureHandlerButton is a native button, so I'm not sure if there's any space where we could artificially add onPress callback to the tree without adding needless complexity.
Are there any alternative ways that testing-library could invoke an event to the RNGestureHandlerButton that could work? I'm not exactly familiar with this project's inner workings, but I'll be happy to help with any question.

@mdjastrzebski
Copy link
Member

Hi @latekvo thanks for providing some context. I can definitely help with the testing library part. I first need to better understand the RNGH inner workings.

Can you explain how RNGH and RNGH Button connects native event to onPress event handler provided by the user step by step?

In RN, Pressable is a composite component with onPress prop that renders a host View component without onPress event handler but with some onResponder* pressability events. How does this differ from RNGH Button? Where the event passes from native code to JS code if there is no event handler prop?

@sidferreira
Copy link
Author

sidferreira commented Jan 22, 2025

Thank you, @latekvo and @mdjastrzebski, for helping with this goal!

I used to think the GestureDetector around NativeButton would intercept the gestures.

https://github.com/software-mansion/react-native-gesture-handler/blob/main/src/components/Pressable/Pressable.tsx

@latekvo
Copy link

latekvo commented Jan 23, 2025

@mdjastrzebski
Which platform does testing-library run on? Is it a specific one or multiple of them (Android, Web, iOS)?

I went through the event flow on android and web, and it seems like the only feasible entry point for the touch events would be to provide regular touch events directly to the native button (MotionEvents on android, PointerEvents on web).

Android:

On android, events are dispatched at the root of the view-tree, and travel downwards.
The RNGestureHandlerButton intercepts all native events propagating through it, which is possible by implementing onInterceptTouchEvent and onTouchEvent. It then forwards said events to our internal logic, which may decide to activate one or more of the handlers. If the STATE of any of the handlers ends up being modified, that change is forwarded to the JS side via DeviceEventEmitter (fabric) or DirectEventEmitter (paper), which results in calling of one of the callbacks of the gesture (.onStart(), .onEnd(), et cetera).
Native events that the RNGestureHandlerButton would have to be called with are: MotionEvent::ACTION_DOWN, MotionEvent::ACTION_UP

iOS:

Likely similar behaviour as on android, utilizes the same DirectEventEmitter mechanism.

Web:

On web, all callbacks provided in the gesture config (.onStart(), .onEnd(), et cetera) are hooked up directly to the RNGestureHandlerButton via addEventListener. Keep in mind, that various event-eligibility checks are performed, like checking if the event is in bounds of the gesture, or if the event should be ignored (pointerup dispatched before pointerdown).
Events that RNGestureHandlerButton would have to be called with: pointerdown, pointerup

Flowchart:

I've attached a visual graph of the flow of events, and as far as I see, no other entry points than listed are available. You could theoretically force Pressable to activate using the Native-JS gap, or the Web-JS gap, but that would require sending a series of DeviceEvents (for android) or manually calling the callbacks provided to the RNGestureHandlerButton (on web) with correct data, which is extremely hacky and fragile.

flowchart TD
	subgraph "JS Side"
		DeviceEventEmitter -- onGestureHandlerEvent, onGestureHandlerStateChange ---> onGestureHandlerEvent
		onGestureHandlerEvent --> Gesture.Native
		subgraph "Pressable"
			InternalLogic[Pressable JS Logic]
			Gesture.Native --> InternalLogic
			InternalLogic --> onPress
			InternalLogic --> onPressIn
			InternalLogic --> onPressOut
		end
	end

	subgraph "Android Implementation [Native]"		
		Event{{"MotionEvent::ACTION_DOWN"}}
		AndroidView([RNGestureHandlerButton])

		Event --> AndroidView

		subgraph RNGestureHandlerModule
			sendEventForDeviceEvent --> DeviceEventEmitter
		end
		subgraph GestureHandler
			dispatchHandlerUpdate --> sendEventForDeviceEvent
		end
		subgraph Orchestrator
			orchestratorLogic["Internal Gesture<br/>Activation Logic"]
			orchestratorLogic --> deliverEventToGestureHandler
			deliverEventToGestureHandler --> dispatchHandlerUpdate
		end

		AndroidView ---> RNGestureHandlerButton.onTouchEvent
		AndroidView -- First event ---> RNGestureHandlerButton.onInterceptTouchEvent
		
		RNGestureHandlerButtonViewManager --> NativeViewGestureHandler.onHandle

		subgraph RNGestureHandlerButtonViewManager
			RNGestureHandlerButton.onTouchEvent[onTouchEvent]
		    RNGestureHandlerButton.onInterceptTouchEvent[onInterceptTouchEvent]
		    RNGestureHandlerButton.onInterceptTouchEvent --> RNGestureHandlerButton.onTouchEvent
		end
		subgraph "NativeViewGestureHandler"
			NativeViewGestureHandler.onHandle[onHandle]
			NativeViewGestureHandler.onHandle --> orchestratorLogic
		end
	end
	subgraph "Web Implementation"
		WebEvent{{"pointerdown"}}
		WebView([RNGestureHandlerButton])
		
		Gesture.Native -. passes callbacks .-> attachEventManager

		attachEventManager --> registerListeners
		registerListeners --> WebView.addEventListener
				WebView.addEventListener["RNGestureHandlerButton.addEventListener('pointerdown')"]
		
		WebView.addEventListener -. attaches callbacks .-> WebView
				
		WebView -- calls callbacks directly --> Gesture.Native

		
	end
	WebEvent --> WebView
Loading

@latekvo
Copy link

latekvo commented Jan 23, 2025

@sidferreira

I used to think the GestureDetector around NativeButton would intercept the gestures.

Yeah that's exactly how it works from the user's perspective.
The inner implementation is a bit more complex though, and uses different methods of intercepting the events depending on the platform. Let me know if the previous comment has any unclarities, I'll try to fix them.

@mdjastrzebski
Copy link
Member

@latekvo Thanks for detailed explanation. So as far as I understand, in order to raise onPress (and related) events on RNGH Pressable RNTL would have to either 1) invoke handlers on composite Pressable component or 2) emit RNGH-specific events to DeviceEventEmitter. For the first way we would like to avoid calling composite components props as this method not-bulletproof and will likely not be supported in the replacement of deprecated React Test Renderer. As for second option it requires knowledge of RNGH internals, which makes it not suitable for inclusion to RNTL as it's better handled inside RNGH.

You already seem to have RNGH specific tooling for invoking different gestures in form of fireGestureHandler. The case with Pressable is a bit different though, as user might reasonably expect that a pressable should simply accept press events.

I see a simple solution which we could use to bridge our libraries:

  1. RNGH to expose testOnly_onPress, (+ testOnly_onPressIn, testOnly_onPressOut) on host RNGestureHandlerButton. The testOnly_ prefix is already used in RN Pressable for exposing test-specific features (see here).
  2. RNTL would inspect not only onXxx but also testOnly_onXxx event handlers in both Fire Event/User Event.

This way, just in testing env. RNTL would have access to relevant event handlers on host components RNGestureHandlerButton. This would avoid going through RNGH internals tough, which is a sligh downside as it reduces reliability of the test. That being said press event is rather simple one so that should be a good trade off for the users.

@latekvo, @sidferreira wdyt?

@latekvo
Copy link

latekvo commented Jan 27, 2025

The testOnly_ solution sounds good.

I tested react-test-renderer a bit further, and it looks like none of the native implementations of RNGestureHandlerButton are read, including the web one. When reading the host view tree via screen.toJSON(), the tree rendering stops on Pressable.
This might be due to:

doesn’t contain any user-written components

Image

I'm not sure if you need Pressable to also work with screen.toTree() and other more complex react-test-renderer functions, or if screen.toJSON() is enough.

Also, should we also be handling testing-library's press events, or does testing-library just extract the testOnly_onPressX callbacks, and calls them directly?

Lastly, I couldn't find any information on what will react-test-renderer be replaced with after it's deprecation, and if that matters in context of this GH-TL integration.
Are you aware of any specific details regarding that deprecation that I should take into account?

Here's the PR applying those changes: software-mansion/react-native-gesture-handler#3357
Of course feel free to leave a review if you find anything wrong.

@sidferreira
Copy link
Author

@latekvo, will you add onClick as well? Some people often use it...

@latekvo
Copy link

latekvo commented Jan 27, 2025

@sidferreira, I think onClick doesn't exist in any of the core React Native libraries.
What is the context in which you are usually using it?

If testing-library provides some kind of interface to use onClick function, despite lack of it in the tested components, i think it's addition is out of gesture-handler's scope.

Let's wait for @mdjastrzebski's opinion on this, as I'm completely unfamiliar with the inner-workings of testing-library.

@mdjastrzebski
Copy link
Member

@sidferreira what is the story with onClick which component declares it?

@mdjastrzebski
Copy link
Member

@latekvo I've checked your PR, looks good. RNTL would be responsible for inspecting and calling testOnly_onPressXxx callbacks from both Fire Event and User Event.

@latekvo
Copy link

latekvo commented Jan 27, 2025

Sounds great. I'll wait for other gesture-handler maintainers to take a look at the PR, and we'll be ready to merge.

@sidferreira
Copy link
Author

sidferreira commented Jan 27, 2025

@latekvo and @mdjastrzebski Sorry for my very short comment before, I was helping the kid with his homework. Now, if you search onClick in RN's repo ( https://github.com/search?q=repo%3Afacebook%2Freact-native%20onclick&type=code ) you will find many responders to onClick, in what seems to be a native behavior for Views, to the point that fireEvent(el, 'click') does work in many cases.

My understanding is that RNGH::Pressable is a drop-in replacement and, ideally, all tests that pass for RN::Pressable must pass for RNGH::Pressable...

TLDR: I think onClick behaves like an "alias" to onPress but RNGH does not handle it

@latekvo
Copy link

latekvo commented Jan 28, 2025

@sidferreira I've looked through the React Native repo, and while it looks like there's some initial implementation present, one of the occurrences mentions that onClick among other Pointer Events is "experimental" and "not yet ready for use" (link).
React Native docs also mention onClick only once, in a 2022 blog about backwards-compatibility with Web (link), mentioning that onClick is yet to be implemented in React Native.

To me it sounds like any usages of onClick are undefined behaviour, and may be working only because of some undocumented backwards-compatibility feature that React Native hasn't fully implemented yet.
I'd prefer to wait for an official, or at least RC release of those features before implementing their handling.

@mdjastrzebski
Copy link
Member

I would not mix supporting click event to RNGH Pressable support. As mentioned by @latekvo this is experimental and seems to bring no additional benefits over regular press events. If click event support should be discussed that would should be a separate issue for RNTL and/or RNGH.

@mdjastrzebski mdjastrzebski linked a pull request Jan 28, 2025 that will close this issue
@sidferreira
Copy link
Author

sidferreira commented Jan 28, 2025

@latekvo @mdjastrzebski TIL onClick is experimental... I assumed it was legacy or something. Will fix our code to prioritize onPress over onClick.

Still, if you use fireEvent(el, 'click') on RN Pressables, it does trigger onPress events.

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

Successfully merging a pull request may close this issue.

3 participants