feat(overlay): add scroll handling strategies #4293

crisbeto · 2017-04-26T21:00:37Z

Adds the scrollStrategy option to the overlay state, allowing the consumer to specify what scroll handling strategy they'd want to use. Also includes a ScrollStrategy interface that users can utilize to build their own strategies.
Adds the RepositionScrollStrategy, CloseScrollStrategy and NoopScrollStrategy as initial, out-of-the-box strategies.
Sets the RepositionScrollStrategy by default on all the connected overlays and removes some repetitive logic from the tooltip, autocomplete, menu and select.

Note: I'll add a BlockScrollStrategy in a follow-up PR. I wanted to keep this one shorter.

Relates to #4093.

jelbourn · 2017-04-26T21:38:48Z

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

+  private _scrollSubscription: Subscription|null = null;
+  private _overlayRef: OverlayRef;
+
+  constructor(private _scrollDispatcher, private _scrollThrottle = 0) { }


Missing type on _scrollDispatcher

jelbourn · 2017-04-26T21:45:17Z

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

+  private _scrollSubscription: Subscription|null = null;
+  private _overlayRef: OverlayRef;
+
+  constructor(private _scrollDispatcher) { }


Missing type on _scrollDipstacher?

jelbourn · 2017-04-26T21:57:39Z

src/lib/core/overlay/overlay.spec.ts

+      fakeScrollStrategy = {
+        attach: jasmine.createSpy('attach spy'),
+        enable: jasmine.createSpy('enable spy'),
+        disable: jasmine.createSpy('disable spy')


Rather than having spies, it would be slightly more accurate to just store attachedOverlay and set isEnabled and make assertions against those.

(with a spy technically you could do something like call enabled(); disabled(); and have it still pass)

The main reason I avoided adding isAttached and isEnabled was that I wanted to keep the ScrollStrategy interface as lean as possible. Also, apart from unit tests, I'm not sure that they provide a lot of value.

I meant only in the fake implementation for tests

Ah, got it. I've updated the tests.

jelbourn · 2017-04-26T22:02:18Z

src/lib/menu/menu-trigger.ts

@@ -218,6 +220,7 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
    overlayState.hasBackdrop = true;
    overlayState.backdropClass = 'cdk-overlay-transparent-backdrop';
    overlayState.direction = this.dir;
+    overlayState.scrollStrategy = new RepositionScrollStrategy(this._scrollDispatcher);


For a follow-up PR: rather than having menu, select, etc. all directly use RepositionScrollStrategy, we should have a provider that can configure the behavior more globally.

Sounds good, although we'd have to have at least two: one for the global overlays and one for the connected ones.

jelbourn · 2017-04-26T22:04:14Z

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

+    if (!this._scrollSubscription) {
+      this._scrollSubscription = this._scrollDispatcher.scrolled(null, () => {
+        if (this._overlayRef.hasAttached()) {
+          this._overlayRef.detach();


It doesn't look like anything communicates the fact that the overlay detached back to the component, which would make their own state be out of sync.

The OverlayRef calls the disable method when it detaches and when it is disposed: https://github.com/angular/material2/pull/4293/files#diff-d4ca44eeaacca5fbb90029f947afc1e0R82

I meant the other way around- for example, if the overlay for MdMenuTrigger is closed due to scrolling, nothing tells MdMenuTrigger that this has happened and its _menuOpen state will be wrong

Got it. We should add a detach observable to the OverlayRef IMO. We already do it on the ConnectedOverlayDirective.

Sounds reasonable to me

Added the observables as discussed. They're not being used anywhere right now since nothing uses the CloseScrollStrategy. Also I initially wanted to have the outputs on the ConnectedOverlayDirective proxy the ones on the overlay ref, but it's not possible since the ref gets created lazily before attaching.

jelbourn · 2017-04-28T18:52:35Z

src/lib/core/overlay/overlay-ref.ts

+  /** Returns an observable that emits when the overlay has been detached. */
+  onDetach(): Observable<void> {
+    return this._onDetach.asObservable();
+  }


Can we make these getters called attachments and detachments? I kind of want to move away from the onAction-style naming for observables

jelbourn

LGTM

mmalerba · 2017-04-28T23:10:25Z

@crisbeto needs rebase

* Adds the `scrollStrategy` option to the overlay state, allowing the consumer to specify what scroll handling strategy they'd want to use. Also includes a `ScrollStrategy` interface that users can utilize to build their own strategies. * Adds the `RepositionScrollStrategy`, `CloseScrollStrategy` and `NoopScrollStrategy` as initial, out-of-the-box strategies. * Sets the `RepositionScrollStrategy` by default on all the connected overlays and removes some repetitive logic from the tooltip, autocomplete, menu and select. **Note:** I'll add a `BlockScrollStrategy` in a follow-up PR. I wanted to keep this one shorter. Relates to angular#4093.

YeomansIII · 2017-05-22T23:30:05Z

How does one use these scroll strategies on a MdDialog? Does that require a separate change?

crisbeto · 2017-05-23T04:25:38Z

The dialog currently has a scroll strategy to disable scrolling. We're still working on a way to be able to override the strategy per component.

angular-automatic-lock-bot · 2019-09-06T13:01: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 Apr 26, 2017

crisbeto assigned jelbourn Apr 26, 2017

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

jelbourn reviewed Apr 26, 2017

View reviewed changes

jelbourn reviewed Apr 28, 2017

View reviewed changes

jelbourn approved these changes Apr 28, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Apr 28, 2017

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Apr 28, 2017

crisbeto added 5 commits April 29, 2017 11:15

fix: missing types on the scroll dispatcher

a87a542

refactor: use class for fake scroll strategy

e86219c

refactor: add onAttached and onDetached observables

9cc802f

chore: rename observables

8558bae

crisbeto force-pushed the 4093/scroll-strategy branch from 031e4d7 to 8558bae Compare April 29, 2017 09:15

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

andrewseguin merged commit c8ec981 into angular:master May 2, 2017

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 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(overlay): add scroll handling strategies #4293

feat(overlay): add scroll handling strategies #4293

crisbeto commented Apr 26, 2017

jelbourn Apr 26, 2017

jelbourn Apr 26, 2017

jelbourn Apr 26, 2017

crisbeto Apr 27, 2017 •

edited

jelbourn Apr 27, 2017

crisbeto Apr 27, 2017 •

edited

jelbourn Apr 26, 2017 •

edited

crisbeto Apr 27, 2017

jelbourn Apr 26, 2017

crisbeto Apr 27, 2017 •

edited

jelbourn Apr 27, 2017 •

edited

crisbeto Apr 27, 2017 •

edited

jelbourn Apr 28, 2017

crisbeto Apr 28, 2017

jelbourn Apr 28, 2017

crisbeto Apr 28, 2017

jelbourn left a comment

mmalerba commented Apr 28, 2017

YeomansIII commented May 22, 2017

crisbeto commented May 23, 2017

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

feat(overlay): add scroll handling strategies #4293

feat(overlay): add scroll handling strategies #4293

Conversation

crisbeto commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Apr 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Apr 27, 2017 • edited

Choose a reason for hiding this comment

jelbourn Apr 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto Apr 27, 2017 • edited

Choose a reason for hiding this comment

jelbourn Apr 27, 2017 • edited

Choose a reason for hiding this comment

crisbeto Apr 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

mmalerba commented Apr 28, 2017

YeomansIII commented May 22, 2017

crisbeto commented May 23, 2017

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

crisbeto Apr 27, 2017 •

edited

crisbeto Apr 27, 2017 •

edited

jelbourn Apr 26, 2017 •

edited

crisbeto Apr 27, 2017 •

edited

jelbourn Apr 27, 2017 •

edited

crisbeto Apr 27, 2017 •

edited