react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

[RAC] Using `Dialog` with 3rd parties that portal into `document.body`

Open amitdahan opened this issue 1 year ago โ€ข 1 comments
trafficstars

Provide a general summary of the issue here

We're trying to use RAC's Modal and Dialog to replace our current implementation of a Modal and Drawer components.

But if we for-example render a 3rd-party that portals into document.body (in our case - for a Select component that's rendered within the Dialog) - clicking on anything within the portal also dismisses the Dialog (because we're also using isDissmisable).

๐Ÿค” Expected Behavior?

Either this should simply not happen, or we should have a way to allowlist certain things from being considered "click outside" (exactly like `shouldCloseOnInteractOutside ๐Ÿ˜…)

๐Ÿ˜ฏ Current Behavior

Everything that's outside the Dialog will dismiss it, including things that are technically within it, but are portaled into document.body.

(Unless - they are known by RAC, in which case they are special-cased?)

๐Ÿ’ Possible Solution

It seems the implementation already allows for passing shouldCloseOnInteractOutside through either ModalOverlay (if used) or through ModalContext.Provider.

I propose:

  • Supporting this in types (currently types prevent this)
  • Also supporting this in the case where a default ModalOverlay is used, so directly passing this to Modal

๐Ÿ”ฆ Context

This is actually only needed when mixing RAC and other third-parties, but in our current real-world case - we are trying to gradually introduce RAC through our component library.

๐Ÿ–ฅ๏ธ Steps to Reproduce

Repro: https://codesandbox.io/p/devbox/vcmc42

Issue is shown in the 3rd dropdown in the first dialog: image

Proposed solution is demonstrated in the 2nd dialog: image

Version

[email protected]

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

MacOS 14.3

๐Ÿงข Your Company/Team

No response

๐Ÿ•ท Tracking Issue

No response

amitdahan avatar Feb 21 '24 07:02 amitdahan

It would be better if you could portal to the Dialog instead, or just remove the target of 'body' as you do in the working example in the first popover.

My reasoning is that I think there is another problem lurking here, which is a user using VoiceOver or another assistive technology may not be able to access the select's popover because we watch the dom and aria-hide everything outside of the dialog.

I did test your 3rd party Select, and it does appear to work because it uses virtual focus management. However, this will not always be the case, so we'll need to be careful exposing this feature.

snowystinger avatar Feb 21 '24 22:02 snowystinger

I'm trying to gradually include react-aria-components into an existing component library and there are many implementations containing potentially many different 3rd party components that leverage portaling. It's not really viable for me to go into each and every implementation and disable 3rd party components' portaling or target the dialog that they may or may not exist within. Is there not something I can do to check that the click/press event starts ON the ModalOverlay?

kylemh avatar Feb 27 '24 08:02 kylemh

@amitdahan instead of approvelisting specific other portals, we could adjust the ModalProvider like so:

<ModalContext.Provider
  value={{
    shouldCloseOnInteractOutside: (target) => {
      return !!target.closest(".react-aria-ModalOverlay");
    },
  }}
>

thereby only closing the modal if the modal's overlay was interacted with.

I'm unsure of the accessibility implications here, but I've never had this portaling issue with Reach UI's or Radix UI's Dialog implementations, so I'm not really sure what's so different here.

kylemh avatar Feb 27 '24 10:02 kylemh

thereby only closing the modal if the modal's overlay was interacted with.

Interesting alternative!

I'm currently trying to make our own use-case - not use a portal, but in general I agree with your point that with some third-parties this may or may not be possible, or may come with other trade-offs.

I agree with preferring not to portal from within a dialog/popover, but since shouldCloseOnInteractOutside was already introduced in Popover, I think it'd make sense to also be allowed in Modal/ModalOverlay.

amitdahan avatar Feb 28 '24 06:02 amitdahan

Linking a related PR my coworker reminded me of Update useModalOverlay types

If anyone wants to take it over it could be used to implement the ask to expose shouldCloseOnInteractOutside in Modal/ModalOverlay

snowystinger avatar Feb 29 '24 00:02 snowystinger