Skip to content

Commit caaeb8d

Browse files
crisbetocrisbeto
committed
chore: address feedback
1 parent 5cd53be commit caaeb8d

File tree

3 files changed

+67
-55
lines changed

3 files changed

+67
-55
lines changed

‎src/lib/core/_core.scss

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,13 @@
4545
// Used when disabling global scrolling.
4646
.cdk-global-scrollblock {
4747
position: fixed;
48+
49+
// Necessary for iOS not to expand past the viewport.
50+
max-width: 100vw;
51+
52+
// Note: this will always add a scrollbar to whatever element it is on, which can
53+
// potentially result in double scrollbars. It shouldn't be an issue, because we won't
54+
// block scrolling on a page that doesn't have a scrollbar in the first place.
4855
overflow-y: scroll;
49-
max-width: 100vw; // necessary for iOS not to expand past the viewport.
5056
}
5157
}

‎src/lib/core/overlay/scroll/block-scroll-strategy.spec.ts

Lines changed: 50 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@ describe('BlockScrollStrategy', () => {
1010
let forceScrollElement: HTMLElement;
1111

1212
beforeEach(async(() => {
13-
TestBed.configureTestingModule({ imports: [OverlayModule] }).compileComponents();
13+
TestBed.configureTestingModule({imports: [OverlayModule]}).compileComponents();
1414
}));
1515

16-
beforeEach(inject([ViewportRuler], (_viewportRuler: ViewportRuler) => {
17-
strategy = new BlockScrollStrategy(_viewportRuler);
18-
viewport = _viewportRuler;
16+
beforeEach(inject([ViewportRuler], (viewportRuler: ViewportRuler) => {
17+
strategy = new BlockScrollStrategy(viewportRuler);
18+
viewport = viewportRuler;
1919
forceScrollElement = document.createElement('div');
2020
document.body.appendChild(forceScrollElement);
2121
forceScrollElement.style.width = '100px';
@@ -27,59 +27,56 @@ describe('BlockScrollStrategy', () => {
2727
setScrollPosition(0, 0);
2828
});
2929

30-
it('should toggle scroll blocking along the y axis', skipUnreliableBrowser(() => {
31-
forceScrollElement.style.height = '3000px';
32-
30+
it('should toggle scroll blocking along the y axis', skipIOS(() => {
3331
setScrollPosition(0, 100);
34-
expect(viewport.getViewportScrollPosition().top).toBe(100,
35-
'Expected viewport to be scrollable initially.');
32+
expect(viewport.getViewportScrollPosition().top)
33+
.toBe(100, 'Expected viewport to be scrollable initially.');
3634

3735
strategy.enable();
38-
expect(document.documentElement.style.top).toBe('-100px',
39-
'Expected <html> element to be offset by the previous scroll amount along the y axis.');
36+
expect(document.documentElement.style.top)
37+
.toBe('-100px', 'Expected <html> element to be offset by the previous scroll amount.');
4038

4139
setScrollPosition(0, 300);
42-
expect(viewport.getViewportScrollPosition().top).toBe(100,
43-
'Expected the viewport not to scroll.');
40+
expect(viewport.getViewportScrollPosition().top)
41+
.toBe(100, 'Expected the viewport not to scroll.');
4442

4543
strategy.disable();
46-
expect(viewport.getViewportScrollPosition().top).toBe(100,
47-
'Expected old scroll position to have bee restored after disabling.');
44+
expect(viewport.getViewportScrollPosition().top)
45+
.toBe(100, 'Expected old scroll position to have bee restored after disabling.');
4846

4947
setScrollPosition(0, 300);
50-
expect(viewport.getViewportScrollPosition().top).toBe(300,
51-
'Expected user to be able to scroll after disabling.');
48+
expect(viewport.getViewportScrollPosition().top)
49+
.toBe(300, 'Expected user to be able to scroll after disabling.');
5250
}));
5351

5452

55-
it('should toggle scroll blocking along the x axis', skipUnreliableBrowser(() => {
53+
it('should toggle scroll blocking along the x axis', skipIOS(() => {
54+
forceScrollElement.style.height = '100px';
5655
forceScrollElement.style.width = '3000px';
5756

5857
setScrollPosition(100, 0);
59-
expect(viewport.getViewportScrollPosition().left).toBe(100,
60-
'Expected viewport to be scrollable initially.');
58+
expect(viewport.getViewportScrollPosition().left)
59+
.toBe(100, 'Expected viewport to be scrollable initially.');
6160

6261
strategy.enable();
63-
expect(document.documentElement.style.left).toBe('-100px',
64-
'Expected <html> element to be offset by the previous scroll amount along the x axis.');
62+
expect(document.documentElement.style.left)
63+
.toBe('-100px', 'Expected <html> element to be offset by the previous scroll amount.');
6564

6665
setScrollPosition(300, 0);
67-
expect(viewport.getViewportScrollPosition().left).toBe(100,
68-
'Expected the viewport not to scroll.');
66+
expect(viewport.getViewportScrollPosition().left)
67+
.toBe(100, 'Expected the viewport not to scroll.');
6968

7069
strategy.disable();
71-
expect(viewport.getViewportScrollPosition().left).toBe(100,
72-
'Expected old scroll position to have bee restored after disabling.');
70+
expect(viewport.getViewportScrollPosition().left)
71+
.toBe(100, 'Expected old scroll position to have bee restored after disabling.');
7372

7473
setScrollPosition(300, 0);
75-
expect(viewport.getViewportScrollPosition().left).toBe(300,
76-
'Expected user to be able to scroll after disabling.');
74+
expect(viewport.getViewportScrollPosition().left)
75+
.toBe(300, 'Expected user to be able to scroll after disabling.');
7776
}));
7877

7978

80-
it('should toggle the `cdk-global-scrollblock` class', skipUnreliableBrowser(() => {
81-
forceScrollElement.style.height = '3000px';
82-
79+
it('should toggle the `cdk-global-scrollblock` class', skipIOS(() => {
8380
expect(document.documentElement.classList).not.toContain('cdk-global-scrollblock');
8481

8582
strategy.enable();
@@ -89,10 +86,9 @@ describe('BlockScrollStrategy', () => {
8986
expect(document.documentElement.classList).not.toContain('cdk-global-scrollblock');
9087
}));
9188

92-
it('should restore any previously-set inline styles', skipUnreliableBrowser(() => {
89+
it('should restore any previously-set inline styles', skipIOS(() => {
9390
const root = document.documentElement;
9491

95-
forceScrollElement.style.height = '3000px';
9692
root.style.top = '13px';
9793
root.style.left = '37px';
9894

@@ -107,24 +103,37 @@ describe('BlockScrollStrategy', () => {
107103
expect(root.style.left).toBe('37px');
108104
}));
109105

110-
it(`should't do anything if the page isn't scrollable`, skipUnreliableBrowser(() => {
106+
it(`should't do anything if the page isn't scrollable`, skipIOS(() => {
111107
forceScrollElement.style.display = 'none';
112108
strategy.enable();
113109
expect(document.documentElement.classList).not.toContain('cdk-global-scrollblock');
114110
}));
115111

116112

117-
// In the iOS simulator (BrowserStack & SauceLabs), adding content to the
118-
// body causes karma's iframe for the test to stretch to fit that content,
119-
// in addition to not allowing us to set the scroll position programmatically.
120-
// This renders the tests unusable and since we can't really do anything about it,
121-
// we have to skip them on iOS.
122-
function skipUnreliableBrowser(spec: Function) {
113+
/**
114+
* Skips the specified test, if it is being executed on iOS. This is necessary, because
115+
* programmatic scrolling inside the Karma iframe doesn't work on iOS, which renders these
116+
* tests unusable. For example, something as basic as the following won't work:
117+
* ```
118+
* window.scroll(0, 100);
119+
* viewport._cacheViewportGeometry();
120+
* expect(viewport.getViewportScrollPosition().top).toBe(100);
121+
* ```
122+
* @param spec Test to be executed or skipped.
123+
*/
124+
function skipIOS(spec: Function) {
123125
return () => {
124-
if (!platform.IOS) { spec(); }
126+
if (!platform.IOS) {
127+
spec();
128+
}
125129
};
126130
}
127131

132+
/**
133+
* Scrolls the viewport and clears the cache.
134+
* @param x Amount to scroll along the x axis.
135+
* @param y Amount to scroll along the y axis.
136+
*/
128137
function setScrollPosition(x: number, y: number) {
129138
window.scroll(x, y);
130139
viewport._cacheViewportGeometry();

‎src/lib/core/overlay/scroll/block-scroll-strategy.ts

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import {ViewportRuler} from '../position/viewport-ruler';
55
* Strategy that will prevent the user from scrolling while the overlay is visible.
66
*/
77
export class BlockScrollStrategy implements ScrollStrategy {
8-
private _prevHTMLStyles = { top: null, left: null };
8+
private _previousHTMLStyles = { top: null, left: null };
99
private _previousScrollPosition: { top: number, left: number };
1010
private _isEnabled = false;
1111

@@ -20,8 +20,8 @@ export class BlockScrollStrategy implements ScrollStrategy {
2020
this._previousScrollPosition = this._viewportRuler.getViewportScrollPosition();
2121

2222
// Cache the previous inline styles in case the user had set them.
23-
this._prevHTMLStyles.left = root.style.left;
24-
this._prevHTMLStyles.top = root.style.top;
23+
this._previousHTMLStyles.left = root.style.left;
24+
this._previousHTMLStyles.top = root.style.top;
2525

2626
// Note: we're using the `html` node, instead of the `body`, because the `body` may
2727
// have the user agent margin, whereas the `html` is guaranteed not to have one.
@@ -35,8 +35,8 @@ export class BlockScrollStrategy implements ScrollStrategy {
3535
disable() {
3636
if (this._isEnabled) {
3737
this._isEnabled = false;
38-
document.documentElement.style.left = this._prevHTMLStyles.left;
39-
document.documentElement.style.top = this._prevHTMLStyles.top;
38+
document.documentElement.style.left = this._previousHTMLStyles.left;
39+
document.documentElement.style.top = this._previousHTMLStyles.top;
4040
document.documentElement.classList.remove('cdk-global-scrollblock');
4141
window.scroll(this._previousScrollPosition.left, this._previousScrollPosition.top);
4242
}
@@ -46,15 +46,12 @@ export class BlockScrollStrategy implements ScrollStrategy {
4646
// Since the scroll strategies can't be singletons, we have to use a global CSS class
4747
// (`cdk-global-scrollblock`) to make sure that we don't try to disable global
4848
// scrolling multiple times.
49-
const isBlockedAlready =
50-
document.documentElement.classList.contains('cdk-global-scrollblock') || this._isEnabled;
51-
52-
if (!isBlockedAlready) {
53-
const body = document.body;
54-
const viewport = this._viewportRuler.getViewportRect();
55-
return body.scrollHeight > viewport.height || body.scrollWidth > viewport.width;
49+
if (document.documentElement.classList.contains('cdk-global-scrollblock') || this._isEnabled) {
50+
return false;
5651
}
5752

58-
return false;
53+
const body = document.body;
54+
const viewport = this._viewportRuler.getViewportRect();
55+
return body.scrollHeight > viewport.height || body.scrollWidth > viewport.width;
5956
}
6057
}

0 commit comments

Comments
 (0)