radix-ui / primitives

Radix Primitives is an open-source UI component library for building high-quality, accessible design systems and web apps. Maintained by @workos.
https://radix-ui.com/primitives
MIT License
15.82k stars 821 forks source link

[Dialog][Popover] Scrolling issue when Popover inside Dialog #1159

Open hipstersmoothie opened 2 years ago

hipstersmoothie commented 2 years ago

Documentation

Relevant Radix Component(s)

Here in the docs it say to make Overlay a sibling to Content.

CleanShot 2022-02-15 at 11 02 22

This setup doesn't work if you have a scrollable popover in the dialog content. On this line a ref is passed to RemoveScroll but I think that by using just a ref that RemoveScroll doesn't consider the portalled element a part of the react tree.

In my own code I fixed this by putting my Content inside of my Overlay. After this change my scrollable popover could scroll again since RemoveScroll now has access to the react tree.

CleanShot 2022-02-15 at 11 07 04

benoitgrelard commented 2 years ago

Both compositions are valid, for different use cases. In fact we have an example of a scrollable overlay here: https://www.radix-ui.com/docs/primitives/components/dialog#scrollable-overlay

hipstersmoothie commented 2 years ago

It's not overlay that I need scrollable though. I'll come up with a code sandbox to show what I'm talking about

hipstersmoothie commented 2 years ago

Here you can see if you follow all of the docs for both popover and dialog you can't scroll a popover in a dialog. I had to read through radix's source code and dependencies to figure out that I needed to wrap my content in the overlay. While both are valid I think the docs should steer people towards code+structure that will work out of the box without having to understand how the internals are actually working.

https://codesandbox.io/s/twilight-http-veu4gw?file=/App.js

hipstersmoothie commented 2 years ago

And if you're unwilling to change that I think the docs should at least detail why both composition are valid and what the use cases actually are.

benoitgrelard commented 2 years ago

Oh sorry I misunderstood what you meant! That seems like a bug potentially, I'll take a look.

jjenzz commented 2 years ago

Hmm, yep looks like it could be a RemoveScroll shards thing 🙈 we need a ticket to create our own react-tree-aware remove scroll at some stage.

dcastil commented 2 years ago

I'm running into the same issue. I'd like to adopt Radix for the design system I'm working on but we have some custom selects with a scrollable area (implemented as popover) which are rendered inside modals. Would it be possible to disable the scroll-blocking in the dialog components? Or just to enable scroll-blocking of the body since the rest is not scrollable because of the dialog overlay anyway.

dcastil commented 2 years ago

To anyone reading, I just found a workaround. The scroll removal is implemented in the Overlay component.

https://github.com/radix-ui/primitives/blob/6da75e0dbb2d1aebd2dae5a044c595bca39a2347/packages/react/dialog/src/Dialog.tsx#L201-L219

So replacing the Dialog.Overlay with a div does the trick. Just keep in mind that you need to block body scroll and deal with the scrollbar yourself with this workaround.

davidavz commented 2 years ago

Thanks @hipstersmoothie @dcastil ! It seems to fix my #1128 issue 🙏

cdeutsch commented 2 years ago

Ran into this as well, when using non-Radix UI bits inside a Dialog.

Would really be nice to have an option to disable it.

giladv commented 1 year ago

Any news on this? scroll doesn't work on third parties with Radix dialog. it's apparently related to the <Modal.Overlay /> component. removing it solves the issue

CarelessCourage commented 1 year ago

Yeah, this is still an issue for us

Nhollas commented 1 year ago

This is still an issue, any idea on a potential fix?

joaom00 commented 1 year ago

@Nhollas If you are using the Radix select, a possible workaround https://github.com/radix-ui/primitives/issues/2125#issuecomment-1545885362

jjenzz commented 1 year ago

i think this is something to do with the third-party remove-scroll lib that radix uses. for now you can set modal={false} on your Dialog. that might not be ideal for a11y (and will hide the overlay) but users will be able to scroll things for now at least.

gruschis commented 1 year ago

modal={false} is the path I took as well. I had composed the library's components in a custom Dialog component. Here's a simple suggestion to substitute the component's overlay (creatively named Overlay2). Even though this is far from pretty, it “works well enough”. The styles applied to Overlay2 are identical to what I had applied to radix' Overlay.

// Above code removed for brevity

export default ({
  children,
  description,
  hasCloseButton = true,
  title,
  trigger,
  ...props
}: ComponentProps) => {
  const [open, setOpen] = useState(false)

  useEffect(() => {
    const body = document.querySelector('body')
    if (!body) return

    if (open) {
      body.style.overflow = 'hidden'
    } else {
      body.style.overflow = ''
    }
  }, [open])

  return (
    <Dialog.Root {...props} open={open} onOpenChange={setOpen}>
      <Dialog.Trigger asChild>{trigger}</Dialog.Trigger>
      {open && <StyledDialog.Overlay2 data-state={open ? 'open' : 'closed'} />}
      <StyledDialog.Content>
        {title && <StyledDialog.Title>{title}</StyledDialog.Title>}
        {description && (
          <StyledDialog.Description>{description}</StyledDialog.Description>
        )}
        {children}
        {hasCloseButton && (
          <StyledDialog.Close asChild>
            <Button shape="square" variant="flat">
              <XMarkIcon className="icon" />
            </Button>
          </StyledDialog.Close>
        )}
      </StyledDialog.Content>
    </Dialog.Root>
  )
}
// Using emotion

export const Overlay2 = styled.div({
  '&[data-state="closed"]': {
    animation: `${FadeFromTo({ from: 0.75, to: 0 })} 200ms ease-in`,
  },
  '&[data-state="open"]': {
    animation: `${FadeFromTo({ from: 0, to: 0.75 })} 300ms ease-out`,
  },
  backgroundColor: theme.color.gray['500'],
  inset: 0,
  opacity: 0.75,
  position: 'fixed',
  zIndex: 10002,
})
deivuss331 commented 1 year ago

Another possible workaround is to render popover content directly inside dialog content and not in portal. So instead of:

<Popover.Portal>
    <Popover.Content />
</Popover.Portal>

you can do:

<Popover.Content />

For sure it doesn't cover all cases because some popovers have to be rendered in portal, but it has covered my case and works well. This solution doesn't force you to set modal={false} on <Dialog.Root />

aslaker commented 1 year ago

To anyone reading, I just found a workaround. The scroll removal is implemented in the Overlay component.

https://github.com/radix-ui/primitives/blob/6da75e0dbb2d1aebd2dae5a044c595bca39a2347/packages/react/dialog/src/Dialog.tsx#L201-L219

So replacing the Dialog.Overlay with a div does the trick. Just keep in mind that you need to block body scroll and deal with the scrollbar yourself with this workaround.

This was the best solution for me for now as it doesn't remove the overlay. It also fixed an issue where padding was getting added to the body of my application whenever the overlay was showing.

Thanks @dcastil

illodev commented 1 year ago

Best Solution ( by @DophinL )

An improvement to this implementation is to add stopPropagation to the onWheel event in the PopoverContent component, like so:

onWheel={(e) => {
    e.stopPropagation();
}}

This prevents the scroll event from propagating to the modal, allowing users to scroll within the popover content without affecting the modal background.

Code examples

Radix

const ExampleOfAnyPopoverInsideModal = () => {
    return (
        <div>
            <Popover.Root>
                <Popover.Trigger />
                <Popover.Anchor />
                <Popover.Portal>
                    <Popover.Content 
                        // Added onWheel event
                        onWheel={(e) => {
                            e.stopPropagation();
                        }}
                    >
                        <Popover.Close />
                        <Popover.Arrow />
                    </Popover.Content>
                </Popover.Portal>
            </Popover.Root>
        </div>
    );
};

Shadcn:

const PopoverContent = React.forwardRef<
    React.ElementRef<typeof PopoverPrimitive.Content>,
    React.ComponentPropsWithoutRef<typeof PopoverPrimitive.Content>
>(
    (
        { className, align = "center", sideOffset = 4, ...props },
        ref
    ) => (
        <PopoverPrimitive.Portal>
            <PopoverPrimitive.Content
                ref={ref}
                align={align}
                // Added onWheel event
                onWheel={(e) => {
                    e.stopPropagation();
                }}
                sideOffset={sideOffset}
                className={cn(
                    "z-50 w-72 rounded-lg border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2",
                    className
                )}
                {...props}
            />
        </PopoverPrimitive.Portal>
    )
);
// ...

Possible Solution (old)

I think it's possible to use the reference of a wrapper or another one that is inside the modal based on this:

imagen imagen

Radix

const ExampleOfAnyPopoverInsideModal = () => {
    const containerRef = useRef<HTMLDivElement>(null);

    return (
        <div ref={containerRef}>
            <Popover.Root>
                <Popover.Trigger />
                <Popover.Anchor />
                <Popover.Portal>
                    <Popover.Content container={containerRef.current}>
                        <Popover.Close />
                        <Popover.Arrow />
                    </Popover.Content>
                </Popover.Portal>
            </Popover.Root>
        </div>
    );
};

Shadcn:


const ExampleOfAnyPopoverInsideModal = () => {
    const containerRef = useRef<HTMLDivElement>(null);
    const [open, setOpen] = useState(false);

    return (
        <div ref={containerRef}>
            <Popover open={open} onOpenChange={setOpen}>
                <PopoverTrigger asChild>
                    <Button
                        variant="outline"
                        role="combobox"
                        aria-expanded={open}
                        className="w-full justify-between"
                    >
                        <span className="truncate">Something</span>
                        <CaretSortIcon className="ml-2 h-4 w-4 shrink-0 opacity-50" />
                    </Button>
                </PopoverTrigger>
                <PopoverContent
                    className="p-0"
                    style={{
                        width: containerRef.current?.offsetWidth
                    }}
                    container={containerRef.current}
                >
                    <Command>
                        //Command with scroll inside :)
                    </Command>
                </PopoverContent>
          </Popover>
      </div>
    );
};
// ...
// Added container prop to PopoverContent
const PopoverContent = React.forwardRef<
    React.ElementRef<typeof PopoverPrimitive.Content>,
    React.ComponentPropsWithoutRef<typeof PopoverPrimitive.Content> & {
        container?: HTMLElement | null;
    }
>(
    (
        { className, container, align = "center", sideOffset = 4, ...props },
        ref
    ) => (
        <PopoverPrimitive.Portal container={container}>
            <PopoverPrimitive.Content
                ref={ref}
                align={align}
                sideOffset={sideOffset}
                className={cn(
                    "z-50 w-72 rounded-lg border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2",
                    className
                )}
                {...props}
            />
        </PopoverPrimitive.Portal>
    )
);
// ...
cssinate commented 10 months ago

A handy tip to anyone implementing the solution from @illodev - if this will seem like devs have to pass the ref as props all the way through a bunch of components, it may help to store the ref in a Context on the styled Dialog component for your popover component to read.

davidmr163 commented 9 months ago

This helped me a lot! @illodev

ozguruysal commented 8 months ago

I think solution suggested by @illodev defeats the purpose of portal, it's really no different than just removing the portal, and that's easier.

benoitgrelard commented 8 months ago

@ozguruysal that's correct.

swingthrough commented 8 months ago

The solution by @illodev also does not work if your Dialog.Content has overflow-auto set - since the div with the containerRef is rendered inside of it, the popover won't be rendered outside of the dialog.

zeroliu commented 7 months ago

Worth mentioning that this also happens to other components that renders in portal. i.e When rendering a react-select with menuPortalTarget in the radix dialog.

kieranm commented 7 months ago

@benoitgrelard Is it possible to get a review on this PR? It sounds like it would solve this issue and it's been open for quite a while now so it would be good to get it merged. Thanks! https://github.com/radix-ui/primitives/pull/2250

rmnoon commented 6 months ago

also just ran into this as well!

vasyaqwe commented 6 months ago

facing this issue.. any other workarounds, other than removing portal from popover and DialogPrimitive.Overlay ? I would like to keep those.

sergical commented 6 months ago

Possible Solution

I think it's possible to use the reference of a wrapper or another one that is inside the modal based on this:

imagen imagen

Radix

const ExampleOfAnyPopoverInsideModal = () => {
    const containerRef = useRef<HTMLDivElement>(null);

    return (
        <div ref={containerRef}>
            <Popover.Root>
                <Popover.Trigger />
                <Popover.Anchor />
                <Popover.Portal>
                    <Popover.Content container={containerRef.current}>
                        <Popover.Close />
                        <Popover.Arrow />
                    </Popover.Content>
                </Popover.Portal>
            </Popover.Root>
        </div>
    );
};

Shadcn:

const ExampleOfAnyPopoverInsideModal = () => {
    const containerRef = useRef<HTMLDivElement>(null);
    const [open, setOpen] = useState(false);

    return (
        <div ref={containerRef}>
            <Popover open={open} onOpenChange={setOpen}>
                <PopoverTrigger asChild>
                    <Button
                        variant="outline"
                        role="combobox"
                        aria-expanded={open}
                        className="w-full justify-between"
                    >
                        <span className="truncate">Something</span>
                        <CaretSortIcon className="ml-2 h-4 w-4 shrink-0 opacity-50" />
                    </Button>
                </PopoverTrigger>
                <PopoverContent
                    className="p-0"
                    style={{
                        width: containerRef.current?.offsetWidth
                    }}
                    container={containerRef.current}
                >
                    <Command>
                        //Command with scroll inside :)
                    </Command>
                </PopoverContent>
          </Popover>
      </div>
    );
};
// ...
// Added container prop to PopoverContent
const PopoverContent = React.forwardRef<
    React.ElementRef<typeof PopoverPrimitive.Content>,
    React.ComponentPropsWithoutRef<typeof PopoverPrimitive.Content> & {
        container?: HTMLElement | null;
    }
>(
    (
        { className, container, align = "center", sideOffset = 4, ...props },
        ref
    ) => (
        <PopoverPrimitive.Portal container={container}>
            <PopoverPrimitive.Content
                ref={ref}
                align={align}
                sideOffset={sideOffset}
                className={cn(
                    "z-50 w-72 rounded-lg border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2",
                    className
                )}
                {...props}
            />
        </PopoverPrimitive.Portal>
    )
);
// ...

ILY <3

ansh-les commented 5 months ago

Facing the same issue.

mochetts commented 5 months ago

Same thing here... @illodev doesn't entirely solve the issue:

Screenshot 2024-05-23 at 3 02 21 PM

See the popover content being cut off by the modal's boundaries.

So I either lose scroll or get the popover completely cut off.

EDIT: This is answer is what ultimately did the trick for me: https://github.com/radix-ui/primitives/issues/1159#issuecomment-1105320294

I'm a shadcn user, so I basically replaced the DialogOverlay instance in the DialogContent component with a <div /> with the same styles as the DialogOverlay.

https://github.com/radix-ui/primitives/assets/3678598/24b25cfc-f9b4-4096-9eb2-ecaf80957ace

CarlosVergikosk commented 5 months ago

Possible Solution

I think it's possible to use the reference of a wrapper or another one that is inside the modal based on this: imagen imagen

Radix

const ExampleOfAnyPopoverInsideModal = () => {
    const containerRef = useRef<HTMLDivElement>(null);

    return (
        <div ref={containerRef}>
            <Popover.Root>
                <Popover.Trigger />
                <Popover.Anchor />
                <Popover.Portal>
                    <Popover.Content container={containerRef.current}>
                        <Popover.Close />
                        <Popover.Arrow />
                    </Popover.Content>
                </Popover.Portal>
            </Popover.Root>
        </div>
    );
};

Shadcn:

const ExampleOfAnyPopoverInsideModal = () => {
    const containerRef = useRef<HTMLDivElement>(null);
    const [open, setOpen] = useState(false);

    return (
        <div ref={containerRef}>
            <Popover open={open} onOpenChange={setOpen}>
                <PopoverTrigger asChild>
                    <Button
                        variant="outline"
                        role="combobox"
                        aria-expanded={open}
                        className="w-full justify-between"
                    >
                        <span className="truncate">Something</span>
                        <CaretSortIcon className="ml-2 h-4 w-4 shrink-0 opacity-50" />
                    </Button>
                </PopoverTrigger>
                <PopoverContent
                    className="p-0"
                    style={{
                        width: containerRef.current?.offsetWidth
                    }}
                    container={containerRef.current}
                >
                    <Command>
                        //Command with scroll inside :)
                    </Command>
                </PopoverContent>
          </Popover>
      </div>
    );
};
// ...
// Added container prop to PopoverContent
const PopoverContent = React.forwardRef<
    React.ElementRef<typeof PopoverPrimitive.Content>,
    React.ComponentPropsWithoutRef<typeof PopoverPrimitive.Content> & {
        container?: HTMLElement | null;
    }
>(
    (
        { className, container, align = "center", sideOffset = 4, ...props },
        ref
    ) => (
        <PopoverPrimitive.Portal container={container}>
            <PopoverPrimitive.Content
                ref={ref}
                align={align}
                sideOffset={sideOffset}
                className={cn(
                    "z-50 w-72 rounded-lg border bg-popover p-4 text-popover-foreground shadow-md outline-none data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2",
                    className
                )}
                {...props}
            />
        </PopoverPrimitive.Portal>
    )
);
// ...

ILY <3

Works like a charm, wp

DophinL commented 5 months ago

You can try avoid event propagation on popover.

Use NextUI select as an example:

<Select
      popoverProps={{
        onWheel: (e) => {
          e.stopPropagation()
        }
      }}
      >
      {topics.map((item) => (
        <SelectItem key={item.value} value={item.value} aria-label={item.label}>
          {item.label}
        </SelectItem>
      ))}
    </Select>
gbrunacci commented 5 months ago

You can try avoid event propagation on popover.

Use NextUI select as an example:

<Select
      popoverProps={{
        onWheel: (e) => {
          e.stopPropagation()
        }
      }}
      >
      {topics.map((item) => (
        <SelectItem key={item.value} value={item.value} aria-label={item.label}>
          {item.label}
        </SelectItem>
      ))}
    </Select>

This one actually solved my issue without removing overlay or setting modal={false}

xspaceboy commented 5 months ago

illodev

Hi, illodev. This solution works for me and thank you very much.

zaleGZL commented 5 months ago

@illodev So good~

sergey-shch commented 5 months ago

You can try avoid event propagation on popover.

@DophinL the best solution! I also added the same to onTouchMove event and that's it, overlay is saved

eliuAtFanatics commented 4 months ago

I've tried all of the solutions and none of them have worked entirely for me, as I'm experiencing a flickering when hovering over a popover trigger with an SVG icon. I tried all the solutions and nothing exactly works. Wondering if anyone else has any other solutions for this.

akshay-nm commented 4 months ago

Could you maybe add a prop on Overlay component, making the RemoveScroll part optional? That way devs won't have to come up with an overlay component of their own.

Setting modal={false} just removes the overlay I think.

jtoomay commented 4 months ago

Documentation

Relevant Radix Component(s)

Here in the docs it say to make Overlay a sibling to Content.

CleanShot 2022-02-15 at 11 02 22

This setup doesn't work if you have a scrollable popover in the dialog content. On this line a ref is passed to RemoveScroll but I think that by using just a ref that RemoveScroll doesn't consider the portalled element a part of the react tree.

In my own code I fixed this by putting my Content inside of my Overlay. After this change my scrollable popover could scroll again since RemoveScroll now has access to the react tree.

CleanShot 2022-02-15 at 11 07 04

THANK YOU SO MUCH. I WAS MESSING WITH THIS FOREVER!!!

bhatvikrant commented 2 months ago

You can try avoid event propagation on popover.

Use NextUI select as an example:

<Select
      popoverProps={{
        onWheel: (e) => {
          e.stopPropagation()
        }
      }}
      >
      {topics.map((item) => (
        <SelectItem key={item.value} value={item.value} aria-label={item.label}>
          {item.label}
        </SelectItem>
      ))}
    </Select>

This worked for me! Thanks

Ryanv030 commented 1 month ago

You can try avoid event propagation on popover.

Use NextUI select as an example:

<Select
      popoverProps={{
        onWheel: (e) => {
          e.stopPropagation()
        }
      }}
      >
      {topics.map((item) => (
        <SelectItem key={item.value} value={item.value} aria-label={item.label}>
          {item.label}
        </SelectItem>
      ))}
    </Select>

You're a legend, thanks for this!

zachlankton commented 1 month ago

You can try avoid event propagation on popover.

Use NextUI select as an example:

<Select
      popoverProps={{
        onWheel: (e) => {
          e.stopPropagation()
        }
      }}
      >
      {topics.map((item) => (
        <SelectItem key={item.value} value={item.value} aria-label={item.label}>
          {item.label}
        </SelectItem>
      ))}
    </Select>

Not sure if this will turn out to be good or bad yet, but this is working for me using ShadCn PopOver

image
niuweili commented 1 month ago

您可以尝试避免弹出窗口上的事件传播。

使用 NextUI select 为例:

<Select
      popoverProps={{
        onWheel: (e) => {
          e.stopPropagation()
        }
      }}
      >
      {topics.map((item) => (
        <SelectItem key={item.value} value={item.value} aria-label={item.label}>
          {item.label}
        </SelectItem>
      ))}
    </Select>

This is the best solution I've ever seen

Lexachoc commented 1 week ago

You can try avoid event propagation on popover. Use NextUI select as an example:

<Select
      popoverProps={{
        onWheel: (e) => {
          e.stopPropagation()
        }
      }}
      >
      {topics.map((item) => (
        <SelectItem key={item.value} value={item.value} aria-label={item.label}>
          {item.label}
        </SelectItem>
      ))}
    </Select>

Not sure if this will turn out to be good or bad yet, but this is working for me using ShadCn PopOver image

Thank you very much! It works and the issue that the Shift key cannot be used for horizontal scrolling has been resolved. See : https://github.com/radix-ui/primitives/issues/3009 and https://github.com/shadcn-ui/ui/issues/5627

dBianchii commented 3 days ago

Adding this to PopoverContent fully worked for me! Thx!

onWheel={(e) => {
          e.stopPropagation(); 
        }}
<PopoverContent
        className="w-auto p-0"
        side={side}
        onWheel={(e) => {
          e.stopPropagation(); 
        }}
      >
          <div className="flex flex-col divide-y sm:h-[300px] sm:flex-row sm:divide-x sm:divide-y-0">
            <ScrollArea className="w-64 sm:w-auto">
              <ScrollBar orientation="horizontal" className="sm:hidden" />
            </ScrollArea>
            <ScrollArea className="w-64 sm:w-auto">
              <ScrollBar orientation="horizontal" className="sm:hidden" />
            </ScrollArea>
          </div>
        </div>
      </PopoverContent>