nandorojo / zeego

Menus for React (Native) done right.
https://zeego.dev
MIT License
1.42k stars 41 forks source link

[SOLVED] Context menu crashes app when wrapping a navigation trigger #61

Open peterferguson opened 9 months ago

peterferguson commented 9 months ago

Hey first off thanks for the library!

On "zeego": "^1.7.0" I have noticed an issue where if you try to cancel opening the context menu just before it should open then you get the navigation & the menu opens then app crashes.

I pulled the code from the example and wrapped my ConversationPreview component with no changes. The component is pretty simple, just a Pressable with an expo-router router.push.

This is using "expo": "^49.0.9" in a dev client & "expo-router": "2.0.4".

If you can't replicate let me know and I can try create a repo.

Not a big enough issue to commit time to rn but thought I would let you know

https://github.com/nandorojo/zeego/assets/7002211/d8cd9739-c165-4515-9c46-29741869beaf

nandorojo commented 9 months ago

ah that’s not good! i’m working through some other issues with the menu but i’ll try to address this if the reason becomes apparent. it’s hard coordinating with native view controllers for these edge cases

peterferguson commented 8 months ago

Found this on issue that solves the problem

https://github.com/dominicstop/react-native-ios-context-menu/issues/9#issuecomment-1047058781

Maybe for zeego is it just better to have this documented somewhere?

A FAQ or common pitfalls section?

I will leave it up to you whether to close or not (might be worth leaving open if you are open to the docs changes?)

nandorojo commented 8 months ago

Oh yeah that does make sense. I wonder if we should make it the default behavior or not. Or I also wonder if there is a way to fix this upstream entirely.

peterferguson commented 8 months ago

Seems like it could be simple enough to check if the child passed to the trigger has onLongPress set and if not override it.

Also a reasonably small change that (on first glance) shouldn't have many repercussions in other trigger use-cases.

nandorojo commented 8 months ago

The issue is, the child of the context menu could be anywhere (as in multiple children down).

So if a nested child is the pressable, it won't get auto-fixed.

nandorojo commented 8 months ago

@peterferguson this is getting fixed upstream in the context menu v2:

https://github.com/dominicstop/react-native-ios-context-menu/issues/77#issuecomment-1789884706

sidorchukandrew commented 6 months ago

This seems to still be happening on the latest version of zeego (1.7.2) and react-native-ios-context-menu (2.3.0). Is it possible that zeego needs to set shouldPreventLongPressGestureFromPropagating on the ContextMenuView? It looks like it's a new prop (I don't know much Swift but it also looks like the default value is getting set to true)

nandorojo commented 6 months ago

Hmm good to know

nandorojo commented 4 months ago

@sidorchukandrew you can now pass __unsafeIosProps to Zeego's ContextMenu to test this. It'll forward props down

sidorchukandrew commented 4 months ago

I'll get that tested shortly!

sidorchukandrew commented 4 months ago

@nandorojo Is there a new release I should install?

peterferguson commented 4 months ago

Just tried this

<ContextMenu.Root __unsafeIosProps={{ shouldPreventLongPressGestureFromPropagating: true }}>

but is still crashing for me on zeego@1..7.2 & react-native-ios-context-menu@2.3.0

sidorchukandrew commented 4 months ago

@peterferguson There's a new version of react-native-ios-context-menu out.

nandorojo commented 4 months ago

There are some fixes underway for v2