Modal & ModalOverlay onOpenChange don't fire

[ArrayKnight]

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

[snowystinger]

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

react-spectrum/packages/react-aria-components/src/Modal.tsx

Line 120 in 4a8b240

let state = props.isOpen != null || props.defaultOpen != null || !contextState ? localState : contextState;

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?

[yaoweiprc]

Same for me

[snowystinger]

Are you able to use it on the trigger instead?

[rodogir]

@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.