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

Fix x-trap.noscroll layout shift conflict with scrollbar-gutter #4512

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dotfrag
Copy link

@dotfrag dotfrag commented Jan 27, 2025

If scrollbar-gutter is set to something other than the initial value 'auto' (e.g. 'stable'), do not modify padding-right. See issue #4511.

This is my attempt to fix it, hoping it's heading to the right direction. I'm using it in my app and it's working. Feel free to discard/modify as you like.

EDIT: Does not appear to work reliably in all cases. I'm trying to understand why, otherwise I'll close it. Fixed, checking the failed test now.


Tested and seems to be working:

scrollbar-gutter undefined => overflow: hidden; padding-right: 15px;

scrollbar-gutter unchanged (defaults to 'auto') => overflow: hidden; padding-right: 15px;

scrollbar-gutter: stable => overflow: hidden;

scrollbar-gutter: stable both-edges => overflow: hidden;


Ready for review.

Caveat: scrollbar-gutter, like many other CSS properties supports the global values inherit/revent/etc... which I'm not accounting for. I believe these are really edge-cases. If you want to check for those, then the if check must be explicit (i.e. check for 'stable' and 'stable both-edges') and the return statements reversed.

@dotfrag dotfrag force-pushed the patch-1 branch 3 times, most recently from 55909e8 to 4dc5cbe Compare January 27, 2025 10:50
@ekwoka
Copy link
Contributor

ekwoka commented Jan 28, 2025

Initial and unset shouldn't impact "get computed style" since that will find what the actual applied value is, not what an elements literal setting is.

@dotfrag
Copy link
Author

dotfrag commented Jan 28, 2025

Ah yes that's true!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants