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.

Sign up for GitHub

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

Jump to bottom

Trigger non-touch events on box-none targets #906

Merged
merged 4 commits into from
Jul 19, 2022
Merged

Trigger non-touch events on box-none targets #906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e0664f
Trigger non-touch events on box-none targets
dcalhoun Jan 22, 2022
dd08c31
Event triggers target box-none View instead of child
dcalhoun Jan 22, 2022
4e58922
Rename events in test description
dcalhoun Jan 27, 2022
e504406
Merge branch 'main' of https://github.com/callstack/react-native-test…
dcalhoun Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/__tests__/fireEvent.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,36 @@ test('should not fire on box-only pointerEvents View with nested elements', () =
expect(handlePress).not.toHaveBeenCalled();
});

test('should fire non-terminal touch events on box-none pointerEvents View', () => {
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved
const handleTouchStart = jest.fn();

const screen = render(
<View pointerEvents="box-none" onTouchStart={handleTouchStart}>
<Pressable onPress={() => {}}>
<Text>Trigger</Text>
</Pressable>
</View>
);

fireEvent(screen.getByText('Trigger'), 'touchStart');
expect(handleTouchStart).toHaveBeenCalled();
});

test('should fire non-touch events on box-none pointerEvents View', () => {
const handleLayout = jest.fn();

const screen = render(
<View pointerEvents="box-none" onLayout={handleLayout}>
<Pressable onPress={() => {}}>
<Text>Trigger</Text>
</Pressable>
</View>
);

fireEvent(screen.getByText('Trigger'), 'layout');
expect(handleLayout).toHaveBeenCalled();
});

test('should pass event up on disabled TouchableOpacity', () => {
const handleInnerPress = jest.fn();
const handleOuterPress = jest.fn();
Expand Down
12 changes: 9 additions & 3 deletions src/fireEvent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,17 @@ const isPointerEventEnabled = (
return isPointerEventEnabled(element.parent, true);
};

const isTouchEvent = (eventName?: string) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name isTouchEvent feels odd given that onTouch* is not considered to be isTouchEvent here. However, I chose the name isTouchEvent based upon the language in the pointerEvents documentation. Open to feedback on this naming.

Copy link

Choose a reason for hiding this comment

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

I'm not sure whether this list is complete and/or even can be a static one because it may be platform-dependent.

On Android setting pointerEvents works by intercepting and discarding MotionEvents. These include at least touchstart, touchend, touchmove and touchcancel events. See: https://github.com/facebook/react-native/blob/main/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java#L220-L245.

In case of iOS it is setting this: https://developer.apple.com/documentation/uikit/uiview/1622577-userinteractionenabled?language=objc (related RN code: https://github.com/facebook/react-native/blob/main/React/Views/RCTView.m#L151-L158) to false, so it includes touch, press, keyboard, and focus events.

I'd experiment with both platforms and see what superset of events is covering both platforms.

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'm not sure whether this list is complete and/or even can be a static one because it may be platform-dependent.

@Killavus I agree it is likely incomplete, however it pointedly addresses the issue outlined in #897. I opted to target the specific discrepancy between React Native and RNTL I experienced.

In regards to the list being static, from my testing the onTouchStart, onTouchEnd, and onLayout events all function the same way on both platforms in relation to pointerEvents="box-none". This behavior is showcased in the example Expo Snack I created for #897. Both onTouchStart and onTouchEnd continue to fire for both iOS and Android even when the View is set to pointerEvents="box-none".

On Android setting pointerEvents works by intercepting and discarding MotionEvents. These include at least touchstart, touchend, touchmove and touchcancel events. See: https://github.com/facebook/react-native/blob/main/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.java#L220-L245.

In case of iOS it is setting this: https://developer.apple.com/documentation/uikit/uiview/1622577-userinteractionenabled?language=objc (related RN code: https://github.com/facebook/react-native/blob/main/React/Views/RCTView.m#L151-L158) to false, so it includes touch, press, keyboard, and focus events.

From reviewing the Android and iOS code in the links, I do not believe it directly invalidates this PR. The Android onInterceptTouchEvent intercepts when none or box-only, which is different then the current subject box-none. The iOS setPointerEvents disables userInteractionEnabled when pointerEvents="none", which again is different than the current subject box-none. Additionally, as showcased in the Expo Snack, the subject events continue to trigger when pointerEvents="box-none".

Please let me know if I am misunderstanding what you attempted to communicate with your code references.

I'd experiment with both platforms and see what superset of events is covering both platforms.

I might posit merging this PR as-is could be a step towards increased accuracy and consistency. A follow up PR could further increase accuracy and consistency. The ultimate "superset" is a bit unknown to me at this point. WDYT?

Copy link

Choose a reason for hiding this comment

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

That seems like a good approach for me. @thymikee has a final say here though!

Copy link
Member

Choose a reason for hiding this comment

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

Still haven't found time for this, please bare with me! :D

Copy link
Member

Choose a reason for hiding this comment

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

Hey @dcalhoun, I'll need more time to process it. Right now I focus on releasing a new library and needed to postpone this. It's not forgotten though.

In the meantime, I'll ask @AugustinLF, maybe he'll be able to check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @thymikee. That makes sense. I appreciate your time.

P.S. - feel free to ignore my ping from a different PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So if I understand correctly, your suggestion of implementation is strictly more correct than what we have, but is missing some additional cases? i.e. it makes things better, but doesn't fix everything, right?

If that's the case, I'm fine with merging this change. I'd like to have documentation (code and/or through a detailed issue) on the missing behaviour, so we can fix it going forward. Does that seem reasonable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your thoughts, @AugustinLF. 🙇🏻

So if I understand correctly, your suggestion of implementation is strictly more correct than what we have, but is missing some additional cases? i.e. it makes things better, but doesn't fix everything, right?

Correct. That was my sentiment in #906 (comment). There may be other cases where RNTL is misaligned with how RN implements pointerEvents, this PR provides a solution for the one case I ran into myself.

I'd like to have documentation (code and/or through a detailed issue) on the missing behaviour, so we can fix it going forward. Does that seem reasonable?

Yes, detailed documentation of the (unproven) further misalignment between RNTL and RN makes sense. That would help avoid any additional issues going unnoticed.

To be transparent, I have not fully explored further misalignments. I am unsure if I will be able to prioritize doing so in the near future. If the maintainers prefer to postpone merging this improvement until that research/documentation is completed, I respect that. However, my perspective is that one step towards alignment is better than no steps. I.e. it is worth merging this work as-is. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest to merge.

return eventName === 'press';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally, I structured this as return ['press'].includes(eventName); to allow easy additions to this allow list, but opted for a simple comparison for now.

};

const isEventEnabled = (
element?: ReactTestInstance,
touchResponder?: ReactTestInstance
touchResponder?: ReactTestInstance,
eventName?: string
) => {
if (isTextInput(element)) return element?.props.editable !== false;
if (!isPointerEventEnabled(element)) return false;
if (!isPointerEventEnabled(element) && isTouchEvent(eventName)) return false;

const touchStart = touchResponder?.props.onStartShouldSetResponder?.();
const touchMove = touchResponder?.props.onMoveShouldSetResponder?.();
Expand All @@ -59,7 +64,8 @@ const findEventHandler = (
: nearestTouchResponder;

const handler = getEventHandler(element, eventName);
if (handler && isEventEnabled(element, touchResponder)) return handler;
if (handler && isEventEnabled(element, touchResponder, eventName))
return handler;

if (element.parent === null || element.parent.parent === null) {
return null;
Expand Down