From 878465d85ea88536037f4d7cf5219874a7d0a608 Mon Sep 17 00:00:00 2001 From: Kara Date: Fri, 10 Feb 2017 17:31:25 -0800 Subject: [PATCH] Revert "feat(tooltip): reposition on scroll" --- src/demo-app/tooltip/tooltip-demo.html | 6 +- src/demo-app/tooltip/tooltip-demo.scss | 6 -- .../overlay/scroll/scroll-dispatcher.spec.ts | 23 ++---- .../core/overlay/scroll/scroll-dispatcher.ts | 20 ++--- src/lib/sidenav/sidenav-container.html | 2 +- src/lib/tooltip/tooltip.spec.ts | 78 +------------------ src/lib/tooltip/tooltip.ts | 35 +-------- tools/gulp/tasks/components.ts | 1 - 8 files changed, 20 insertions(+), 151 deletions(-) diff --git a/src/demo-app/tooltip/tooltip-demo.html b/src/demo-app/tooltip/tooltip-demo.html index 15025d7a1df3..79691037bea0 100644 --- a/src/demo-app/tooltip/tooltip-demo.html +++ b/src/demo-app/tooltip/tooltip-demo.html @@ -1,7 +1,7 @@

Tooltip Demo

-
+

-

Scroll down while tooltip is open to see it hide automatically
-
-
+

diff --git a/src/demo-app/tooltip/tooltip-demo.scss b/src/demo-app/tooltip/tooltip-demo.scss index c2483c856273..d3ff6bac1a6c 100644 --- a/src/demo-app/tooltip/tooltip-demo.scss +++ b/src/demo-app/tooltip/tooltip-demo.scss @@ -1,12 +1,6 @@ .demo-tooltip { .centered { text-align: center; - height: 200px; - overflow: auto; - - button { - margin: 16px; - } } .mat-radio-button { display: block; diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts index 77342af30f20..f342d56cb10d 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.spec.ts @@ -1,5 +1,5 @@ -import {inject, TestBed, async, fakeAsync, ComponentFixture, tick} from '@angular/core/testing'; -import {NgModule, Component, ViewChild, ElementRef} from '@angular/core'; +import {inject, TestBed, async, ComponentFixture} from '@angular/core/testing'; +import {NgModule, Component, ViewChild, ElementRef, QueryList, ViewChildren} from '@angular/core'; import {ScrollDispatcher} from './scroll-dispatcher'; import {OverlayModule} from '../overlay-directives'; import {Scrollable} from './scrollable'; @@ -38,17 +38,15 @@ describe('Scroll Dispatcher', () => { expect(scroll.scrollableReferences.has(componentScrollable)).toBe(false); }); - it('should notify through the directive and service that a scroll event occurred', - fakeAsync(() => { + it('should notify through the directive and service that a scroll event occurred', () => { let hasDirectiveScrollNotified = false; // Listen for notifications from scroll directive let scrollable = fixture.componentInstance.scrollable; scrollable.elementScrolled().subscribe(() => { hasDirectiveScrollNotified = true; }); - // Listen for notifications from scroll service with a throttle of 100ms - const throttleTime = 100; + // Listen for notifications from scroll service let hasServiceScrollNotified = false; - scroll.scrolled(throttleTime).subscribe(() => { hasServiceScrollNotified = true; }); + scroll.scrolled().subscribe(() => { hasServiceScrollNotified = true; }); // Emit a scroll event from the scrolling element in our component. // This event should be picked up by the scrollable directive and notify. @@ -57,17 +55,9 @@ describe('Scroll Dispatcher', () => { scrollEvent.initUIEvent('scroll', true, true, window, 0); fixture.componentInstance.scrollingElement.nativeElement.dispatchEvent(scrollEvent); - // The scrollable directive should have notified the service immediately. expect(hasDirectiveScrollNotified).toBe(true); - - // Verify that the throttle is used, the service should wait for the throttle time until - // sending the notification. - expect(hasServiceScrollNotified).toBe(false); - - // After the throttle time, the notification should be sent. - tick(throttleTime); expect(hasServiceScrollNotified).toBe(true); - })); + }); }); describe('Nested scrollables', () => { @@ -117,6 +107,7 @@ class ScrollingComponent { }) class NestedScrollingComponent { @ViewChild('interestingElement') interestingElement: ElementRef; + @ViewChildren(Scrollable) scrollables: QueryList; } const TEST_COMPONENTS = [ScrollingComponent, NestedScrollingComponent]; diff --git a/src/lib/core/overlay/scroll/scroll-dispatcher.ts b/src/lib/core/overlay/scroll/scroll-dispatcher.ts index 15cefb2d2443..e4f8b3864713 100644 --- a/src/lib/core/overlay/scroll/scroll-dispatcher.ts +++ b/src/lib/core/overlay/scroll/scroll-dispatcher.ts @@ -4,12 +4,8 @@ import {Subject} from 'rxjs/Subject'; import {Observable} from 'rxjs/Observable'; import {Subscription} from 'rxjs/Subscription'; import 'rxjs/add/observable/fromEvent'; -import 'rxjs/add/operator/auditTime'; -/** Time in ms to throttle the scrolling events by default. */ -export const DEFAULT_SCROLL_TIME = 20; - /** * Service contained all registered Scrollable references and emits an event when any one of the * Scrollable references emit a scrolled event. @@ -54,17 +50,11 @@ export class ScrollDispatcher { /** * Returns an observable that emits an event whenever any of the registered Scrollable - * references (or window, document, or body) fire a scrolled event. Can provide a time in ms - * to override the default "throttle" time. + * references (or window, document, or body) fire a scrolled event. */ - scrolled(auditTimeInMs: number = DEFAULT_SCROLL_TIME): Observable { - // In the case of a 0ms delay, return the observable without auditTime since it does add - // a perceptible delay in processing overhead. - if (auditTimeInMs == 0) { - return this._scrolled.asObservable(); - } - - return this._scrolled.asObservable().auditTime(auditTimeInMs); + scrolled(): Observable { + // TODO: Add an event limiter that includes throttle with the leading and trailing events. + return this._scrolled.asObservable(); } /** Returns all registered Scrollables that contain the provided element. */ @@ -100,7 +90,7 @@ export class ScrollDispatcher { export function SCROLL_DISPATCHER_PROVIDER_FACTORY(parentDispatcher: ScrollDispatcher) { return parentDispatcher || new ScrollDispatcher(); -} +}; export const SCROLL_DISPATCHER_PROVIDER = { // If there is already a ScrollDispatcher available, use that. Otherwise, provide a new one. diff --git a/src/lib/sidenav/sidenav-container.html b/src/lib/sidenav/sidenav-container.html index 24f0789dcde2..75eb4b037ab8 100644 --- a/src/lib/sidenav/sidenav-container.html +++ b/src/lib/sidenav/sidenav-container.html @@ -3,6 +3,6 @@ -

+
diff --git a/src/lib/tooltip/tooltip.spec.ts b/src/lib/tooltip/tooltip.spec.ts index 5873f340f0e6..7305715ca920 100644 --- a/src/lib/tooltip/tooltip.spec.ts +++ b/src/lib/tooltip/tooltip.spec.ts @@ -10,15 +10,13 @@ import { Component, DebugElement, AnimationTransitionEvent, - ViewChild, ChangeDetectionStrategy } from '@angular/core'; import {By} from '@angular/platform-browser'; -import {TooltipPosition, MdTooltip, MdTooltipModule, SCROLL_THROTTLE_MS} from './tooltip'; +import {TooltipPosition, MdTooltip, MdTooltipModule} from './tooltip'; import {OverlayContainer} from '../core'; import {Dir, LayoutDirection} from '../core/rtl/dir'; import {OverlayModule} from '../core/overlay/overlay-directives'; -import {Scrollable} from '../core/overlay/scroll/scrollable'; const initialTooltipMessage = 'initial tooltip message'; @@ -29,11 +27,10 @@ describe('MdTooltip', () => { beforeEach(async(() => { TestBed.configureTestingModule({ imports: [MdTooltipModule.forRoot(), OverlayModule], - declarations: [BasicTooltipDemo, ScrollableTooltipDemo, OnPushTooltipDemo], + declarations: [BasicTooltipDemo, OnPushTooltipDemo], providers: [ {provide: OverlayContainer, useFactory: () => { overlayContainerElement = document.createElement('div'); - document.body.appendChild(overlayContainerElement); return {getContainerElement: () => overlayContainerElement}; }}, {provide: Dir, useFactory: () => { @@ -315,43 +312,6 @@ describe('MdTooltip', () => { }); }); - describe('scrollable usage', () => { - let fixture: ComponentFixture; - let buttonDebugElement: DebugElement; - let buttonElement: HTMLButtonElement; - let tooltipDirective: MdTooltip; - - beforeEach(() => { - fixture = TestBed.createComponent(ScrollableTooltipDemo); - fixture.detectChanges(); - buttonDebugElement = fixture.debugElement.query(By.css('button')); - buttonElement = buttonDebugElement.nativeElement; - tooltipDirective = buttonDebugElement.injector.get(MdTooltip); - }); - - it('should hide tooltip if clipped after changing positions', fakeAsync(() => { - expect(tooltipDirective._tooltipInstance).toBeUndefined(); - - // Show the tooltip and tick for the show delay (default is 0) - tooltipDirective.show(); - fixture.detectChanges(); - tick(0); - - // Expect that the tooltip is displayed - expect(tooltipDirective._isTooltipVisible()).toBe(true); - - // Scroll the page but tick just before the default throttle should update. - fixture.componentInstance.scrollDown(); - tick(SCROLL_THROTTLE_MS - 1); - expect(tooltipDirective._isTooltipVisible()).toBe(true); - - // Finish ticking to the throttle's limit and check that the scroll event notified the - // tooltip and it was hidden. - tick(1); - expect(tooltipDirective._isTooltipVisible()).toBe(false); - })); - }); - describe('with OnPush', () => { let fixture: ComponentFixture; let buttonDebugElement: DebugElement; @@ -414,39 +374,6 @@ class BasicTooltipDemo { message: string = initialTooltipMessage; showButton: boolean = true; } - -@Component({ - selector: 'app', - template: ` -
- -
` -}) -class ScrollableTooltipDemo { - position: string = 'below'; - message: string = initialTooltipMessage; - showButton: boolean = true; - - @ViewChild(Scrollable) scrollingContainer: Scrollable; - - scrollDown() { - const scrollingContainerEl = this.scrollingContainer.getElementRef().nativeElement; - scrollingContainerEl.scrollTop = 250; - - // Emit a scroll event from the scrolling element in our component. - // This event should be picked up by the scrollable directive and notify. - // The notification should be picked up by the service. - const scrollEvent = document.createEvent('UIEvents'); - scrollEvent.initUIEvent('scroll', true, true, window, 0); - scrollingContainerEl.dispatchEvent(scrollEvent); - } -} - @Component({ selector: 'app', template: ` @@ -460,3 +387,4 @@ class OnPushTooltipDemo { position: string = 'below'; message: string = initialTooltipMessage; } + diff --git a/src/lib/tooltip/tooltip.ts b/src/lib/tooltip/tooltip.ts index 7b27abaa3968..cc8732d588d0 100644 --- a/src/lib/tooltip/tooltip.ts +++ b/src/lib/tooltip/tooltip.ts @@ -15,7 +15,6 @@ import { NgZone, Optional, OnDestroy, - OnInit, ChangeDetectorRef } from '@angular/core'; import { @@ -26,24 +25,19 @@ import { ComponentPortal, OverlayConnectionPosition, OriginConnectionPosition, - CompatibilityModule + CompatibilityModule, } from '../core'; import {MdTooltipInvalidPositionError} from './tooltip-errors'; import {Observable} from 'rxjs/Observable'; import {Subject} from 'rxjs/Subject'; import {Dir} from '../core/rtl/dir'; import 'rxjs/add/operator/first'; -import {ScrollDispatcher} from '../core/overlay/scroll/scroll-dispatcher'; -import {Subscription} from 'rxjs/Subscription'; export type TooltipPosition = 'left' | 'right' | 'above' | 'below' | 'before' | 'after'; /** Time in ms to delay before changing the tooltip visibility to hidden */ export const TOUCHEND_HIDE_DELAY = 1500; -/** Time in ms to throttle repositioning after scroll events. */ -export const SCROLL_THROTTLE_MS = 20; - /** * Directive that attaches a material design tooltip to the host element. Animates the showing and * hiding of a tooltip provided position (defaults to below the element). @@ -60,10 +54,9 @@ export const SCROLL_THROTTLE_MS = 20; }, exportAs: 'mdTooltip', }) -export class MdTooltip implements OnInit, OnDestroy { +export class MdTooltip implements OnDestroy { _overlayRef: OverlayRef; _tooltipInstance: TooltipComponent; - scrollSubscription: Subscription; private _position: TooltipPosition = 'below'; @@ -130,22 +123,11 @@ export class MdTooltip implements OnInit, OnDestroy { set _matShowDelay(v) { this.showDelay = v; } constructor(private _overlay: Overlay, - private _scrollDispatcher: ScrollDispatcher, private _elementRef: ElementRef, private _viewContainerRef: ViewContainerRef, private _ngZone: NgZone, @Optional() private _dir: Dir) { } - ngOnInit() { - // When a scroll on the page occurs, update the position in case this tooltip needs - // to be repositioned. - this.scrollSubscription = this._scrollDispatcher.scrolled(SCROLL_THROTTLE_MS).subscribe(() => { - if (this._overlayRef) { - this._overlayRef.updatePosition(); - } - }); - } - /** * Dispose the tooltip when destroyed. */ @@ -153,8 +135,6 @@ export class MdTooltip implements OnInit, OnDestroy { if (this._tooltipInstance) { this._disposeTooltip(); } - - this.scrollSubscription.unsubscribe(); } /** Shows the tooltip after the delay in ms, defaults to tooltip-delay-show or 0ms if no input */ @@ -205,18 +185,7 @@ export class MdTooltip implements OnInit, OnDestroy { private _createOverlay(): void { let origin = this._getOrigin(); let position = this._getOverlayPosition(); - - // Create connected position strategy that listens for scroll events to reposition. - // After position changes occur and the overlay is clipped by a parent scrollable then - // close the tooltip. let strategy = this._overlay.position().connectedTo(this._elementRef, origin, position); - strategy.withScrollableContainers(this._scrollDispatcher.getScrollContainers(this._elementRef)); - strategy.onPositionChange.subscribe(change => { - if (change.scrollableViewProperties.isOverlayClipped && - this._tooltipInstance && this._tooltipInstance.isVisible()) { - this.hide(0); - } - }); let config = new OverlayState(); config.positionStrategy = strategy; diff --git a/tools/gulp/tasks/components.ts b/tools/gulp/tasks/components.ts index f2cc6030ef45..bf00567ebef9 100644 --- a/tools/gulp/tasks/components.ts +++ b/tools/gulp/tasks/components.ts @@ -77,7 +77,6 @@ task(':build:components:rollup', () => { 'rxjs/add/observable/of': 'Rx.Observable', 'rxjs/add/observable/merge': 'Rx.Observable', 'rxjs/add/observable/throw': 'Rx.Observable', - 'rxjs/add/operator/auditTime': 'Rx.Observable.prototype', 'rxjs/add/operator/toPromise': 'Rx.Observable.prototype', 'rxjs/add/operator/map': 'Rx.Observable.prototype', 'rxjs/add/operator/filter': 'Rx.Observable.prototype',