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.

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/lib/core/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export {OverlayContainer} from './overlay/overlay-container';
export {FullscreenOverlayContainer} from './overlay/fullscreen-overlay-container';
export {OverlayRef} from './overlay/overlay-ref';
export {OverlayState} from './overlay/overlay-state';
export {DisableBodyScroll} from './overlay/disable-body-scroll';
export {
ConnectedOverlayDirective,
OverlayOrigin,
Expand Down
144 changes: 144 additions & 0 deletions src/lib/core/overlay/disable-body-scroll.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
import {TestBed, async, inject} from '@angular/core/testing';
import {OverlayModule} from './overlay-directives';
import {DisableBodyScroll} from './disable-body-scroll';


describe('DisableBodyScroll', () => {
let service: DisableBodyScroll;
let startingWindowHeight = window.innerHeight;
let forceScrollElement: HTMLElement = document.createElement('div');

forceScrollElement.style.height = '3000px';
forceScrollElement.style.width = '100px';

beforeEach(async(() => {
TestBed.configureTestingModule({
imports: [OverlayModule.forRoot()],
providers: [DisableBodyScroll]
});
}));

beforeEach(inject([DisableBodyScroll], (disableBodyScroll: DisableBodyScroll) => {
service = disableBodyScroll;
}));

afterEach(() => {
if (forceScrollElement.parentNode) {
forceScrollElement.parentNode.removeChild(forceScrollElement);
}

service.deactivate();
});

it('should prevent scrolling', () => {
document.body.appendChild(forceScrollElement);
window.scrollTo(0, 100);

// In the iOS simulator (BrowserStack & SauceLabs), adding the content to the
// body causes karma's iframe for the test to stretch to fit that content once we attempt to
// scroll the page. Setting width / height / maxWidth / maxHeight on the iframe does not
// successfully constrain its size. As such, skip assertions in environments where the
// window size has changed since the start of the test.
if (window.innerHeight > startingWindowHeight) {
return;
}

window.scrollTo(0, 100);

service.activate();

window.scrollTo(0, 500);

expect(window.pageYOffset).toBe(0);
});

it('should toggle the isActive property', () => {
document.body.appendChild(forceScrollElement);
window.scrollTo(0, 100);

// In the iOS simulator (BrowserStack & SauceLabs), adding the content to the
// body causes karma's iframe for the test to stretch to fit that content once we attempt to
// scroll the page. Setting width / height / maxWidth / maxHeight on the iframe does not
// successfully constrain its size. As such, skip assertions in environments where the
// window size has changed since the start of the test.
if (window.innerHeight > startingWindowHeight) {
return;
}

service.activate();
expect(service.isActive).toBe(true);

service.deactivate();
expect(service.isActive).toBe(false);
});

it('should not disable scrolling if the content is shorter than the viewport height', () => {
service.activate();
expect(service.isActive).toBe(false);
});

it('should add the proper inline styles to the <body> and <html> nodes', () => {
document.body.appendChild(forceScrollElement);
window.scrollTo(0, 500);

// In the iOS simulator (BrowserStack & SauceLabs), adding the content to the
// body causes karma's iframe for the test to stretch to fit that content once we attempt to
// scroll the page. Setting width / height / maxWidth / maxHeight on the iframe does not
// successfully constrain its size. As such, skip assertions in environments where the
// window size has changed since the start of the test.
if (window.innerHeight > startingWindowHeight) {
return;
}

let bodyCSS = document.body.style;
let htmlCSS = document.documentElement.style;

service.activate();

expect(bodyCSS.position).toBe('fixed');
expect(bodyCSS.width).toBe('100%');
expect(bodyCSS.top).toBe('-500px');
expect(bodyCSS.maxWidth).toBeTruthy();
expect(htmlCSS.overflowY).toBe('scroll');
});

it('should revert any previously-set inline styles', () => {
let bodyCSS = document.body.style;
let htmlCSS = document.documentElement.style;

document.body.appendChild(forceScrollElement);

bodyCSS.position = 'static';
bodyCSS.width = '1000px';
htmlCSS.overflowY = 'hidden';

service.activate();
service.deactivate();

expect(bodyCSS.position).toBe('static');
expect(bodyCSS.width).toBe('1000px');
expect(htmlCSS.overflowY).toBe('hidden');

bodyCSS.cssText = '';
htmlCSS.cssText = '';
});

it('should restore the scroll position when enabling scrolling', () => {
document.body.appendChild(forceScrollElement);
window.scrollTo(0, 1000);

// In the iOS simulator (BrowserStack & SauceLabs), adding the content to the
// body causes karma's iframe for the test to stretch to fit that content once we attempt to
// scroll the page. Setting width / height / maxWidth / maxHeight on the iframe does not
// successfully constrain its size. As such, skip assertions in environments where the
// window size has changed since the start of the test.
if (window.innerHeight > startingWindowHeight) {
return;
}

service.activate();
service.deactivate();

expect(window.pageYOffset).toBe(1000);
});
});
74 changes: 74 additions & 0 deletions src/lib/core/overlay/disable-body-scroll.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import {Injectable, Optional, SkipSelf} from '@angular/core';
import {ViewportRuler} from './position/viewport-ruler';


/**
* 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?

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?

private _htmlStyles: string = '';
private _previousScrollPosition: number = 0;
private _isActive: boolean = false;

/** Whether scrolling is disabled. */
public get isActive(): boolean {
return this._isActive;
}

constructor(private _viewportRuler: ViewportRuler) { }

/**
* 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.

let body = document.body;
let bodyHeight = body.scrollHeight;
let viewportHeight = this._viewportRuler.getViewportRect().height;

if (!this.isActive && bodyHeight > viewportHeight) {
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.

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

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.

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.


// 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.


this._isActive = true;
}
}

/**
* Re-enables scrolling.
*/
deactivate(): void {
if (this.isActive) {
document.body.style.cssText = this._bodyStyles;
document.documentElement.style.cssText = this._htmlStyles;
window.scroll(0, this._previousScrollPosition);
this._isActive = false;
}
}
}

export function DISABLE_BODY_SCROLL_PROVIDER_FACTORY(parentDispatcher: DisableBodyScroll,
viewportRuler: ViewportRuler) {
return parentDispatcher || new DisableBodyScroll(viewportRuler);
};

export const DISABLE_BODY_SCROLL_PROVIDER = {
provide: DisableBodyScroll,
deps: [[new Optional(), new SkipSelf(), DisableBodyScroll]],
useFactory: DISABLE_BODY_SCROLL_PROVIDER_FACTORY
};
2 changes: 2 additions & 0 deletions src/lib/core/overlay/overlay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {OverlayPositionBuilder} from './position/overlay-position-builder';
import {VIEWPORT_RULER_PROVIDER} from './position/viewport-ruler';
import {OverlayContainer, OVERLAY_CONTAINER_PROVIDER} from './overlay-container';
import {SCROLL_DISPATCHER_PROVIDER} from './scroll/scroll-dispatcher';
import {DISABLE_BODY_SCROLL_PROVIDER} from './disable-body-scroll';


/** Next overlay unique ID. */
Expand Down Expand Up @@ -96,4 +97,5 @@ export const OVERLAY_PROVIDERS: Provider[] = [
VIEWPORT_RULER_PROVIDER,
SCROLL_DISPATCHER_PROVIDER,
OVERLAY_CONTAINER_PROVIDER,
DISABLE_BODY_SCROLL_PROVIDER,
];