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
Restoring pointer events box_none behavior on Android #1808
Restoring pointer events box_none behavior on Android #1808
Conversation
Can this get merged please. Thank you. |
+1 |
+1 |
1 similar comment
+1 |
+1 |
+1 |
Conflict solved and retested. |
I applied the changes and run the example attached on the newest main in |
Expected behavior. This change aims to make only the painted area touchable (if it's using the pointerEvents={'box-none'}). Therefore the upper right corner of the blue section SVG won't respond to touch cause there is nothing painted there. |
@labmorales but here on the video you click on the upper corner of the blue section and it fires the event: |
Like any other element, SVG is a rectangle. The video shows two SVG on top of each other (code: https://snack.expo.dev/@amorales85/react-native-svg---box-none-not-working-properly). The PR makes it possible for only the painted area of SVG (blue background shapes and red background shapes) to respond to a click event. Without this fix, the transparent (not painted area of the rectangle) would also respond to events. That would lead to issues when overlapping SVG's. In the image below I'm tapping an area of the SVG (inside it's a drawable area) however it does not have anything painted there, therefore it should not result in an event. Of course, this behavior only happens when you set the pointerEvents box-none like the example below. <Svg
width={180}
height={115}
fill="none"
style={{position: 'absolute', top: 0, left: 0}}
pointerEvents={'box-none'}>
<Path
opacity={0.4}
pointerEvents={'box-none'}
onPress={() => Alert.alert('BLUE SECTION')}
d="M178.829 50.333c-24.775-17.185-77.2-39.96-102.06-49.895a1.96 1.96 0 0 0-2.017.348L2.169 63C.045 64.77-.147 65.888.07 66.352c.05.109.128.19.199.287C5.078 73.19 33.718 86.532 40.669 91c7 4.5 37.5 23 42.5 23.5 4 .4 18-9.167 24.5-14l71.102-46.917c1.162-.767 1.202-2.457.058-3.25Z"
fill="#2E90FA"
/>
</Svg> I can create a repo with the code that I used for the demo if you like. But following the steps on the "What are the steps to reproduce" section should lead to the same code and results. |
Maybe something needs to be changed after the conflicts were solved. I'll test it out and will provide a repo with a working example. |
@WoLewicki seems like pointer-events values are no longer being accepted in the Path element in the newest version o react-native-svg. Doesn't matter the value you assign to the element it stays as AUTO. That's part of the issue. Is that by design? Cannot find a way to enable them again. |
@WoLewicki I don't know if it helps but I created a repo with the fix that I made on top of version 12.4.4 of react-native-svg. In it, you'll be able to see the fix working (https://github.com/labmorales/react-native-svg-demo-android) just like in the video. With the newer versions of react-native-svg it does not work. My guess is that it does not work because:
Any tips, on how to fix those two things? |
@labmorales do you test it on the new arch or the old one? It may be a problem with the changes of the extended view managers which do not forward |
@WoLewicki don't think so. I'm using expo 45 and react-native 68 and downloaded the initial files directly from https://snack.expo.dev/ demo. Didn't do anything to enable Fabric. Don't think that's enabled by default at this version. |
It is not enabled, but now the code of library uses the managers and delegates generated by Fabric, so unfortunately bugs from Fabric can affect old architecture. Therefore, it might be necessary to add |
@labmorales could you include #1879 in your tests? It should fix handling |
Hi, @WoLewicki will do! |
@WoLewicki I merged #1879 into my branch and it did not solve the problem. The pointerEvents is still being set to AUTO on Android no matter what value I set on the react component. |
@WoLewicki I just got it working. I made the same change you did on #1879 for the other elements for the SvgView and now it's working 🎉 🎉 🚀 . Demo code running with the fix (this branch) can be found here. |
@labmorales the code you provided in demo crashes for me on |
@WoLewicki totally forgot about that. iOS does not handle box-none. I think it only works with none or auto. Therefore to use box-none for android we need to check the platform first. const pointerEvents = Platform.OS === 'ios' ? '' : 'box-none'; Updated my code adding that check. It should work now. |
I took a deeper dive on how
|
But despite that, it seems that behavior differs on |
@WoLewicki I think so. How about we create a separate ticket for that? I never programmed anything on Swift or Objective C, so for me is going to be pretty hard to work on that. |
Yeah, we should do it like that. I'll merge the PR with |
@labmorales I merged the PR. Please rebase yours and we can then test if it works as it should 🚀 |
@WoLewicki I just tested it and it did not work. We need the changes that I did to the RenderableView in this PR to make it work (https://github.com/react-native-svg/react-native-svg/pull/1808/files#diff-478360aa26a523f45f1f6c9ebd8d5a060a4618b6179f69c609ee6166e4a3fc15). This is necessary because of these lines were changed in react-native 66. So the whole box-none changed. The getHitSlopRect makes this line return null in case the view has pointerEvents="box-none" and when that happens it's possible to reach this line calling the reactTagForTouch method which was the old plugin behavior prior to RN 66. |
@WoLewicki also reverted these changes you made. I was getting null as the pointerEvents value for the SVG (react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/TouchTargetHelper.java) on this part: private static @Nullable View findTouchTargetViewWithPointerEvents(
float eventCoords[], View view, @Nullable List<Integer> pathAccumulator) {
PointerEvents pointerEvents =
view instanceof ReactPointerEventsView
? ((ReactPointerEventsView) view).getPointerEvents()
: PointerEvents.AUTO; That was crashing the app. |
I believe you had to revert changes in |
@WoLewicki you are right. Just refactored removing the local |
So does it work correctly now? Is this PR ready for review? |
@WoLewicki it works correctly. Yes, please review 😃. |
Great 🚀. So could you add a test example into |
@WoLewicki just added a test example for the pointer events. Here is a video of it running on Android. Screen_Recording_20221005_105046_TestsExample.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉 Thanks for your contribution!
Summary
This PR is a bug fix and makes the pointer-events box-none work again after react-native 66 changes. It changes a bit how it should be used as I'll describe below. This solves #1807 .
I created a getHitSlopRect in the RenderableView that responds with a Rect that make the hit test detection fail on the react-native forcing it to run reactTagForTouch like it used to in previous versions.
This PR impacts pointer events box-none usage. This is an important change for me because it enables the usage of overlapping SVG keeping only the painted area as touchable elements, and not the whole SVG box.
Test Plan
Download the snack (https://snack.expo.dev/@amorales85/react-native-svg---box-none-not-working-properly) change the dependency from react-native-svg to load the fix branch:
working_react_native_66.mov
What's required for testing (prerequisites)?
All react-native and android prerequisites.
What are the steps to reproduce (after prerequisites)?
Notes
This changes a bit how pointerEvents box-none is used. Now use it you must define box-none on SVG level and on the last descendent child that has the event attached. Like this:
That was the only way I found in order to make it work.
Compatibility
Checklist
README.md
__tests__
folder