adobe
Modal & ModalOverlay onOpenChange don't fire
Provide a general summary of the issue here
When implementing a dialog, there are three possible spots to implement the callback onOpenChange, on the DialogTrigger, ModalOverlay (if present), and the Modal. But only the one on the DialogTrigger ever fires
🤔 Expected Behavior?
All onOpenChange callbacks can be utilized and will be called
😯 Current Behavior
ModalOverlay and Modal's implementations of onOpenChange never get called
💁 Possible Solution
No response
🔦 Context
No response
🖥️ Steps to Reproduce
https://codesandbox.io/p/sandbox/nifty-buck-jmkkl7
Version
1.2.1
What browsers are you seeing the problem on?
Chrome
If other, please specify.
No response
What operating system are you using?
Mac & Windows
🧢 Your Company/Team
No response
🕷 Tracking Issue
No response
The reason it's happening is because we only use the state from the DialogTrigger, ModalOverlay doesn't use its internal state anymore when it's wrapped by a DialogTrigger https://github.com/adobe/react-spectrum/blob/4a8b24028308831a06a2450e6975b8816406315f/packages/react-aria-components/src/Modal.tsx#L120
Issue is, we can't just merge onOpenChange into the state, we'd actually need to intercept open/close/toggle, and those may not result in an actual state change, so we may fire onOpenChange when we shouldn't. If we wait for the parent to resolve it, then we'll fire the event too late.
We might be able to use a register pattern, but it'd be weird to add that to the hooks as it's an implementation detail of RAC. Maybe we could add it to RAC. That would likely cause some additional renders.
Maybe we can just warn people who try to pass onOpenChange to a component which has given up its state control to a parent?
@snowystinger
I believe the trigger alternative is fine for most cases, as modal and trigger are usually collocated. In other cases, we can still use the controlled API.
That being said, the DX could be vastly improved by warning people and directing them towards using onOpenChange of the trigger.
I've just come across this after having tried for a while to implement some basic modal stacking (kind of like what can be seen here). Not being able to set onOpenChange within the implementation of our Modal component means we have to expose this complexity to all callsites that need stacking, which they then need to choose the right place to add this prop, depending on how the modal is triggered. It would be great to find a solution to this issue that isn't just a warning.
One potential solution for the usecase I've described could be to also expose an DialogTriggerContext that we can provide props to for merging. This way, we could support both mechanisms for opening modals without having to leak any logic to callers.
const ModalStackingProvider = ({ children }: { children: ReactNode }) => {
// ... logic here
return (
<ModalStackingContext.Provider value={value}>
<ModalContext.Provider value={{ onOpenChange: registerNestedModalOpenChange }}>
<DialogTriggerContext.Provider value={{ onOpenChange: registerNestedModalOpenChange }}>
{children}
</DialogTriggerContext.Provider>
</ModalContext.Provider>
</ModalStackingContext.Provider>
)
}
Though this would only be a solution to my usecase. I think it would ideally be best to allow onOpenChange to work as expected, independently of the trigger.
@snowystinger as I understand it, that context (and others ending in StateContext) are only used for reading values from parent components, and not for providing props to be merged into child components. Have I misunderstood that?
Ah, I think maybe I misunderstood your ask. Maybe a more concrete example would help. I'm not quite following what the two dialogs have to do with each other, aside from one is a child of the other.
@snowystinger no worries, I'm happy to share a more detailed example, thought I think we'd be getting off-topic a bit from the main issue since that's just about my particular usecase, so maybe I'll open a discussion on the topic later. Coming back to this issue, basically I'm advocating for onOpenChange to "just work" no matter what component it's been added to.
@snowystinger the discussion I mentioned is here: https://github.com/adobe/react-spectrum/discussions/8500, and I've linked to a minimal repro as well.