emilkowalski / vaul

A drawer component for React.
https://vaul.emilkowal.ski
MIT License
6.45k stars 215 forks source link

Controlled drawer with `modal={false}` does not work because of bug in `useControllableState` #492

Open skirsten opened 1 month ago

skirsten commented 1 month ago

This took me a while to trace down, but basically a Drawer that is controlled and has modal false e.g.:

const [open, setOpen] = useState(false);

return (
  <>
    <Drawer open={open} onOpenChange={setOpen} modal={false}>
      {/* ... */}
    </Drawer>
    {/* Button somewhere else outside */}
    <Button onClick={() => setOpen(true)}>Open</Button>
  </>
);

and open is set from somewhere outside, it does not get to this line to hack the pointerEvents:

https://github.com/emilkowalski/vaul/blob/504102918ed53d5037c694b2f3300825b4e2f3fb/src/index.tsx#L187

because if the prop changes it does not get to this line to call the handleChange function here: https://github.com/emilkowalski/vaul/blob/504102918ed53d5037c694b2f3300825b4e2f3fb/src/use-controllable-state.ts#L50

So to conclude, under these circumstances it will not remove the pointerEvents = "none" :cry:

Also I am curious why these awful hacks with the pointerEvents are necessary instead of just setting modal={false} on the DialogPrimitive.Root? Maybe this would break something else?

skirsten commented 1 month ago

Actually, I think the useControllableState works as expected. Its probably not great if the changing of the prop would cause the handleChange to be called (which would probably cause a loop).

So I guess the handleChange function should not contain any code with side effects meaning the current logic would have to be moved to a useEffect :thinking:

Edit: On first look using the upstream modal prop and removing all the hacks seems to work just fine (at least in my limited test case): https://github.com/skirsten/vaul/tree/no-hacks

NicHaley commented 1 month ago

Running into an issue with this as well

xharris commented 1 week ago

Currently I'm doing this as a workaround:

<Drawer.Root
      open={isOpen}
      onOpenChange={(v) => {
        setIsOpen(v)
        document.body.style.pointerEvents = "auto"
      }}
      modal={false}
    >
</Drawer.Root>

It doesn't work reliably and feels like a hack to undo a hack. 🤷

Edit: Nevermind, I just put this in the css instead:

body {
  pointer-events: all !important;
}

Not an ideal solution.