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.85k stars 821 forks source link

Select causing layout shift in external monitor. #1925

Closed ghost closed 1 year ago

ghost commented 1 year ago

Bug report

Current Behavior

Select add --removed-body-scroll-bar-size: 15px on 24inch secondary monitor.
But when I visit on my 13inch, it's --removed-body-scroll-bar-size: 0px. The same value gets added to the margin-right property of body and it's causing a layout shift.

Suggested solution

By removing

body {
   margin-right: 15px !important;
}


Software Name(s) Version
Radix Package(s) Select
React n/a
Browser
Assistive tech
Node n/a
npm/yarn
Operating System MacOS
ghost commented 1 year ago

I see the same behavior in the example from website https://www.radix-ui.com/docs/primitives/components/select

kiastorm commented 1 year ago

I'm getting this behaviour too when using the DropdownMenu component

fveracoechea commented 1 year ago

Same issue here with DropdownMenu, it adds some CSS that makes the whole page move, but only seems to happen on large screen sizes:

image

luizfm commented 1 year ago

I'm getting this behaviour too when using the DropdownMenu component

In my case @kiastorm, I was facing the same issue on the DropdownMenu component, but I fixed that by removing a custom css prop I've added at html, body tags. which was overflow-x: hidden

Screenshot 2023-03-07 at 15 20 14

After that, it worked again!

thewinger commented 1 year ago

I'm seeing the same behavior, any work around? My body doesn't habe any classes. I have tried using position popper and items-aligned and same issue always

CleanShot 2023-04-01 at 10 57 02

Update: I had to add a (margin-right: 0 !important) to the body and seems to work.

akshay-nm commented 1 year ago

Why are you adding a stylesheet with marginRight:19px !important? image image

None of the above mentioned fix work for me.

zuzabrzozowska commented 1 year ago

Worked for me adding margin: 0 !important; on the body! Also worked with overflow: hidden !important. But why does it add this margin-right: 15px to the body in the first place? 🤔

brenoprata10 commented 1 year ago

I am also having the same issue 😿

TheHanna commented 1 year ago

Any update on this? I'm experiencing this issue with RadixOverlay in React

joshdavenport commented 1 year ago

With 100% width position: fixed elements (e.g. a fixed header) the margin applied to body by react-remove-scroll causes a visual shift in that element as the page margin doesn't affect the fixed element. That causes a very obvious shift when the select is open and radix disables scroll via react-remove-scroll.

Thankfully react-remove-scroll adds a CSS var --removed-body-scroll-bar-size that you can use to manually apply the offset that it adds as a margin to the page onto the width of your fixed element. i.e. something like: width: calc(100% - var(--removed-body-scroll-bar-size, 0px)); or w-[calc(100%-var(--removed-body-scroll-bar-size,0px))] for Tailwind users

For reproduction, on Mac at least, it helps to turn set show scroll bars in General preferences to "always":

Screenshot 2023-05-13 at 00 10 12
lukevella commented 1 year ago

This seems to happen to avoid a layout shift on body when the scrollbar is removed by activating a popover. What worked for me is adding:

body {
  overflow-y: scroll;
}

This makes the scrollbar always visible which prevents a layout shift on pages where there is no overflow.

benoitgrelard commented 1 year ago

Can someone provide a sandbox with the issue. I'm not sure what scenario you have, but out of the box things should work correctly as you can see here:

https://github.com/radix-ui/primitives/assets/1539897/26d955bc-a5ab-4545-b831-6b25aff70056

benoitgrelard commented 1 year ago

With 100% width position: fixed elements (e.g. a fixed header) the margin applied to body by react-remove-scroll causes a visual shift in that element as the page margin doesn't affect the fixed element. That causes a very obvious shift when the select is open and radix disables scroll via react-remove-scroll.

Thankfully react-remove-scroll adds a CSS var --removed-body-scroll-bar-size that you can use to manually apply the offset that it adds as a margin to the page onto the width of your fixed element. i.e. something like: width: calc(100% - var(--removed-body-scroll-bar-size, 0px)); or w-[calc(100%-var(--removed-body-scroll-bar-size,0px))] for Tailwind users

For reproduction, on Mac at least, it helps to turn set show scroll bars in General preferences to "always":

Screenshot 2023-05-13 at 00 10 12

For position fixed elements, react-remove-scroll exposes some classes you can use on these elements so things work out of the box: https://github.com/theKashey/react-remove-scroll#positionfixed-elements

lukevella commented 1 year ago

Can someone provide a sandbox with the issue. I'm not sure what scenario you have, but out of the box things should work correctly as you can see here:

CleanShot.2023-05-30.at.16.33.54.mp4

Here you go:

https://codesandbox.io/s/dazzling-oskar-ddkwl7

As I mentioned in my previous comment, the problem for me was that I was expecting overflow on the <html> element rather than the <body>. I believe I was using some components from headlessui which did the same trick radix is doing here (but on <html>) to avoid the layout shift so the issue seems to be that these two don't play well together.

benoitgrelard commented 1 year ago

Thanks @lukevella. Yeah this makes sense, react-remove-scroll operates on body scroll, not html level as this is what where the default overflow is natively when there's too much content on the body.

That doesn't really help me though because this isn't really an issue per-se. (ie. simply don't force overflow on html).

I am interested in a reproduction when used correctly.

lukevella commented 1 year ago

I am interested in a reproduction when used correctly.

I don't believe there is actually any issue with radix-ui's behavior here. I suspect most people having layout shift issues simply have overflow rules on <html> rather than <body>.

benoitgrelard commented 1 year ago

Yeah that's what I'm trying to determine too. But given I cannot reproduce the issue I feel you might be right.

benoitgrelard commented 1 year ago

I am going to close this issue as explained above this is likely a non-issue if using natural native page overflow. If someone has a genuine issue with a reproduction, feel free to post it here and I can re-assess/re-open if necessary.

jenniferhail commented 1 year ago

With 100% width position: fixed elements (e.g. a fixed header) the margin applied to body by react-remove-scroll causes a visual shift in that element as the page margin doesn't affect the fixed element. That causes a very obvious shift when the select is open and radix disables scroll via react-remove-scroll.

Thankfully react-remove-scroll adds a CSS var --removed-body-scroll-bar-size that you can use to manually apply the offset that it adds as a margin to the page onto the width of your fixed element. i.e. something like: width: calc(100% - var(--removed-body-scroll-bar-size, 0px)); or w-[calc(100%-var(--removed-body-scroll-bar-size,0px))] for Tailwind users

For reproduction, on Mac at least, it helps to turn set show scroll bars in General preferences to "always":

Screenshot 2023-05-13 at 00 10 12

This is the solve. It would be great to see the css variable mentioned in the docs for anyone who has a fixed nav or element.

I moved overflow from html to body and then used width: calc(100% - var(--removed-body-scroll-bar-size, 0px)); on my fixed header to fix the shift. This is the best solution because Firefox treats custom scrollbars differently than Chrome & Safari.

stylessh commented 1 year ago

It's not the best approach, but putting the following line will override the inline-style props on the css:

body[style] { margin: 0 !important;}
pahans commented 1 year ago

This bug is caused by a margin, --removed-body-scroll-bar-size added by radix or its dependency(possibly react-remove-scroll). this breaks the page layout inchrome for some cases. how to reproduce.

Add a <div style="position: fixed; width: 100%"> inside body and add a radix select. open the select and fixed element will move as you open-close select component.

I can try to debug it more and open a PR. :)

incpo commented 1 year ago

Hello, I fixed this by adding w-screen (width: 100vw) class to fixed component. I had a fixed header so when I opened the Dialog component, header shifts.

NicoToff commented 1 year ago

Hello everyone,

I am experiencing the same issue when using both Dropdown and Select. Being a Tailwind user, I had to add the "important" property to my tailwind.config.cjs file:

/** @type {import('tailwindcss').Config} */
module.exports = {
  important: true,
  // ...
}

If I understand correctly, this will break inline styles, but I'm not using any, nor do the libs I'm using, so it's all good for me.

More info here: https://tailwindcss.com/docs/configuration#important

edit: This doesn't solve the issue entirely :( It only solves it for some screen sizes... edit2: For now, I've fixed the issue by removing the container mx-auto from my <body>, but that's not an OK solution

stylessh commented 1 year ago

@NicoToff I don't personally recommend you using the important option, since this adds "!important" rule on each tailwindcss style, this might cause numerous issues if you try to add some custom css that can override tailwind. I suggest you disabling the inline margin property that radix adds to the container.

NicoToff commented 1 year ago

@NicoToff I don't personally recommend you using the important option, since this adds "!important" rule on each tailwindcss style, this might cause numerous issues if you try to add some custom css that can override tailwind. I suggest you disabling the inline margin property that radix adds to the container.

Are you talking about this fix you mentioned above?

body[style] { margin: 0 !important;}

Also tried this, but nothing works:

w-[calc(100%-var(--removed-body-scroll-bar-size,0px))]

I get the idea between those approaches, I'm just very confused about this whole problem in the first place :( I'm not a CSS expert by any means, tbh.

NicoToff commented 1 year ago

Alright, I think I get the whole picture, now.

I find this behaviour very intrusive. I'd love to have a way to disable it entirely rather than having to fix it with variable overriding. Removing the scroll bar is simply not a behaviour I want on my UI.

stylessh commented 1 year ago

Unfortunately, there's no way to disable it by setting up a config param, that's why this issue for as well. Would be really good if they'd allow us to disable that behavior

NicoToff commented 1 year ago

My best solution was returning to native HTML <select>. It works great 😂

rahulpeacock commented 1 year ago

We can solve this issue, let me debug what's happening

when you use Radix UI two things happen

  1. overflow: hidden; on the body and
  2. Radix UI will add margin-right: on the body.

Why the margin-right: is being added on the body element?? when there is overflow: hidden; on the body the scrollbar will disappear(width of the scroll bar is zero) and Radix UI will add imaginary scrollbar width in order to prevent layout shift using the margin-right: where the margin-right value will be the width of the scrollbar.

How to Fix this behavior:

  1. First we will add scrollbar-gutter: stable property on the html element(This will have a fixed width for the scrollbar even when the there overflow: hidden is present.) Youtube link to the scrollbar-gutter property and how it works
    html {
    scrollbar-gutter: stable;
    }
  2. Secondly we will add margin-right: 0px !important; inside body[style]
    
    body {
    margin: 0;
    }

body[style] { margin-right: 0px !important; }

rahulpeacock commented 1 year ago

Unfortunately, there's no way to disable it by setting up a config param, that's why this issue for as well. Would be really good if they'd allow us to disable that behavior

No it is possible to disable this behavior Issue link

NicoToff commented 1 year ago

We can solve this issue, let me debug what's happening

when you use Radix UI two things happen

  1. overflow: hidden; on the body and
  2. Radix UI will add margin-right: on the body.

Why the margin-right: is being added on the body element?? when there is overflow: hidden; on the body the scrollbar will disappear(width of the scroll bar is zero) and Radix UI will add imaginary scrollbar width in order to prevent layout shift using the margin-right: where the margin-right value will be the width of the scrollbar.

How to Fix this behavior:

  1. First we will add scrollbar-gutter: stable property on the html element(This will have a fixed width for the scrollbar even when the there overflow: hidden is present.) Youtube link to the scrollbar-gutter property and how it works
html {
    scrollbar-gutter: stable;
}
  1. Secondly we will add margin-right: 0px !important; inside body[style]
body {
    margin: 0;
}

body[style] {
    margin-right: 0px !important;
}

Well, @Rahul-Palamarthi , I think you fixed it for me 😄 The missing piece for my use case was the scrollbar-gutter, apparently. A fine bit of CSS dark magic (thx for the link, btw).

I modified you snippet since I use auto margins with Tailwind (container mx-auto) and everything works great:

html {
  scrollbar-gutter: stable;
}

body[style] {
  margin: 0 auto !important;
}

Thank you so much!

vinit-churi commented 1 year ago

for me just increasing the css specificity to the body selector worked

html body {
  margin-right: 0px !important;
}
DanielSpindler commented 1 year ago

I am interested in a reproduction when used correctly.

I don't believe there is actually any issue with radix-ui's behavior here. I suspect most people having layout shift issues simply have overflow rules on <html> rather than <body>.

the problem is that a simple drop down for example can manipulate the body styles...

NicoToff commented 1 year ago

Hello beautiful people! Long time no see :D Turns out my issue wasn't entirely solved! Not for all screen sizes at the very least.

I unfortunately couldn't reproduce the problem in a simple Vite app, which led me to believe something was wrong with shadcn's components. I checked issues on this master's repo, and I found this: https://github.com/shadcn-ui/ui/issues/977

A gentleperson over there shared this tip:

I had the same issue, removing the class of body to a new div under the body solved the issue.

So my layout file:

    <body class={bodyClass}>
        // ...

became

    <body>
        <div class={bodyClass}>
           // ...

This now entirely solves my issue! ......

Until next time XD

raphaelpg commented 1 year ago

html {
  scrollbar-gutter: stable;
}

body[style] {
  margin: 0 auto !important;
}
```@NicoToff Thanks, this is the only solution that worked for me
Roeefl commented 10 months ago

Can someone please explain why is this closed?

Is this intended, legit behaviour? not a bug?

When toggling the Select to be opened in the radix website, the scroll in the page is not displayed.

is this legit?

rahulpeacock commented 10 months ago

Can someone please explain why is this closed?

Is this intended, legit behaviour? not a bug?

When toggling the Select to be opened in the radix website, the scroll in the page is not displayed.

is this legit?

they are using overflow: hidden on the body or html tag. Hence the scroll will not be displayed

joshdavenport commented 10 months ago

It's broken down here (make sure you're aware of current browser support before going this route, hopefully Safari adds support soon) and here and there are viable solutions in both of those comments for anyone interested. If you actually want the scrollbar to be visible when the select is open then I imagine that's not something that anything in this issue would help with.

Bit of a re-breakdown if it helps anyone, though there is loads of information already here, this is just an attempt:

The issue isn't really about whether the scroll bar is visible or not, it's relating to layout shift caused by the implementation of preventing a shift due to the scrollbar taking viewport space before the select is opened, and then maintaining the previously occupied scrollbar space when it is no longer showing after the select is opened due to overflow: hidden on the body. As there is a scrollbar and then there isn't and radix's implementation of dealing with that via [react-remove-scroll] doesn't affect fixed elements, because the margin on the body it adds doesn't affect those elements, there is a visible layout shift:

2023-12-20 12 25 03

However, you'll only see that issue if you have scrollbars set to "Always" in MacOS (see https://github.com/radix-ui/primitives/issues/1925#issuecomment-1546419319 for reference of the setting I'm talking about). That header is using fixed positioning and so it's affected. This happens because the viewport width actually changes when the scrollbar goes.

If you have the OS level setting set to "When scrolling" (or they aren't showing based on the automatic setting) then you won't see the issue in the first place. This is because with that setting, the scrollbar sits on top of the page, not ever affecting the viewport width.

2023-12-20 12 33 06

That screenshot shows my original proposed solution, but scrollbar-gutter does also help, I just couldn't use it when I had to deal with this issue due to lack of Safari support for that property.

AndrShept commented 10 months ago

html body {

overflow-y: auto !important; margin-right: 0 !important;

}

Roeefl commented 10 months ago

Can someone please explain why is this closed? Is this intended, legit behaviour? not a bug? When toggling the Select to be opened in the radix website, the scroll in the page is not displayed. is this legit?

they are using overflow: hidden on the body or html tag. Hence the scroll will not be displayed

Yes, I am well aware, thank you. the issue with that is that it's not considered super legit to do that to begin with.

Roeefl commented 10 months ago

Roeefl

@AndrShept You are misleading other people to modify their entire's app body styling especially for 1 specific UI component consumed from radix-ui. Yes, it's a great hack, but this should really be discouraged.

JasonColeyNZ commented 8 months ago

I have just come across this. And its even worse if you have a popover on a modal. It appears the same -15px is applied to the modal, however, when you open the popover on the modal, then close it you get the shift even with the fixes above. Because I assume that when the popover closes the -15px is removed, but the modal is still open, causing a shift of the dialog, which is even worse than the layout shift happening behind the modal as was happening. Surely there is a better solution to this, or least a way to opt out of the scroll-lock thing.

Actually my bad, its a dropdownmenu that is causing this issue on a dialog, however, same issue really.

DanielSpindler commented 8 months ago

I have just come across this. And its even worse if you have a popover on a modal. It appears the same -15px is applied to the modal, however, when you open the popover on the modal, then close it you get the shift even with the fixes above. Because I assume that when the popover closes the -15px is removed, but the modal is still open, causing a shift of the dialog, which is even worse than the layout shift happening behind the modal as was happening. Surely there is a better solution to this, or least a way to opt out of the scroll-lock thing.

Actually my bad, its a dropdownmenu that is causing this issue on a dialog, however, same issue really.

also the same issue. for a few months now. i dont know why this is getting closed if the "solutions" are only workarounds

JasonColeyNZ commented 8 months ago

also the same issue. for a few months now. i dont know why this is getting closed if the "solutions" are only workarounds

I even decided to look at other ui and went to mantine, they also use react-remove-scroll, but at least they allow you to pass parameters down to it.

From what I can tell, they need to have a count of controls using the react-remove-scroll (or react-remove-scroll) should do this internally, and not remove the css until the count is down to zero. This may get around at least the multiple controls using it?

JasonColeyNZ commented 8 months ago

also the same issue. for a few months now. i dont know why this is getting closed if the "solutions" are only workarounds

Hopefully they are looking into this... see here.

JasonColeyNZ commented 8 months ago

Therehas been a fix associated with my issue, i.e. multiple layered controls using the react-remove-scroll, see here, will this be implemented to help fix the issues?

Regression introduced when used in nested contexts

digitaltim-de commented 7 months ago

Best worked solutions for me for shadcn and macos tested:

html {
  scrollbar-gutter: stable;
}

body[style] {
  margin: 0 auto !important;
}

body[data-scroll-locked] {
  overflow: hidden;
  position: fixed;
  width: 100%;
}
JasonColeyNZ commented 7 months ago

OK so I have overridden the react-remove-scroll used in radix-ui components, react-dialog and react-popover to use the latest "2.5.9" and the issues have gone so far from testing, this is to do with using layered components using these components, i.e. a dropdown menu on a dialog etc. This is what I placed in my package.json...

"overrides": {
"@radix-ui/react-dialog": {
      "react-remove-scroll": "2.5.9"
    },
    "@radix-ui/react-popover": {
      "react-remove-scroll": "2.5.9"
    }
}
thehashton commented 6 months ago

This is what fixed it for me inside global.css (tailwind css). You have to target body[data-scroll-locked] but you have to be even more specific by adding html as well because the DropDownMenu component will still try to override it.

  html body[data-scroll-locked] {
    margin-right: 0 !important;
  }
Maliksidk19 commented 4 months ago

Don't need to add extra classes or something just remove this min width class from select component imported from shadcn and this issue will gone

image

Also from the dropdown

image