[useScrollLock] Avoid scrollbar layout shift issues #604

atomiks · 2024-09-12T06:20:39Z

Closes #603

I have followed (at least) the PR section of the contributing guide.

This applies the internal iOS technique to all browsers aside from iOS, removing the need for scrollbar compensation.
iOS implementation is pending

Problem 1

This currently breaks the positioning of (anchored) floating elements. Edit: works if using the fixed positioning strategy instead of absolute + fixed in latest Floating UI version.

This raises some minor interoperability concerns with extensions and third party widgets that use outdated versions of Floating UI, Popper (assuming it's broken there) or custom positioning methods that don't handle this edge case (where html has an offset).

Problem 2

The forced overflow of the <html> scrollbar appears on top of backdrops, which looks a bit unsightly if it's not a custom scrollbar

mui-bot · 2024-09-12T06:23:47Z

Netlify deploy preview

https://deploy-preview-604--base-ui.netlify.app/

Generated by 🚫 dangerJS against a9d020e

atomiks · 2024-09-12T07:04:17Z

Problem 1: floating-ui/floating-ui#3045

vladmoroz

However, this raises the concern of interoperability with extensions and third party widgets that use outdated versions of Floating UI, Popper (assuming it's broken there) or custom positioning methods that don't handle this edge case (where html has an offset).

I think this is fine given that Base UI will take some time to get real traction, and a newer Floating UI version will be around during this time. Given that Floating UI is a dependency, I think we should recommend to use a matching Floating UI version in the docs either way.

The forced overflow of the scrollbar appears on top of backdrops, which looks a bit unsightly if it's not a custom scrollbar

I agree this is not ideal. I'd take that over any layout shift though.

packages/mui-base/src/utils/useScrollLock.ts

When the body has an overflow-y: scroll style, layout shift would still occur

packages/mui-base/src/utils/useScrollLock.ts

vladmoroz

Alright, desktop looks fantastic.

Checking iOS now and I can get it to do weird jerky right after zooming in or out... Is there any hope we can nail this?

RPReplay_Final1726222510.mov

vladmoroz · 2024-09-13T14:00:34Z

Doing another test:

On an iOS device, I can't seem to scroll or zoom at all
There's a shift when resizing the viewport in Chrome with scrollbar-gutter: stable. This used to works fine in 51fd682

Screen.Recording.2024-09-13.at.15.59.42.mov

atomiks · 2024-09-13T15:00:48Z

On an iOS device, I can't seem to scroll or zoom at all

You mean once the scroll lock is activated and using pinch-zoom? I can also reproduce this. I suppose it's a limitation of react-aria's solution then, unfortunately. If you mean scrolling or zooming while it's not activated, I can't reproduce that.

There's a shift when resizing the viewport in Chrome with scrollbar-gutter: stable. This used to works fine in 51fd682

Will have to check that.

vladmoroz · 2024-09-13T15:35:35Z

You mean once the scroll lock is activated and using pinch-zoom? I can also reproduce this. I suppose it's a limitation of react-aria's solution then, unfortunately

Wow I'm shocked that that's what they have... if you open a dialog zoomed in, your only way to get out of it is basically to close the page.

atomiks · 2024-09-14T04:15:24Z

I'll try to complete the touch-action prototype I had which still allowed pinch-zooming. The logic to find the scrollable container is possible with DOM traversal on a touch event

oliviertassinari · 2024-09-14T23:25:16Z

@atomiks To reproduce, you need to empty the page to not have enough content to create a scrollbar:

Screen.Recording.2024-09-15.at.01.23.39.mov

https://deploy-preview-604--base-ui.netlify.app/components/react-dialog/

@joshwcomeau Cool hack in https://www.joshwcomeau.com/css/has/#global-detection-6 by the way:

but we can't use this here 😄.

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Signed-off-by: atomiks <cc.glows@gmail.com>

packages/mui-base/src/utils/useScrollLock.ts

vladmoroz

Phew, looks great. Added a slight comment on the scrollbar-gutter edge case, otherwise good to go.

I'm super excited about this, reliable scroll locking without layout shifts was such a pain with Radix and other libs.

For posterity, here's what I was testing across browsers

html with overflow: scroll

Scrollable page
Non-scrollable page
Reducing viewport size to make the page scrollable
Expanding viewport size to make the page not scrollable

html with scrollbar-gutter: stable

Scrollable page
Non-scrollable page
Reducing viewport size to make the page scrollable
Expanding viewport size to make the page not scrollable

body with overflow: scroll

Scrollable page
Non-scrollable page
Reducing viewport size to make the page scrollable
Expanding viewport size to make the page not scrollable

Default browser styles on body and html

Scrollable page
Non-scrollable page
Reducing viewport size to make the page scrollable
Expanding viewport size to make the page not scrollable

not cleanest feeling UX. I guess the idea is that it's easier for developers not to have to add .mui-fixed like class names? I would feel better like this though:

@oliviertassinari check this out:

Screen.Recording.2024-09-16.at.12.02.34.mov

Screen.Recording.2024-09-16.at.12.05.34.mov

Developers consistently forget about hard scrollbars. Even in the teams of the highest calibre, these often end up being completely unhandled.

I agree that it would be ideal if the backdrop covered the scrollbar area. I am confident though that's not what the end user would be looking at or care about, since their attention is going to be on the part of the page that presents new information. Any risk of having a layout shift is not worth it in my opinion.

With other component libraries, we've been living with the common scrollbar compensation method that requires you to use a CSS variable to set a right value for position: fixed elements, and realistically people just forget to do this all the time.

As another example, here is a UI I had built:

Screen.Recording.2024-09-16.at.12.06.29.mov

The header doesn't move because I did handle that, but the input got messed up over time. Maybe it was me, maybe another developer forgot that; it's natural because you don't anticipate any relationship between a random element on the page and an unrelated dialog.

My guiding rule here is that great DX and great UX go hand in hand. Compromises in DX tend to lead to end user issues.

packages/mui-base/src/utils/useScrollLock.ts

Co-authored-by: Vlad Moroz <hi@vladmoroz.com> Signed-off-by: atomiks <cc.glows@gmail.com>

oliviertassinari · 2024-09-16T17:35:46Z

Developers consistently forget about hard scrollbars. Even in the teams of the highest calibre, these often end up being completely unhandled.

@vladmoroz My guess it's because everyone on those teams works on macOS, so they don't see it. They forgot that 70% of the end-users are on Windows. And even for developers, it's still a majority https://www.notion.so/mui-org/Product-Analytics-f2e2576203564e6c8dc3429cf305a36f?pvs=4#8a38433d21814a15acfb99ce2eac7d6b.

Any risk of having a layout shift is not worth it in my opinion.

Yeah, hard dilemma. Your rationale makes sense. Not peak of UX but we avoid the worst-case scenario, and since this is what people tend to remember, it's probably for the best: https://www.notion.so/mui-org/DevEx-DX-044f844fbda74826b2dea32a6e6d8a38?pvs=4#2a825570dd744a5a90b75e014100562e (point made by https://github.com/rsms).

I guess in this case, it's critical to have color-scheme: dark set for the top-level scrollbar to be dark in dark mode 😄.

@atomiks overflow-y: scroll on the html works now 👌

oliviertassinari · 2024-09-16T17:46:23Z

packages/mui-base/src/utils/useScrollLock.ts

+  const html = document.documentElement;
+  const body = document.body;


As far as I know, this should only be a fallback. It breaks supports for iframes and popups, we fixed a bunch of those through the years, e.g. mui/material-ui#18701, mui/material-ui#10328. Example of use case: https://www.openfin.co/.

That doesn't apply here since it's locking document level of the current script. There's no element to grab an owner of.

There's no element to grab an owner of.

This is the point I get to, I don't see how this API can work:

base-ui/packages/mui-base/src/Dialog/Popup/useDialogPopup.tsx

Line 49 in 1ab27b0

useScrollLock(modal && mounted);

I think we should provide to useScrollLock.ts enough context to know which document it needs to scroll lock. Issue created #629.

vladmoroz

iOS looks good too

I think we got to remove the theme_color from manifest.json because it's super obnoxious when testing iOS; kind of spoils the impression when it doesn't have to.

[useLockScroll] Avoid scrollbar issues

39e9ef3

atomiks added the core Infrastructure work going on behind the scenes label Sep 12, 2024

atomiks requested review from colmtuite and michaldudak as code owners September 12, 2024 06:20

atomiks added 3 commits September 12, 2024 17:41

window.scrollTo?. for jsdom

353ca90

Check for property

f55a1ea

Check for native scrollTo

f9c5894

vladmoroz reviewed Sep 12, 2024

View reviewed changes

packages/mui-base/src/utils/useScrollLock.ts Outdated Show resolved Hide resolved

vladmoroz reviewed Sep 12, 2024

View reviewed changes

packages/mui-base/src/utils/useScrollLock.ts Outdated Show resolved Hide resolved

vladmoroz reviewed Sep 12, 2024

View reviewed changes

packages/mui-base/src/utils/useScrollLock.ts Outdated Show resolved Hide resolved

atomiks added 2 commits September 12, 2024 19:47

Handle original styles

de27905

Handle scrollbar-gutter: stable

51fd682

atomiks force-pushed the fix/lockScroll branch from ddf3668 to 51fd682 Compare September 12, 2024 10:03

atomiks added 3 commits September 12, 2024 20:10

Improve nested locking

9e9c74a

Apply to body

8d23647

When the body has an overflow-y: scroll style, layout shift would still occur

Playground + resize + iOS fixes

25438b6

vladmoroz reviewed Sep 13, 2024

View reviewed changes

packages/mui-base/src/utils/useScrollLock.ts Outdated Show resolved Hide resolved

atomiks added 4 commits September 13, 2024 19:02

Move scrollbarWidth into function

9392376

Comments

c74dc6b

typo

d27e797

Improve experiment

acefccd

vladmoroz reviewed Sep 13, 2024

View reviewed changes

atomiks added 2 commits September 13, 2024 22:38

Use react-aria for iOS

4ccd89b

Simplify lockIds

2a21a48

atomiks and others added 6 commits September 16, 2024 15:08

Rewrite implementation

e3f9bf7

Remove body style

66abfac

Remove id

01fec79

Handle constant overflow

bc8ecc9

Handle dual scrollbars

bbf5374

Update docs/app/experiments/scroll-lock.tsx

512c5d0

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Signed-off-by: atomiks <cc.glows@gmail.com>

vladmoroz reviewed Sep 16, 2024

View reviewed changes

packages/mui-base/src/utils/useScrollLock.ts Outdated Show resolved Hide resolved

vladmoroz reviewed Sep 16, 2024

View reviewed changes

packages/mui-base/src/utils/useScrollLock.ts Outdated Show resolved Hide resolved

vladmoroz reviewed Sep 16, 2024

View reviewed changes

packages/mui-base/src/utils/useScrollLock.ts Outdated Show resolved Hide resolved

atomiks added 8 commits September 16, 2024 18:47

getComputedStyle

88e0e4f

perf

a7fbacf

Remove return

87444f8

Conditional fixed styles

70bbdfa

Assign vars earlier

8c8adc2

Add scrollable x check

80ec0c1

Handle body overflow

56ca8b9

Handle body overflow

2458dc0

vladmoroz approved these changes Sep 16, 2024

View reviewed changes

packages/mui-base/src/utils/useScrollLock.ts Outdated Show resolved Hide resolved

Update packages/mui-base/src/utils/useScrollLock.ts

d8715dd

Co-authored-by: Vlad Moroz <hi@vladmoroz.com> Signed-off-by: atomiks <cc.glows@gmail.com>

oliviertassinari reviewed Sep 16, 2024

View reviewed changes

Add iOS implementation

a9d020e

vladmoroz approved these changes Sep 17, 2024

View reviewed changes

atomiks merged commit ac6b56c into mui:master Sep 17, 2024
18 checks passed

atomiks deleted the fix/lockScroll branch September 17, 2024 08:58

This was referenced Sep 17, 2024

[useScrollLock] overflow-y: scroll support no longer working #611

Closed

[Modal] Support scrollbar-gutter #79

Closed

atomiks mentioned this pull request Sep 18, 2024

[Select] Create new Select component #541

Open

3 tasks

oliviertassinari mentioned this pull request Sep 20, 2024

[core] Refactor getScrollbarSize to operate on window instead of document mui/material-ui#43827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[useScrollLock] Avoid scrollbar layout shift issues #604

[useScrollLock] Avoid scrollbar layout shift issues #604

atomiks commented Sep 12, 2024 •

edited

Loading

mui-bot commented Sep 12, 2024 •

edited

Loading

atomiks commented Sep 12, 2024

vladmoroz left a comment

vladmoroz left a comment

vladmoroz commented Sep 13, 2024

atomiks commented Sep 13, 2024

vladmoroz commented Sep 13, 2024

atomiks commented Sep 14, 2024

oliviertassinari commented Sep 14, 2024 •

edited

Loading

vladmoroz left a comment •

edited

Loading

oliviertassinari commented Sep 16, 2024 •

edited

Loading

oliviertassinari Sep 16, 2024 •

edited

Loading

atomiks Sep 17, 2024

oliviertassinari Sep 17, 2024 •

edited

Loading

vladmoroz left a comment

		const html = document.documentElement;
		const body = document.body;

[useScrollLock] Avoid scrollbar layout shift issues #604

[useScrollLock] Avoid scrollbar layout shift issues #604

Conversation

atomiks commented Sep 12, 2024 • edited Loading

Problem 1

Problem 2

mui-bot commented Sep 12, 2024 • edited Loading

Netlify deploy preview

atomiks commented Sep 12, 2024

vladmoroz left a comment

Choose a reason for hiding this comment

vladmoroz left a comment

Choose a reason for hiding this comment

vladmoroz commented Sep 13, 2024

atomiks commented Sep 13, 2024

vladmoroz commented Sep 13, 2024

atomiks commented Sep 14, 2024

oliviertassinari commented Sep 14, 2024 • edited Loading

vladmoroz left a comment • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Sep 16, 2024 • edited Loading

oliviertassinari Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

atomiks Sep 17, 2024

Choose a reason for hiding this comment

oliviertassinari Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

vladmoroz left a comment

Choose a reason for hiding this comment

atomiks commented Sep 12, 2024 •

edited

Loading

mui-bot commented Sep 12, 2024 •

edited

Loading

oliviertassinari commented Sep 14, 2024 •

edited

Loading

vladmoroz left a comment •

edited

Loading

oliviertassinari commented Sep 16, 2024 •

edited

Loading

oliviertassinari Sep 16, 2024 •

edited

Loading

oliviertassinari Sep 17, 2024 •

edited

Loading