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][Popover] Scrolling issue when Popover inside Dialog #1159

Open
hipstersmoothie opened this issue Feb 15, 2022 · 40 comments · May be fixed by #2250
Open

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

hipstersmoothie opened this issue Feb 15, 2022 · 40 comments · May be fixed by #2250

Comments

@hipstersmoothie
Copy link

hipstersmoothie commented Feb 15, 2022

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

hipstersmoothie commented Feb 15, 2022

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
Copy link
Author

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
Copy link
Collaborator

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

@jjenzz
Copy link
Contributor

jjenzz commented Feb 15, 2022

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.

AaronPlave added a commit to nasa-jpl/react-stellar that referenced this issue Feb 16, 2022
@benoitgrelard benoitgrelard added the Type: Bug Confirmed bug label Mar 29, 2022
@benoitgrelard benoitgrelard changed the title Change Dialog.Overlay to wrap Content in examples [Dialog][Popover] Scrolling issue when Popover inside Dialog Mar 29, 2022
@benoitgrelard benoitgrelard self-assigned this Apr 5, 2022
@dcastil
Copy link

dcastil commented Apr 21, 2022

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
Copy link

dcastil commented Apr 21, 2022

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

const DialogOverlayImpl = React.forwardRef<DialogOverlayImplElement, DialogOverlayImplProps>(
(props: ScopedProps<DialogOverlayImplProps>, forwardedRef) => {
const { __scopeDialog, ...overlayProps } = props;
const context = useDialogContext(OVERLAY_NAME, __scopeDialog);
return (
// Make sure `Content` is scrollable even when it doesn't live inside `RemoveScroll`
// ie. when `Overlay` and `Content` are siblings
<RemoveScroll as={Slot} allowPinchZoom={context.allowPinchZoom} shards={[context.contentRef]}>
<Primitive.div
data-state={getState(context.open)}
{...overlayProps}
ref={forwardedRef}
// We re-enable pointer-events prevented by `Dialog.Content` to allow scrolling the overlay.
style={{ pointerEvents: 'auto', ...overlayProps.style }}
/>
</RemoveScroll>
);
}
);

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
Copy link

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

@cdeutsch
Copy link

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
Copy link

giladv commented Apr 30, 2023

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
Copy link

CarelessCourage commented May 9, 2023

Yeah, this is still an issue for us

@aslaker
Copy link

aslaker commented Sep 21, 2023

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

const DialogOverlayImpl = React.forwardRef<DialogOverlayImplElement, DialogOverlayImplProps>(
(props: ScopedProps<DialogOverlayImplProps>, forwardedRef) => {
const { __scopeDialog, ...overlayProps } = props;
const context = useDialogContext(OVERLAY_NAME, __scopeDialog);
return (
// Make sure `Content` is scrollable even when it doesn't live inside `RemoveScroll`
// ie. when `Overlay` and `Content` are siblings
<RemoveScroll as={Slot} allowPinchZoom={context.allowPinchZoom} shards={[context.contentRef]}>
<Primitive.div
data-state={getState(context.open)}
{...overlayProps}
ref={forwardedRef}
// We re-enable pointer-events prevented by `Dialog.Content` to allow scrolling the overlay.
style={{ pointerEvents: 'auto', ...overlayProps.style }}
/>
</RemoveScroll>
);
}
);

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
Copy link

illodev commented Sep 29, 2023

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>
    )
);
// ...

@cssinate
Copy link

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
Copy link

This helped me a lot! @illodev

@ozguruysal
Copy link

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
Copy link
Collaborator

@ozguruysal that's correct.

@swingthrough
Copy link

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
Copy link

zeroliu commented Mar 25, 2024

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
Copy link

kieranm commented Mar 25, 2024

@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!
#2250

@rmnoon
Copy link

rmnoon commented Apr 24, 2024

also just ran into this as well!

@vasyaqwe
Copy link

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

@sergical
Copy link

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
Copy link

Facing the same issue.

@mochetts
Copy link

mochetts commented May 23, 2024

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:
#1159 (comment)

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.

Screen.Recording.2024-05-23.at.3.15.49.PM.mov

@CarlosVergikosk
Copy link

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
Copy link

DophinL commented May 27, 2024

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
Copy link

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
Copy link

illodev

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

@zaleGZL
Copy link

zaleGZL commented Jun 4, 2024

@illodev
So good~

@sergey-shch
Copy link

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
Copy link

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
Copy link

akshay-nm commented Jul 2, 2024

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
Copy link

jtoomay commented Jul 6, 2024

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!!!

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