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.

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

Fix: web modals being closed unintentionally #6097

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

khuddite
Copy link
Contributor

@khuddite khuddite commented Nov 4, 2024

close #6096 and close #6040

  • Description of the issue
    The reproduction steps and details are well described in the linked issue. In addition to that, this issue is not specific to the custom domain modal but all modals on the web.

  • Root cause explained
    The backdrop component is listening to onPress event and closes out the modal when it happens.
    But as described here, onPress event occurs after onPressOut event, which means it will occur whenever the mouse is pressed out on the Pressable component.
    And that's exactly what's happening in the current implementation. When you press the mouse inside the modal and drag it out to the backdrop component (TouchableWithoutFeedback), the onPress event still occurs thus the modal disappears.

  • Changes made in the PR
    Replaced onPress with onPressIn so the modal closes only when the mouse press event happens inside the backdrop (not the modal).

  • Screen recording of the updated behavior
    https://github.com/user-attachments/assets/df9bade5-fb0d-4027-a7ed-f3312f38d1ee

@khuddite khuddite changed the title use onPressIn instead of onPress for more accurate behavior for modal… Fix: web modals being closed unintentionally Nov 4, 2024
@mary-ext
Copy link
Contributor

mary-ext commented Nov 4, 2024

This doesn't seem ideal as onPressIn gets called when it is held, not when it's no longer held (see the how it works section on the docs).

@khuddite
Copy link
Contributor Author

khuddite commented Nov 4, 2024

@mary-ext Appreciate your speedy response.
That's a good callout. I updated the PR to utilize onPressOut, which seems to be doing exactly what we want here.

What I've noticed, though, is when you press outside the modal, drag it inside the modal, and release, that still closes out the modal, which I think is probably okay.

@khuddite
Copy link
Contributor Author

khuddite commented Nov 4, 2024

CleanShot.2024-11-03.at.23.53.57.mp4

Here's the updated behavior.

@khuddite
Copy link
Contributor Author

khuddite commented Nov 4, 2024

CleanShot.2024-11-04.at.00.12.20.mp4

I've found this interesting behavior in the production build. The issue starts not happening once I drag from outside to inside.

So, I think there is something more to investigate here, and I will keep posting my updates here.

@khuddite
Copy link
Contributor Author

khuddite commented Nov 4, 2024

I am not 100% sure what's happening under the hood, but here are my findings.
drag in = (drag from the outside modal to the inside) drag out = (drag from the inside modal to the outside)

  1. When I drag out, only the onPress event occurs for the backdrop.
  2. When I drag in
    • without selecting anything in the modal, all onPressIn, onPressOut, and onPress events (no `onPress) occur for the backdrop. Thus, the modal closes out.
    • When selecting something in the modal, only onPressIn and onPressOut occur for the backdrop. Thus, the modal doesn't close.
  3. Once I do case 2 with selection, it changes case 1, and onPress event starts not occurring, so no events occur for the backdrop. That's why the above behavior is happening in the production build.

Here is a visual explanation of what's described above -

CleanShot.2024-11-04.at.10.40.34.mp4

Here is my fix.
I introduced an additional state flag, isMaskPressedIn to make sure that onPress event is ignored when occurred without onPressIn event for the backdrop. It prevents case 1 from closing out the modal and fixes the reported issue.

My only concern with the fix is when case 2 happens without the selection, it still closes out the modal.

@mary-ext If you can take another look, that would be appreciated. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants