Close QR-modal on outside click

[owi92]

This closes the modal when clicked outside, but keeps the share menu open.
Closes #879

[LukasKalbertodt]

The problem with this approach is that clicking inside the modal (e.g. on the QR code) also closes the modal, as "outside" is defined as "outside of ref". And ref is the share menu. So the whole modal overlay is outside.

I think I'd prefer it if the Modal would take a prop closeOnOutsideClick or something like that, which implements this logic inside Modal. This should probably default to false to not change the behavior of other modal uses.

[owi92]

Weird. There are two things is a thing I'm rather puzzeled by and failed to debug.
- The Modal component already has some logic which I think is meant to close it on outside clicks. The onClick is on the outer div, which is providing the darkened background. But it somehow only triggers when clicked on the inner div that contains the modal itself. OK just figured this out... it's because of the FocusTrap I added a couple of months ago, that needs to wrap around the background as well... meaning I wasted some time by implementing another solution 🤦 This also means that the ConfirmationModals used to close on outside clicks as well, so maybe we should keep or rather revert to that behaviour? Though I still wonder about the second thing:

• The Modal component also has some logic to close it when pressing Escape. This works on the ConfirmationModals, but not on the QR modal. It doesn't even log the event when pressing Escape. Any other keys do... I don't get it.
  Actually it seems that the whole share menu won't close on pressing Escape, unlike all the other Floating menus.

TLDR: There is some issue I haven't figured out but don't spend too much if any time debugging (I already did). Marking this as a draft for now.

[LukasKalbertodt]

maybe we should keep or rather revert to that behaviour?

After testing it out, I think I like that it doesn't close on outside click for these confirmation modals. So I think the PR as is is good.

I investigated the escape thing. The problem is: the share menu does its own "open" state management and just passes open={state !== "closed"} to the floating. And since the floating cannot affect the state in ShareButton, nothing happens.
Since I already looked at it locally, I also implemented a fix: I added another prop to FloatingContainer: onClose. A function that is called when the floating container determines it wants to close. Will push in a sec.

[owi92]

Cool, thank you. I also narrowed it down to the "open" prop and made a fix (removing the "closed" state and using the trigger="click" prop for the FloatingContainer instead, which results in the same outcome as your change.

Unfortunately this came with another issue that is also present in your fix: Using escape to close the QR modal also closes the share menu, which I'd really prefer to stay open. Floating UI seems to do it's own thing with the escape key and I'm out of ideas on how to fix that.
Would you say this behaviour is ok?

[LukasKalbertodt]

Cool, thank you. I also narrowed it down to the "open" prop and made a fix (removing the "closed" state and using the trigger="click" prop for the FloatingContainer instead, which results in the same outcome as your change.

Oh, I hope we didn't work in parallel all the time so that I wasted your work? But yeah, your approach is totally valid as well. I do think though that the "having the state management outside" use case will be inevitable in the future, so the onClose prop is still useful? Or do you prefer your solution?

Unfortunately this came with another issue that is also present in your fix: Using escape to close the QR modal also closes the share menu, which I'd really prefer to stay open. Floating UI seems to do it's own thing with the escape key and I'm out of ideas on how to fix that.
Would you say this behaviour is ok?

Yeah, I noticed this too. Honestly, I think it's fine. I also have no idea how to fix that and I don't think investing time is terribly useful as it's such a minor thing.
Further, it might even be useful in some cases where people just want to scan the QR code and then close both things.

[owi92]

I do think though that the "having the state management outside" use case will be inevitable in the future, so the onClose prop is still useful?

Yeah I definitely agree, so let's merge this.
Also, I didn't really use that much time on the fix itself but rather the debugging and reading FloatingUI docs. So I wouldn't consider it wasted time.