Skip to content

feat(overlay): add a utility for disabling body scroll #1971

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

feat(overlay): add a utility for disabling body scroll #1971

wants to merge 5 commits into from

Conversation

crisbeto
Copy link
Member

Adds a DisableBodyScroll injectable that can toggle whether the body is scrollable.

Fixes #1662.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 23, 2016
@crisbeto
Copy link
Member Author

@jelbourn I have a couple of doubts about this:

  1. I had to do a workaround with max-height for the default case where the body has a margin. The workaround can be avoided if we clear the body margin. Should I do that in the core styles?
  2. I wasn't sure whether this was the "Angular 2" way of doing the DOM manipulation (e.g. whether it wouldn't conflict with other renderers).

@crisbeto crisbeto force-pushed the disable-body-scroll branch 3 times, most recently from fbbb72a to 6b4492c Compare November 24, 2016 20:40
@jelbourn jelbourn mentioned this pull request Nov 30, 2016
Closed
@fxck
Copy link
Contributor

fxck commented Nov 30, 2016

So this will only work on body. How about making it into directive instead and making it work on any element user adds it to? Shouldn't be too hard, just a couple of changes, maybe a little different method for disabling the scroll as well.

@crisbeto
Copy link
Member Author

This may not be an issue for elements, at least the ones that use an overlay. Otherwise it should be simpler since we can block the wheel and keyboard events.

@MateuszG
Copy link

MateuszG commented Dec 24, 2016 •
edited
Loading

The idea with DisableBodyScroll sounds good for me.
This PR waiting month only for review?

@crisbeto crisbeto force-pushed the disable-body-scroll branch from b2ab61e to cf01df8 Compare January 4, 2017 19:36
@crisbeto crisbeto force-pushed the disable-body-scroll branch from cf01df8 to f4e5585 Compare January 12, 2017 18:43
@crisbeto
Copy link
Member Author

@jelbourn can you take a look at https://github.com/angular/material2/pull/1971/files#diff-9e063ef46d4c614a351630e38fff09b9R66 specifically? I'm not too sure how to do the DI in the constructor with the new pattern for the forRoot providers.

@devversion
Copy link
Member

@crisbeto Ah, you ran exactly into the same issue I did 😄 You need to do it similar as in a275906#diff-a648493d9ddb16f3470a4d9d93419cffR74

@crisbeto crisbeto force-pushed the disable-body-scroll branch from 00aedd9 to 2d82339 Compare January 14, 2017 16:12
@crisbeto crisbeto mentioned this pull request Jan 20, 2017
Closed
@fxck
Copy link
Contributor

fxck commented Feb 6, 2017

I think this would conflict with other renderers. You should wrap it, but I'm not really sure how it would really help in other renderers, unless they implemented document's and window's methods.

Is there any other issue blocking this?

@crisbeto
Copy link
Member Author

crisbeto commented Feb 6, 2017

We don't really need to wrap it, it just won't do anything if the document or window aren't there. There shouldn't be anything else blocking this.

@fxck fxck mentioned this pull request Feb 21, 2017
Closed
@alexw10
Copy link

alexw10 commented Feb 24, 2017

Any update on this and when it will be merged?
Also this should work on all things that use overlays right?

Mainly concerned with dialog right now as it is I am getting the body scrolling happening in the background even though I only want the dialog to be scrollable.

@fxck
Copy link
Contributor

fxck commented Feb 27, 2017

@alexw10 this is just a utility, different components will use different approaches (if I remember correctly, three were mentioned, blocking, repositioning.. and dunno), but yes, dialog will most likely utilise this. If it ever gets merged. I have serious concerns if something didn't happen to @jelbourn. I hope it's just well deserved holiday.

@dahaupt
Copy link

dahaupt commented Feb 27, 2017

@kara Can you confirm that @jelbourn is fine and "just" on holiday?

@kara
Copy link
Contributor

kara commented Feb 27, 2017 •
edited
Loading

@dahaupt Haha yes, I can confirm that nothing bad has happened to Jeremy. He should be able to review this soon.

@dahaupt
Copy link

dahaupt commented Feb 27, 2017

@kara Okay fine :) I can sleep better now 😄

@alexw10
Copy link

alexw10 commented Feb 27, 2017 •
edited
Loading

Just as a note as well I did run this recently and it didn't seem to work when utilized with dialog at least. I might have been doing something wrong though so just something to check on that is if it is intended to work with dialog :) good to hear @kara that @jelbourn is doing okay! 💯

@crisbeto
Copy link
Member Author

This PR only sets up the infrastructure that allows for scrolling to be disabled, it doesn't integrate it with any of the components.

@fxck
Copy link
Contributor

fxck commented Mar 20, 2017

How's the review coming?

@jelbourn
Copy link
Member

I've been holding off on this one since I'm generally suspicious of the "disable body scroll" concept; I'd like to stick to the philosophy of only modifying the elements that material "owns". However, there could be value in having this as an option for overlays (vs. re-positioning w/ ScrollDispatcher) that users explicitly opt into, so I still want to take a look

@fxck
Copy link
Contributor

fxck commented Mar 21, 2017

Repositioning sucks though. You won't ever manage to make it smooth, especially on phones, tablets and older pcs. I wish it wasn't implemented on tooltip at all #3413 it lags even my new MBP / chrome.

@fxck fxck mentioned this pull request Apr 5, 2017
Closed
@jelbourn jelbourn added needs: discussion Further discussion with the team is needed before proceeding and removed pr: needs review labels Apr 19, 2017
jelbourn
jelbourn reviewed Apr 19, 2017
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally left some comments for discussion. I suspect you'll want to open a fresh PR based on our discussion on the different scroll handling strategies, but we can still talk about how to implement this aspect of it here.

src/lib/core/overlay/disable-body-scroll.ts
*/
@Injectable()
export class DisableBodyScroll {
private _bodyStyles: string = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add descriptions for _htmlStyles and _bodyStyles since they're not immediately obvious?

src/lib/core/overlay/disable-body-scroll.ts
* Utilitity that allows for toggling scrolling of the viewport on/off.
*/
@Injectable()
export class DisableBodyScroll {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about PageScrollBlocker?

src/lib/core/overlay/disable-body-scroll.ts
/**
* Disables scrolling if it hasn't been disabled already and if the body is scrollable.
*/
activate(): void {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How able calling the methods disableScrolling and enableScrolling, since I could see someone getting mixed up, e.g., activate meaning to turn scrolling on.

src/lib/core/overlay/disable-body-scroll.ts
body.style.position = 'fixed';
body.style.width = '100%';
body.style.top = -this._previousScrollPosition + 'px';
html.style.overflowY = 'scroll';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always setting overflow: scroll could be a problem if the page does not already have a visible scrollbar. I think we'd have to be more intelligent about whether or not a scrollbar is already showing. Ultimately we don't want scrollbars appearing when they weren't there before.

Also, there can potentially be a horizontal scrollbar as well.

src/lib/core/overlay/disable-body-scroll.ts
let html = document.documentElement;
let initialBodyWidth = body.clientWidth;

this._htmlStyles = html.style.cssText || '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than caching the styles and setting them back, what do you think of just adding a removing a CSS class from the body where all of the rules are !important? It might make things simpler.

src/lib/core/overlay/disable-body-scroll.ts
// TODO(crisbeto): this avoids issues if the body has a margin, however it prevents the
// body from adapting if the window is resized. check whether it's ok to reset the body
// margin in the core styles.
body.style.maxWidth = initialBodyWidth + 'px';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if we could avoid getting into the reset styles game. Can you go into more details about what the issue is with the default margin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been a while since I wrote this, but IIRC the margin ended up throwing off some of the measurements.

src/lib/core/overlay/disable-body-scroll.ts
this._bodyStyles = body.style.cssText || '';
this._previousScrollPosition = this._viewportRuler.getViewportScrollPosition().top;

body.style.position = 'fixed';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you about to talk at all about this approach vs. just setting overflow: hidden to the body?

Copy link
Member Author

@crisbeto crisbeto Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that for the position and the width, but the top is dynamic. Regarding the overflow: hidden, I believe that it doesn't work on mobile.

@crisbeto
Copy link
Member Author

Closing in favor of #4500.

@crisbeto crisbeto closed this May 12, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

Assignees

@jelbourn jelbourn

Labels
cla: yes PR author has agreed to Google's Contributor License Agreement needs: discussion Further discussion with the team is needed before proceeding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

overlay: Investigate ways of blocking body scroll
9 participants
@crisbeto @fxck @MateuszG @devversion @alexw10 @dahaupt @kara @jelbourn @googlebot