feat(overlay): add scroll blocking strategy #4500

crisbeto · 2017-05-12T15:04:45Z

Adds the BlockScrollStrategy which, when activated, will prevent the user from scrolling. For now it is only used in the dialog.

Relates to #4093.

Adds the `BlockScrollStrategy` which, when activated, will prevent the user from scrolling. For now it is only used in the dialog. Relates to angular#4093.

jelbourn · 2017-05-12T19:40:54Z

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

+ * Strategy that will prevent the user from scrolling while the overlay is visible.
+ */
+export class BlockScrollStrategy implements ScrollStrategy {
+  private _prevHTMLStyles = { top: null, left: null };


_previousHtmlElementStyles ?

jelbourn · 2017-05-12T19:42:52Z

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

+  let forceScrollElement: HTMLElement;
+
+  beforeEach(async(() => {
+    TestBed.configureTestingModule({ imports: [OverlayModule] }).compileComponents();


No spaces inside object literals

jelbourn · 2017-05-12T19:43:40Z

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

+    TestBed.configureTestingModule({ imports: [OverlayModule] }).compileComponents();
+  }));
+
+  beforeEach(inject([ViewportRuler], (_viewportRuler: ViewportRuler) => {


Don't use _ for function params; for inject I'm fine with just doing something like vpr

jelbourn · 2017-05-12T19:48:58Z

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

+  // we have to skip them on iOS.
+  function skipUnreliableBrowser(spec: Function) {
+    return () => {
+      if (!platform.IOS) { spec(); }


I don't want to always skip iOS, since sometimes it does pass (and I think iOS 10 has this problem less frequently than iOS 9).

I'm not a fan of skipping tests either, but I couldn't find a decent way to even scroll the page on iOS 10.3. E.g. something as simple as this doesn't work:

it('should scroll down', () => { forceScrollElement.style.height = '3000px'; window.scroll(0, 100); viewport._cacheViewportGeometry(); expect(viewport.getViewportScrollPosition().top).toBe(100); });

This renders the entire test pointless since we can't verify whether we're actually doing anything.

In that case I would call the function skipIOS and expand the comment to include that code snippet (just the window.scroll(0, 100); expect(viewport.getViewportScrollPosition().top).toBe(100); part) as reasoning for why it's skipped.

jelbourn · 2017-05-12T19:50:00Z

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

+    forceScrollElement.style.height = '3000px';
+
+    setScrollPosition(0, 100);
+    expect(viewport.getViewportScrollPosition().top).toBe(100,


Prefer breaking at the higher syntactic level, e.g.

expect(viewport.getViewportScrollPosition().top) .toBe(100, 'Expected viewport to be scrollable initially.');

jelbourn · 2017-05-12T19:52:26Z

src/lib/core/_core.scss

+  // Used when disabling global scrolling.
+  .cdk-global-scrollblock {
+    position: fixed;
+    overflow-y: scroll;


Won't adding overflow-y: scroll cause a scrollbar to always appear, even if there wasn't one there already? This could lead to an app suddenly multiple scrollbars when a, e.g., a dialog is opened.

It will, but we don't activate the strategy unless there was a scrollbar in the first place.

Should add a comment mentioning that

jelbourn · 2017-05-12T19:56:32Z

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

+    viewport._cacheViewportGeometry();
+  }
+
+});


I think we should have an e2e test w/ screenshots for this as well that actually attempts to scroll the page through user input.

I agree, but wouldn't it be pointless since we only run the e2e tests against Chrome?

It's better than not having it; there were issues w/ the AngularJS version that manifested in Chrome.

crisbeto · 2017-05-13T14:05:08Z

Added e2e tests and addressed the feedback @jelbourn.

jelbourn

LGTM, one minor comment

Also, I'm shocked that webdriver doesn't have anything to make scrolling happen other than window.scrollTo (just looked it up).

jelbourn · 2017-05-15T16:36:12Z

src/lib/core/_core.scss

@@ -41,4 +41,17 @@
  .mat-theme-loaded-marker {
    display: none;
  }
+
+  // Used when disabling global scrolling.
+  .cdk-global-scrollblock {


This should go into the cdk-overlay mixin

xtianus79 · 2017-05-25T23:30:51Z

Hi @jelbourn is this where a user is using a dialog box or sidenav(panel) flyout and the overlay content still scrolls up and down? For now I used a method that alters the body element to position: fixed when opening one of these is opened up.

tobias74 · 2017-10-23T13:50:06Z

How can I use the BlockScrollStrategy on a Select-Dropdown?

willshowell · 2017-10-23T15:58:34Z

@tobias74 answered at https://stackoverflow.com/a/46893614/4811678

angular-automatic-lock-bot · 2019-09-07T11:02:19Z

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

crisbeto added the pr: needs review label May 12, 2017

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 12, 2017

crisbeto self-assigned this May 12, 2017

feat(overlay): add scroll blocking strategy

5cd53be

Adds the `BlockScrollStrategy` which, when activated, will prevent the user from scrolling. For now it is only used in the dialog. Relates to angular#4093.

crisbeto force-pushed the scroll-block branch from b544211 to 5cd53be Compare May 12, 2017 15:42

crisbeto assigned jelbourn and unassigned crisbeto May 12, 2017

crisbeto mentioned this pull request May 12, 2017

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

Closed

jelbourn reviewed May 12, 2017

View reviewed changes

chore: address feedback

caaeb8d

test: add e2e tests for the scroll blocking

c861b21

crisbeto force-pushed the scroll-block branch from 80d69d7 to c861b21 Compare May 13, 2017 14:28

jelbourn approved these changes May 15, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels May 15, 2017

chore: move scroll block class

0d65b81

crisbeto added the action: merge The PR is ready for merge by the caretaker label May 15, 2017

jelbourn merged commit 6842046 into angular:master May 15, 2017

macjohnny mentioned this pull request May 17, 2017

Feature(MdSidenav): add scroll block strategy #4625

Closed

axtho mentioned this pull request Oct 19, 2017

autocomplete: panel does not reposition on page scroll #7897

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

feat(overlay): add scroll blocking strategy #4500

feat(overlay): add scroll blocking strategy #4500

Uh oh!

Conversation

crisbeto commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto commented May 13, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtianus79 commented May 25, 2017

Uh oh!

tobias74 commented Oct 23, 2017

Uh oh!

willshowell commented Oct 23, 2017

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Uh oh!