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

Dialog's open state is not fully controllable #3402

Open
Dremora opened this issue Jan 29, 2024 · 4 comments
Open

Dialog's open state is not fully controllable #3402

Dremora opened this issue Jan 29, 2024 · 4 comments

Comments

@Dremora
Copy link
Sponsor

Dremora commented Jan 29, 2024

Current behavior

When using useDialogStore with open and setOpen properties defined, there's a glitch happening. When Ariakit calls setOpen, it assumes that open will be updated accordingly to the value passed as an argument. While the final value is what I expect it to be, it will be briefly incorrect. Best understood with example below.

Steps to reproduce the bug

  1. Open https://stackblitz.com/edit/ds7plf?file=dialog-nested%2Findex.tsx&theme=dark
  2. Click "View Cart"
  3. Click outside the dialog
  4. Watch the dialog disappear for one frame. Also, check out the console - the open state will flicker from true to false to true.

Expected behavior

Since I'm controlling the value, I never expect it to be out of sync.

It also makes sense to explain why I'm doing this. I want to display a confirmation dialog (a nested modal) when a user tries to close the dialog, including on click outside.

Workaround

It's possible that onClose event on the modal will solve my problem in a different way (I haven't tried it yet, and I'm aware that it doesn't get called in all scenarios).

#3402 (comment)

Possible solutions

No response

@diegohaz
Copy link
Member

I've seen this behavior before, but I'm not sure if there's an action we can take here given how React works.

The current solution is to prevent the onClose event:

<Dialog onClose={(event) => event.preventDefault()}>

@Dremora
Copy link
Sponsor Author

Dremora commented Jan 31, 2024

I've seen this behavior before, but I'm not sure if there's an action we can take here given how React works.

I haven't seen this in React. For example, if you have a controlled input <input type="text" value="foo" onChange={() => {}}/>, it doesn't matter what the user tries to type, the value will always be foo, without temporary glitches.

@diegohaz
Copy link
Member

I've seen this behavior before, but I'm not sure if there's an action we can take here given how React works.

I haven't seen this in React. For example, if you have a controlled input <input type="text" value="foo" onChange={() => {}}/>, it doesn't matter what the user tries to type, the value will always be foo, without temporary glitches.

Yes, React does some magic to manage controlled inputs, but this isn't how native elements typically work. For instance, you can't fully control a native dialog element using the open and onClose props: https://stackblitz.com/edit/stackblitz-starters-gwwkum?devToolsHeight=33&file=src%2FApp.tsx

We're not using the native dialog element, and I agree we should do something to avoid these glitches. However, when I say "how React works," I'm referring to the fact that controlled behavior isn't the norm in React. It's a special approach that React uses for inputs.

@Dremora
Copy link
Sponsor Author

Dremora commented Feb 7, 2024

I agree that turning native elements into controlled ones is a hack on React's side, since HTML elements are not designed to be controlled in the first place. The fact that details and dialog don't work as controlled elements can probably be considered a bug in React (there's an issue open at least about details).

When it comes to making custom controlled components, the solution typically lies in abandoning internal state in favour of external one, I would consider this approach clean, and it doesn't cause glitches. Whether it's trivial to do so for a given component, of course, depends on component's implementation, e.g. when implementation requires to have internal state and thus synchronising it with the external one (and I assume this might be happening in case of <Dialog>).

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

No branches or pull requests

2 participants