software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
6.13k stars 982 forks source link

fix(web): broken at dropping #3125

Closed MJRT closed 1 month ago

MJRT commented 1 month ago

Description

After upgraded RN from 0.74 to 0.75 ( follow expo changelog), web page is broken when I do a drag and drop.

console log: https://gist.github.com/MJRT/149969001b9dd853e6b7d0d413afcea7 expo-device-info: https://gist.github.com/MJRT/2b772c0879f0a867ffb6bcf4a18d8637

It return to normal after adding this check.

Test plan

I use pnpm patch in my project, and it's all ok. Since this is a simple and non-invasive change, I didn't write additional test.

m-bert commented 1 month ago

Hi @MJRT, thank you for this PR 😄

web page is broken when I do a drag and drop.

Do you mean Drag & drop example from our example app?

I've seen this error a few times but it happened more-less randomly and it would be great if you could provide some reproduction code - maybe we will find root cause instead of just adding a null check 😅

MJRT commented 1 month ago

Drag & drop example from our example app?

I haven't tried this example yet, but this error can stable reproduce in my project after upgrade to RN 0.75 and never see it on RN 0.74.

I’ll extract some code to reproduce it in a while. But I’m quite busy these days, so it might take a few days

m-bert commented 1 month ago

I’ll extract some code to reproduce it in a while. But I’m quite busy these days, so it might take a few days

It's okay 😅 If I come across this error I'll let you know.

m-bert commented 1 month ago

Hi @MJRT! I think I've found out what was the problem in our case.

Under some circumstances we enter this if statement, which basically means that gestureHandler is not initialized.

The problem is, currently it is a silent fail and we would like to change it to Error instead. However I'm not sure what caused that crash in your case and it would be great to check that before doing so.

MJRT commented 1 month ago

Hi @m-bert , thanks for your sync, I extract a little code to reproduce this issue: https://github.com/MJRT/gesture-handle-web-broken-demo

it may have relation with I use lucide icons, you can stable reproduce it when switch tabs.

And I think we best can address this situation, that bring consistent dev experience, instead of handle a exception alone on web

m-bert commented 1 month ago

Okay, I've checked your repro and I know what happened. For some reason <svg> is not treated as HTMLElement (it isSVGElement instead), so there was a silent fail on the line that I've mentioned earlier.

Now, we would like to give you opportunity to contribute to our library, but we do want to solve it in a different way than you proposed. Would you mind if I edit this PR?

MJRT commented 1 month ago

Sure, tell me what I can do to help. I’m eager to contribute and collaborate on this solution.

m-bert commented 1 month ago

Okay, I've introduced required changes. Turns out that there was more to it than meets the eye so I've decided to do dirty work by myself. Thank you once more for spotting this problem and submittin PR ❤️