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 add-padding-to-html technique for scrollbar is problematic and there's no way to control it #1319

Closed
firxworx opened this issue Apr 11, 2022 · 17 comments
Assignees

Comments

@firxworx
Copy link

firxworx commented Apr 11, 2022

Package: @headlessui/react
Version: v1.5.0 (latest at time of writing)
Chrome + Windows 11 (this issue affects many others as well; see description + case)

Reproduction URL

Any example of the Dialog component is a valid reproduction: open DevTools and monitor the HTML element while opening a Dialog.

Refer to the implementation in dialog.tsx where the issue behaviour is implemented in a useEffect() preceded by the comment line // Scroll lock:

Issue Description

The current Dialog implementation makes less-than-ideal assumptions re scrollbars + project CSS and when a Dialog/modal is open, it applies a technique that adds an inline overflow: hidden and padding-right (with value based on a calculation of scrollbar width) to the document's <html> tag.

This results in undesired behaviour / jank in different cases especially on Windows PC's where scrollbars are rather visually imposing.

The lack of control for this behaviour (especially the padding) leaves devs without the ability to more elegantly handle scrollbars and/or handle corner cases or conflicts caused by other styles in a given project.

Why is this important?

Scrollbars are a notoriously fickle beast between different OS's and browsers (see: one example among many articles on the subject).

I would argue a good reason to check/switch between Mac + PC when working on front-end projects are the differences in scrollbars between these OS's. Our traffic doesn't always have the same taste in computers vs. many of us web devs :D

Example case:

The following breaks down a single case that is part of a wider issue; refer to linked issue + discussion for related cases.

A straightforward cross-platform-friendly technique to avoid jank during loading + transitions that is particularly gross on Windows is to add overflow-y: scroll to the body tag. This causes browsers on PC's to always display a scrollbar even when the content fits within the viewport (in this case the scrollbar is present but is presented as disabled/inactive).

This technique is in the tailwind-preset.js on several projects I work on and is compatible with many packages/libraries/frameworks to help avoid "jumps" and overall "jank".

However in this case Dialog is the cause of the very problem it is attempting to prevent:

image

Content behind the Dialog noticeably "jumps" to the left by the width of the scrollbar.

In the screenshot we see padding-right jumping the content by the width of the scrollbar in both of the following cases: when the content is taller than the viewport (i.e. case for y scrollbar), and when it is shorter (i.e. nothing to scroll).

Confirming:

  • when overflow-y: scroll is removed from the body then Dialog behaves the way that the devs presumably intended (tested on Chrome+PC): no padding or jump is present in the content-fits-window case (no scrollbar visible), and in the content-taller-than-window case (scrollbar visible) the added padding compensates for the scrollbar disappearance resulting from Dialog's addition of overflow: hidden.

The presumably-intended behaviour isn't particularly elegant in the content-taller-than-window-case on PC either:

Windows' imposing scrollbar suddenly disappears and this can result in users' "wtf just blinked"/"jank detection" neurons firing. While a content "jump" is prevented the sudden replacement of the scrollbar with a solid vertical block that doesn't match the app layout isn't much better.

At present, devs have no way to control any of this behaviour if they wish to use Dialog --

Before Dialog is rendered (imposing Windows scrollbar is visible):

image

With Dialog modal showing (scrollbar suddenly disappears and it ain't pretty):

image

Expected/Desired Behaviour

Developers require the ability to control the overflow+padding behaviour of Headless Dialog (e.g. exposed via props)

  • Dialog is making an assumption that depends on other CSS in the project being a certain way that may not always hold true
  • Devs may disagree with an opinionated technique (e.g. jump avoided but jarring swap of scrollbar may not be desired) and wish to override, but still leverage all the other benefits of Headless + Dialog.

Depending on maintainers' perspective re the presumably-intended behaviour + based on screenshots of it on PC they may wish to classify it as a bug and revisit.

  • Different technique(s) or additional case handling (e.g. scrollbar, PC, etc.) could be implemented to improve Dialog in this area

Related Ideas & Discussions:

@youssefm
Copy link

I just filed this related issue that seems to further support your reasoning:

#1436

@RobinMalfait
Copy link
Member

Hey! Thank you for your bug report!
Much appreciated! 🙏

This should be fixed by #1457, and will be available in the next release.

You can already try it using:

  • npm install @headlessui/react@insiders.
  • npm install @headlessui/vue@insiders.

Can you double check that this is fixed for you if you install the insiders build?

@RobinMalfait
Copy link
Member

Hey!

Doing some cleanup and haven't heard back from you so therefore going to close this issue. If this is still an issue for you, please open a new issue with a reproduction repo that we can clone and run and I'm happy to take a look!

@missouak
Copy link

missouak commented Oct 6, 2022

Package: @headlessui/react Version: v1.5.0 (latest at time of writing) Chrome + Windows 11 (this issue affects many others as well; see description + case)

Reproduction URL

Any example of the Dialog component is a valid reproduction: open DevTools and monitor the HTML element while opening a Dialog.

Refer to the implementation in dialog.tsx where the issue behaviour is implemented in a useEffect() preceded by the comment line // Scroll lock:

Issue Description

The current Dialog implementation makes less-than-ideal assumptions re scrollbars + project CSS and when a Dialog/modal is open, it applies a technique that adds an inline overflow: hidden and padding-right (with value based on a calculation of scrollbar width) to the document's <html> tag.

This results in undesired behaviour / jank in different cases especially on Windows PC's where scrollbars are rather visually imposing.

The lack of control for this behaviour (especially the padding) leaves devs without the ability to more elegantly handle scrollbars and/or handle corner cases or conflicts caused by other styles in a given project.

Why is this important?

Scrollbars are a notoriously fickle beast between different OS's and browsers (see: one example among many articles on the subject).

I would argue a good reason to check/switch between Mac + PC when working on front-end projects are the differences in scrollbars between these OS's. Our traffic doesn't always have the same taste in computers vs. many of us web devs :D

Example case:

The following breaks down a single case that is part of a wider issue; refer to linked issue + discussion for related cases.

A straightforward cross-platform-friendly technique to avoid jank during loading + transitions that is particularly gross on Windows is to add overflow-y: scroll to the body tag. This causes browsers on PC's to always display a scrollbar even when the content fits within the viewport (in this case the scrollbar is present but is presented as disabled/inactive).

This technique is in the tailwind-preset.js on several projects I work on and is compatible with many packages/libraries/frameworks to help avoid "jumps" and overall "jank".

However in this case Dialog is the cause of the very problem it is attempting to prevent:

image

Content behind the Dialog noticeably "jumps" to the left by the width of the scrollbar.

In the screenshot we see padding-right jumping the content by the width of the scrollbar in both of the following cases: when the content is taller than the viewport (i.e. case for y scrollbar), and when it is shorter (i.e. nothing to scroll).

Confirming:

  • when overflow-y: scroll is removed from the body then Dialog behaves the way that the devs presumably intended (tested on Chrome+PC): no padding or jump is present in the content-fits-window case (no scrollbar visible), and in the content-taller-than-window case (scrollbar visible) the added padding compensates for the scrollbar disappearance resulting from Dialog's addition of overflow: hidden.

The presumably-intended behaviour isn't particularly elegant in the content-taller-than-window-case on PC either:

Windows' imposing scrollbar suddenly disappears and this can result in users' "wtf just blinked"/"jank detection" neurons firing. While a content "jump" is prevented the sudden replacement of the scrollbar with a solid vertical block that doesn't match the app layout isn't much better.

At present, devs have no way to control any of this behaviour if they wish to use Dialog --

Before Dialog is rendered (imposing Windows scrollbar is visible):

image

With Dialog modal showing (scrollbar suddenly disappears and it ain't pretty):

image

Expected/Desired Behaviour

Developers require the ability to control the overflow+padding behaviour of Headless Dialog (e.g. exposed via props)

  • Dialog is making an assumption that depends on other CSS in the project being a certain way that may not always hold true
  • Devs may disagree with an opinionated technique (e.g. jump avoided but jarring swap of scrollbar may not be desired) and wish to override, but still leverage all the other benefits of Headless + Dialog.

Depending on maintainers' perspective re the presumably-intended behaviour + based on screenshots of it on PC they may wish to classify it as a bug and revisit.

  • Different technique(s) or additional case handling (e.g. scrollbar, PC, etc.) could be implemented to improve Dialog in this area

Related Ideas & Discussions:

you can fix it by adding to your main stylesheet file the following :
html{
overflow-y: auto!important;
}

@willemmulder
Copy link

This problem still exists. My page jumps and there is a gap on the right side of the screen. I had this

overflow-y: scroll;

And had to change it into this to fix the problem

overflow-y: scroll !important;
padding: 0px !important;

@Anu-cool-007
Copy link

I added the margin to the body instead of HTML which solved the issue.

body {
    margin-left: calc(100vw - 100%);
    /* overflow-y: auto !important; */
}

@imclint21
Copy link

Yes the problem still exists:

<html lang="en" style="overflow: hidden; padding-right: 15px;">

It looks like this:

Screenshot 2022-11-14 at 15 15 33

To fix it I did this below, but I would prefer don't have this to do:

html
{
    padding: 0 !important;
}

@amaroyche
Copy link

Same here, just not having a control over Dialog's addition of overflow: hidden. This is what is the culprit.
Suggesting to solve this and related issues by giving devs a choice to modify this behavior (leaving default as is).

{
    modifyScrollUnderModal: true,
    modifyScrollUnderModalTag: 'html',
    modifyScrollUnderModalStyles: { overflow: 'hidden' }
}

@imclint21
Copy link

Same here, just not having a control over Dialog's addition of overflow: hidden. This is what is the culprit.

Suggesting to solve this and related issues by giving devs a choice to modify this behavior (leaving default as is).


{

    modifyScrollUnderModal: true,

    modifyScrollUnderModalTag: 'html',

    modifyScrollUnderModalStyles: { overflow: 'hidden' }

}

Good suggestion

@EliBates
Copy link

Would also like a way to control this.

@dbstjddbwls
Copy link

I think this is one of the urgent issues for developers who use headlessui. Has anyone solved this issue?

@firxworx
Copy link
Author

@RobinMalfait given the comments + activity I think its grounds to reopen this issue?

@RobinMalfait
Copy link
Member

Can anyone running into this issue open a new issue with a minimal reproduction repo attached that showcases the problem so that we can take a look?

@hymair
Copy link

hymair commented Feb 2, 2023

Had the same issue on 1.7.4 but upgrading to 1.7.8 fixed it.

@aaroncemery
Copy link

aaroncemery commented Feb 2, 2023

I'm on 1.7.8 and just ran into this issue.

What i see happening is that the issue is not present for me, but it is for some of the people who were testing.

@maximedotair
Copy link

Hello ! 1.7.10 and same here

@RobinMalfait
Copy link
Member

Alright, going to lock this issue. For the people running into this, please open a new issue with a minimal reproduction repo so that we can take a look. Otherwise it is impossible for us to debug what's going on.

@tailwindlabs tailwindlabs locked as resolved and limited conversation to collaborators Feb 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests