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

fix: pointer events getter #2395

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

coado
Copy link
Collaborator

@coado coado commented Aug 2, 2024

Summary

Regarding this issue, the PR adds a getter for mPointerEvents in VirtualView class. Thanks to this, the pointer event type can be accessed from the RNSVG components in react-native-gesture-handler.

Test Plan

You can test it by changing the pointerEvents value and checking if the 'tap' is logged when tapping on the red square:

import React from 'react';
import { SafeAreaView, StyleSheet } from 'react-native';
import Svg, {  Rect } from 'react-native-svg';
import { Gesture, GestureDetector, GestureHandlerRootView } from 'react-native-gesture-handler';

export default function App() {
  const tap = Gesture.Tap().onBegin((event) => console.log('tap'));

  return (
    <GestureHandlerRootView>
      <SafeAreaView style={styles.container}>
        <Svg
          xmlns="http://www.w3.org/2000/svg"
          xmlnsXlink="http://www.w3.org/1999/xlink"
          xmlSpace="preserve"
          viewBox="0 0 382.7 382.7"
          id="container">
          <GestureDetector gesture={tap}>
            <Rect
                width={90.3}
                height={90.3}
                x={242.7}
                y={242.7}
                fill="red"
                strokeWidth={1}
            />
          </GestureDetector>

        <Rect
            width={375}
            height={374}
            fill="none"
            stroke="#000"
            strokeWidth={1}
            pointerEvents="none" 
          />
        </Svg>
      </SafeAreaView>
    </GestureHandlerRootView>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    backgroundColor: '#ecf0f1',
    padding: 8,
  },
});

Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jakex7 jakex7 left a comment

Choose a reason for hiding this comment

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

I wonder if we should move mPointerEvents to RenderableView as VirtualView is also extended by DefinitionView that shouldn't be pressable?

jakex7 added a commit that referenced this pull request Aug 5, 2024
@bohdanprog
Copy link
Member

bohdanprog commented Aug 6, 2024

Related Closes #2239 to that problem.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Aug 12, 2024
Summary:
I maintain the `react-native-svg` library, where our elements extend `ReactViewGroup`. Currently, `ReactViewGroup` only exposes the getter for `mPointerEvents` publicly, so we cannot set it. To properly handle `pointerEvents`, we would have to duplicate all methods related to `mPointerEvents`, which results in maintaining a separate state. This duplication can lead to desynchronization between the state in our class and the state in the superclass.

PR with a workaround that we can avoid with this change software-mansion/react-native-svg#2395

## Changelog:

[ANDROID] [CHANGED] - make `setPointerEvents` public

Pull Request resolved: #45975

Test Plan: This change was tested manually by making the field public, allowing dependent classes to override or reference it.

Reviewed By: cortinico

Differential Revision: D61124293

Pulled By: javache

fbshipit-source-id: 389d0a670375a8a68c975294f98c33c28ef41ffe
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 this pull request may close these issues.

4 participants