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

Prevent scroll on parent when dialog is open #98

Closed
sjelfull opened this issue Dec 13, 2018 · 11 comments
Closed

Prevent scroll on parent when dialog is open #98

sjelfull opened this issue Dec 13, 2018 · 11 comments
Labels
Type: Enhancement General improvements or suggestions

Comments

@sjelfull
Copy link

Could Dialog prevent scroll on body when open, or is that up to the app?

@theKashey
Copy link
Contributor

theKashey commented Jan 2, 2019

It should be handled by Dialog itself.
And here is one more good reason to do it - iOS Safary does not properly handle position:fixed elements with input. Once you will add input/textarea into Dialog, and focus it - your customer is lost.

That's a feature - they browser is focusing on the input, to provide a better UX... but not.

https://stackoverflow.com/questions/14492613/ios-ipad-fixed-position-breaks-when-keyboard-is-opened

@ryanflorence
Copy link
Member

Yeah I want this.

I've seen solutions where the screen jumps because the scroll bar gets removed (add overflow: hidden to the body).

I haven't had a chance to research other ways to do it that avoid the jump. Open to suggestions. Here's the spectrum thread on the same topic: https://spectrum.chat/reach/general/modal-dialog-should-lock-focus~3cd21c66-6c3d-4b73-adcb-10856a44eadd

@ryanflorence
Copy link
Member

ryanflorence commented Jan 4, 2019

(also, happy to merge a PR that simply adds overflow: hidden to body until then :)

@theKashey
Copy link
Contributor

Please don't.
overflow:hidden is working mostly for desktop browsers, while problem is mostly about mobile ones. You have to silence scroll/onwheel events - it will fix iOS, and prevent "normal way" of scroll on desktop - you still could drag the scroll bar, but would you?
But it's not so easy, and (today) absolutely not friendly with portals by design(killing events everywhere, except whitelisted node). While some selects nowadays uses portals - that's not a great idea (anyway current focus-trap is also not portal-friendly).

@ryanflorence
Copy link
Member

@theKashey What is your solution then?

@sjelfull
Copy link
Author

sjelfull commented Jan 5, 2019

Could https://github.com/willmcpo/body-scroll-lock be a good alternative, either by copying their approach or using it as a dependency?

@theKashey
Copy link
Contributor

@sjelfull - 1.1KB total size seems quite good, twice better than "react" locks. And allowTouchMove callback probably could be used to handle portals in the future.

@ryanflorence - mine solution nowadays would block any interactions with portaled content, ie menu, and twice bigger than body-lock.

@karlhorky
Copy link
Contributor

karlhorky commented May 3, 2019

#111 has an issue with scroll locking: #161 (upstream theKashey/react-remove-scroll#4).

@bradwestfall bradwestfall added the Type: Enhancement General improvements or suggestions label Oct 1, 2019
@chaance chaance closed this as completed Oct 15, 2019
@donaldpipowitch
Copy link

donaldpipowitch commented Oct 30, 2019

Note: If you use width: 100vw; overflow-x: hidden; on <html/> to prevent scroll bar jumps (see here or here) than opening a @reach/dialog will currently re-introduce the layout jump, because of https://github.com/theKashey/react-remove-scroll. Maybe @reach/dialog could add a class to <html/> whenever a dialog is open so we can react on it. (The concrete problem is a margin-right: 15px !important; on body. I currently wrap DialogOverlay in another custom component where I now adjust body on mount/unmount to override this style.)

@theKashey
Copy link
Contributor

theKashey commented Oct 30, 2019

Not sure how it looks like, but react-remove-scroll is exporting a few classes to (probably) handle situations like this - RemoveScroll.fullWidth to set margin-right: 15px !important; and RemoveScroll.zeroRight to set right: 15px !important; (the actual "gap" depends on the system)
These classes are actually just a string constants, and exists only if ScrollLock is active. Feel free to apply them to the target node.
see https://github.com/theKashey/react-remove-scroll-bar/blob/master/src/constants.ts

@donaldpipowitch
Copy link

Note: If you use width: 100vw; overflow-x: hidden; on <html/> to prevent scroll bar jumps

Additionally... If you use position: sticky for the header you actually need to set overflow-x: visible; on <html/> whenever the dialog is open to prevent the header from jumping around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement General improvements or suggestions
Projects
None yet
Development

No branches or pull requests

7 participants