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

dcalhoun · 2022-01-22T14:35:32Z

Summary

Fixes #897. Currently, this library disables all events for a target with pointerEvents="box-none". This behavior is counter to how React Native itself functions. This change continues to disable touch events, e.g. press, for targets set to "box-none", but allows triggering non-touch events, e.g. layout, touchStart.

Test plan

Verify all CI tests pass, including the new tests within this pull request.

Currently, this library disables _all_ events for a target with `pointerEvents="box-none"`. This behavior is counter to how React Native itself functions. This change continues to disable touch events, e.g. `press`, for targets set to "box-none", but allows triggering non-touch events, e.g. `layout`, `touchStart`.

src/__tests__/fireEvent.test.js

dcalhoun · 2022-01-22T14:44:28Z

src/fireEvent.js

@@ -33,12 +33,17 @@ const isPointerEventEnabled = (
  return isPointerEventEnabled(element.parent, true);
 };

+const isTouchEvent = (eventName?: string) => {


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.

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.

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?

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

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

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.

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

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

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?

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?

I'd suggest to merge.

dcalhoun · 2022-01-22T14:44:35Z

src/fireEvent.js

@@ -33,12 +33,17 @@ const isPointerEventEnabled = (
  return isPointerEventEnabled(element.parent, true);
 };

+const isTouchEvent = (eventName?: string) => {
+  return eventName === 'press';


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.

This approach more closely aligns with the original failing test case.

Killavus

@thymikee I think code-wise it looks good but I don't think it's right now doing the right thing semantically. As explained in https://github.com/callstack/react-native-testing-library/pull/906/files#r792240808 it may be needed to find a superset of events this option is supressing before merge.

Attempt to better communicate the test is focused on ensuring that touch events continue to trigger (e.g. `touchStart`), but the final event from a touch tap does not trigger.

…ing-library into trigger-non-touch-events-on-box-none-targets

AugustinLF

As discussed in this thread #906 (comment), we should look to better document the platform difference to get a semantically more correct behaviour, but this pull request is still an improvement on the current status, so I'd say let's merge.

thymikee · 2022-07-19T09:55:53Z

Ok, let's do this. We'll fit it into the next major release.

Btw, please check the work that the core team is doing on the pointer events recently: https://github.com/facebook/react-native/search?q=pointer+events&type=commits. They're making commitments to have pointer events behave more web-like.

dcalhoun force-pushed the trigger-non-touch-events-on-box-none-targets branch from 747b6af to 30602bc Compare January 22, 2022 14:41

dcalhoun force-pushed the trigger-non-touch-events-on-box-none-targets branch from 30602bc to 0e0664f Compare January 22, 2022 14:43

dcalhoun commented Jan 22, 2022

View reviewed changes

dcalhoun marked this pull request as ready for review January 22, 2022 14:46

Event triggers target box-none View instead of child

dd08c31

This approach more closely aligns with the original failing test case.

dcalhoun mentioned this pull request Jan 24, 2022

Setting pointerEvents to "box-none" prevents triggering onTouch* and onLayout events on the element #897

Closed

Killavus reviewed Jan 26, 2022

View reviewed changes

dcalhoun added 2 commits January 27, 2022 08:29

Rename events in test description

4e58922

Attempt to better communicate the test is focused on ensuring that touch events continue to trigger (e.g. `touchStart`), but the final event from a touch tap does not trigger.

Merge branch 'main' of https://github.com/callstack/react-native-test…

e504406

…ing-library into trigger-non-touch-events-on-box-none-targets

thymikee requested review from AugustinLF and mdjastrzebski June 14, 2022 09:34

AugustinLF approved these changes Jul 13, 2022

View reviewed changes

thymikee merged commit d2269f6 into callstack:main Jul 19, 2022

mdjastrzebski mentioned this pull request Oct 16, 2022

Test fails because new isPointerEventEnabled prevents handler from getting called #699

Closed

dcalhoun deleted the trigger-non-touch-events-on-box-none-targets branch June 20, 2023 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dcalhoun commented Jan 22, 2022

dcalhoun Jan 22, 2022

Killavus Jan 26, 2022

dcalhoun Jan 27, 2022

Killavus Feb 2, 2022

thymikee Feb 2, 2022

thymikee Jun 20, 2022

dcalhoun Jun 20, 2022

AugustinLF Jun 29, 2022

dcalhoun Jun 29, 2022

AugustinLF Jul 13, 2022

dcalhoun Jan 22, 2022

Killavus left a comment

AugustinLF left a comment

thymikee commented Jul 19, 2022

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

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

Conversation

dcalhoun commented Jan 22, 2022

Summary

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Killavus left a comment

Choose a reason for hiding this comment

AugustinLF left a comment

Choose a reason for hiding this comment

thymikee commented Jul 19, 2022