fix(material-experimental/mdc-slider): fix change and input events on the mdc slider

* use #fromEvent to simplify the global change and input listener
* go back to dispatching real events instead of using Angular's event emitter system
* fix how the global change and input listener is used in the slider adapter
* Rename fake event indicator boolean
* Simplified change & input event logic to make things more readable

Author: wagnermaciel

src/material-experimental/mdc-slider/global-change-and-input-listener.ts

@@ -7,64 +7,61 @@
  */
 
 import {DOCUMENT} from '@angular/common';
-import {Inject, Injectable, NgZone} from '@angular/core';
+import {Inject, Injectable, NgZone, OnDestroy} from '@angular/core';
 import {SpecificEventListener} from '@material/base';
-import {Subject, Subscription} from 'rxjs';
-import {finalize} from 'rxjs/operators';
+import {fromEvent, Observable, Subject, Subscription} from 'rxjs';
+import {finalize, share, takeUntil} from 'rxjs/operators';
 
 /**
  * Handles listening for all change and input events that occur on the document.
  *
  * This service exposes a single method #listen to allow users to subscribe to change and input
- * events that occur on the document. Since listening for these events on the document can be
- * expensive, we lazily attach listeners to the document when the first subscription is made, and
- * remove the listeners once the last observer unsubscribes.
+ * events that occur on the document. Since listening for these events can be expensive, we use
+ * #fromEvent which will lazily attach a listener when the first subscription is made and remove the
+ * listener once the last observer unsubscribes.
  */
 @Injectable({providedIn: 'root'})
-export class GlobalChangeAndInputListener<K extends 'change'|'input'> {
+export class GlobalChangeAndInputListener<K extends 'change'|'input'> implements OnDestroy {
 
   /** The injected document if available or fallback to the global document reference. */
   private _document: Document;
 
   /** Stores the subjects that emit the events that occur on the global document. */
-  private _subjects = new Map<K, Subject<Event>>();
+  private _observables = new Map<K, Observable<Event>>();
 
-  /** Stores the event handlers that emit the events that occur on the global document. */
-  private _handlers = new Map<K, ((event: Event) => void)>();
+  /** The notifier that triggers the global event observables to stop emitting and complete. */
+  private _destroyed = new Subject();
 
   constructor(@Inject(DOCUMENT) document: any, private _ngZone: NgZone) {
     this._document = document;
   }
 
-  /** Returns a function for handling the given type of event. */
-  private _createHandlerFn(type: K): ((event: Event) => void) {
-    return (event: Event) => {
-      this._subjects.get(type)!.next(event);
-    };
+  ngOnDestroy() {
+    this._destroyed.next();
+    this._destroyed.complete();
+    this._observables.clear();
   }
 
   /** Returns a subscription to global change or input events. */
   listen(type: K, callback: SpecificEventListener<K>): Subscription {
-    // This is the first subscription to these events.
-    if (!this._subjects.get(type)) {
-      const handlerFn = this._createHandlerFn(type).bind(this);
-      this._subjects.set(type, new Subject<Event>());
-      this._handlers.set(type, handlerFn);
-      this._ngZone.runOutsideAngular(() => {
-        this._document.addEventListener(type, handlerFn, true);
-      });
+    // If this is the first time we are listening to this event, create the observable for it.
+    if (!this._observables.has(type)) {
+      this._observables.set(type, this._createGlobalEventObservable(type));
     }
 
-    const subject = this._subjects.get(type)!;
-    const handler = this._handlers.get(type)!;
+    return this._ngZone.runOutsideAngular(() =>
+      this._observables.get(type)!.subscribe((event: Event) =>
+        this._ngZone.run(() => callback(event))
+      )
+    );
+  }
 
-    return subject.pipe(finalize(() => {
-        // This is the last event listener unsubscribing.
-        if (subject.observers.length === 1) {
-          this._document.removeEventListener(type, handler, true);
-          this._subjects.delete(type);
-          this._handlers.delete(type);
-        }
-    })).subscribe(callback);
+  /** Creates an observable that emits all events of the given type. */
+  private _createGlobalEventObservable(type: K) {
+    return fromEvent(this._document, type, {capture: true, passive: true}).pipe(
+      takeUntil(this._destroyed),
+      finalize(() => this._observables.delete(type)),
+      share(),
+    );
   }
 }

src/material-experimental/mdc-slider/slider.spec.ts

@@ -6,10 +6,8 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {LEFT_ARROW, RIGHT_ARROW} from '@angular/cdk/keycodes';
 import {Platform} from '@angular/cdk/platform';
 import {
-  dispatchFakeEvent,
   dispatchMouseEvent,
   dispatchPointerEvent,
   dispatchTouchEvent,
@@ -524,11 +522,12 @@ describe('MDC-based MatSlider' , () => {
   });
 
   describe('slider with set step', () => {
+    let fixture: ComponentFixture<SliderWithStep>;
     let sliderInstance: MatSlider;
     let inputInstance: MatSliderThumb;
 
     beforeEach(waitForAsync(() => {
-      const fixture = createComponent(SliderWithStep);
+      fixture = createComponent(SliderWithStep);
       fixture.detectChanges();
       const sliderDebugElement = fixture.debugElement.query(By.directive(MatSlider));
       sliderInstance = sliderDebugElement.componentInstance;
@@ -553,28 +552,20 @@ describe('MDC-based MatSlider' , () => {
     });
 
     it('should truncate long decimal values when using a decimal step', () => {
-      // TODO(wagnermaciel): Uncomment this test once b/182504575 is resolved.
-      // sliderInstance.step = 0.1;
-      // slideToValue(sliderInstance, 33.3333, Thumb.END, platform.IOS);
-      // expect(inputInstance.value).toBe(33);
-    });
-
-    it('should truncate long decimal values when using a decimal step and the arrow keys', () => {
       sliderInstance.step = 0.1;
-      changeValueUsingArrowKeys(sliderInstance, RIGHT_ARROW, Thumb.END);
-      changeValueUsingArrowKeys(sliderInstance, RIGHT_ARROW, Thumb.END);
-      changeValueUsingArrowKeys(sliderInstance, RIGHT_ARROW, Thumb.END);
-      expect(inputInstance.value).toBe(0.3);
+      slideToValue(sliderInstance, 66.3333, Thumb.END, platform.IOS);
+      expect(inputInstance.value).toBe(66.3);
     });
   });
 
   describe('range slider with set step', () => {
+    let fixture: ComponentFixture<RangeSliderWithStep>;
     let sliderInstance: MatSlider;
     let startInputInstance: MatSliderThumb;
     let endInputInstance: MatSliderThumb;
 
     beforeEach(waitForAsync(() => {
-      const fixture = createComponent(RangeSliderWithStep);
+      fixture = createComponent(RangeSliderWithStep);
       fixture.detectChanges();
       const sliderDebugElement = fixture.debugElement.query(By.directive(MatSlider));
       sliderInstance = sliderDebugElement.componentInstance;
@@ -617,33 +608,23 @@ describe('MDC-based MatSlider' , () => {
     });
 
     it('should truncate long decimal start values when using a decimal step', () => {
-      // TODO(wagnermaciel): Uncomment this test once b/182504575 is resolved.
-      // sliderInstance.step = 0.1;
-      // slideToValue(sliderInstance, 33.3333, Thumb.START, platform.IOS);
-      // expect(startInputInstance.value).toBe(33);
+      sliderInstance.step = 0.1;
+      slideToValue(sliderInstance, 66.3333, Thumb.START, platform.IOS);
+      expect(startInputInstance.value).toBe(66.3);
     });
 
     it('should truncate long decimal end values when using a decimal step', () => {
-      // TODO(wagnermaciel): Uncomment this test once b/182504575 is resolved.
-      // sliderInstance.step = 0.1;
-      // slideToValue(sliderInstance, 66.6666, Thumb.END, platform.IOS);
-      // expect(endInputInstance.value).toBe(66);
-    });
-
-    it('should truncate long decimal start values when using a decimal step arrow keys', () => {
       sliderInstance.step = 0.1;
-      changeValueUsingArrowKeys(sliderInstance, RIGHT_ARROW, Thumb.START);
-      changeValueUsingArrowKeys(sliderInstance, RIGHT_ARROW, Thumb.START);
-      changeValueUsingArrowKeys(sliderInstance, RIGHT_ARROW, Thumb.START);
-      expect(startInputInstance.value).toBe(0.3);
-    });
+      slideToValue(sliderInstance, 66.3333, Thumb.END, platform.IOS);
+      expect(endInputInstance.value).toBe(66.3);
 
-    it('should truncate long decimal end values when using a decimal step arrow keys', () => {
-      sliderInstance.step = 0.1;
-      changeValueUsingArrowKeys(sliderInstance, LEFT_ARROW, Thumb.END);
-      changeValueUsingArrowKeys(sliderInstance, LEFT_ARROW, Thumb.END);
-      changeValueUsingArrowKeys(sliderInstance, LEFT_ARROW, Thumb.END);
-      expect(endInputInstance.value).toBe(99.7);
+      // NOTE(wagnermaciel): Different browsers treat the clientX dispatched by us differently.
+      // Below is an example of a case that should work but because Firefox rounds the clientX
+      // down, the clientX that gets dispatched (1695.998...) is not the same clientX that the MDC
+      // Foundation receives (1695). This means the test will pass on chromium but fail on Firefox.
+      //
+      // slideToValue(sliderInstance, 66.66, Thumb.END, platform.IOS);
+      // expect(endInputInstance.value).toBe(66.7);
     });
   });
 
@@ -979,22 +960,6 @@ function slideToValue(slider: MatSlider, value: number, thumbPosition: Thumb, is
   dispatchPointerOrTouchEvent(sliderElement, PointerEventType.POINTER_UP, endX, endY, isIOS);
 }
 
-/**
- * Mimics changing the slider value using arrow keys.
- *
- * Dispatching keydown events on inputs do not trigger value changes. Thus, to mimic this behavior,
- * we manually change the slider inputs value and then dispatch a change event (which is what the
- * MDC Foundation is listening for & how it handles these updates).
- */
-function changeValueUsingArrowKeys(slider: MatSlider, arrow: number, thumbPosition: Thumb) {
-  const input = slider._getInput(thumbPosition);
-  const value = arrow === RIGHT_ARROW
-    ? input.value + slider.step
-    : input.value - slider.step;
-  input._hostElement.value = value.toString();
-  dispatchFakeEvent(input._hostElement, 'change');
-}
-
 /** Dispatch a pointerdown or pointerup event if supported, otherwise dispatch the touch event. */
 function dispatchPointerOrTouchEvent(
   node: Node, type: PointerEventType, x: number, y: number, isIOS: boolean) {

src/material-experimental/mdc-slider/slider.ts

@@ -383,9 +383,8 @@ export class MatSliderThumb implements AfterViewInit, ControlValueAccessor, OnIn
 
   _emitFakeEvent(type: 'change'|'input') {
     const event = new Event(type) as any;
-    event.isFake = true;
-    const emitter = type === 'change' ? this.change : this.input;
-    emitter.emit(event);
+    event._matIsHandled = true;
+    this._hostElement.dispatchEvent(event);
   }
 
   /**
@@ -772,10 +771,85 @@ export class MatSlider extends _MatSliderMixinBase
 /** The MDCSliderAdapter implementation. */
 class SliderAdapter implements MDCSliderAdapter {
 
-  /** The global change listener subscription used to handle change events on the slider inputs. */
-  changeSubscription: Subscription;
+  /** The global event listener subscription used to handle events on the slider inputs. */
+  private _globalEventSubscriptions = new Subscription();
 
-  constructor(private readonly _delegate: MatSlider) {}
+  /** The MDC Foundations handler function for start input change events. */
+  private _startInputChangeEventHandler: SpecificEventListener<EventType>;
+
+  /** The MDC Foundations handler function for end input change events. */
+  private _endInputChangeEventHandler: SpecificEventListener<EventType>;
+
+  constructor(private readonly _delegate: MatSlider) {
+    this._globalEventSubscriptions.add(this._subscribeToSliderInputEvents('change'));
+    this._globalEventSubscriptions.add(this._subscribeToSliderInputEvents('input'));
+  }
+
+  /**
+   * Handles "change" and "input" events on the slider inputs.
+   *
+   * Exposes a callback to allow the MDC Foundations "change" event handler to be called for "real"
+   * events.
+   *
+   * ** IMPORTANT NOTE **
+   *
+   * We block all "real" change and input events and emit fake events from #emitChangeEvent and
+   * #emitInputEvent, instead. We do this because interacting with the MDC slider won't trigger all
+   * of the correct change and input events, but it will call #emitChangeEvent and #emitInputEvent
+   * at the correct times. This allows users to listen for these events directly on the slider
+   * input as they would with a native range input.
+   */
+  private _subscribeToSliderInputEvents(type: 'change'|'input') {
+      return this._delegate._globalChangeAndInputListener.listen(type, (event: Event) => {
+        const thumbPosition = this._getInputThumbPosition(event.target);
+
+        // Do nothing if the event isn't from a thumb input.
+        if (thumbPosition === null) { return; }
+
+        // Do nothing if the event is "fake".
+        if ((event as any)._matIsHandled) { return ; }
+
+        // Prevent "real" events from reaching end users.
+        event.stopImmediatePropagation();
+
+        // Relay "real" change events to the MDC Foundation.
+        if (type === 'change') {
+          this._callChangeEventHandler(event, thumbPosition);
+        }
+      });
+  }
+
+  /** Calls the MDC Foundations change event handler for the specified thumb position. */
+  private _callChangeEventHandler(event: Event, thumbPosition: Thumb) {
+    if (thumbPosition === Thumb.START) {
+      this._startInputChangeEventHandler(event);
+    } else {
+      this._endInputChangeEventHandler(event);
+    }
+  }
+
+  /** Save the event handler so it can be used in our global change event listener subscription. */
+  private _saveChangeEventHandler(thumbPosition: Thumb, handler: SpecificEventListener<EventType>) {
+    if (thumbPosition === Thumb.START) {
+      this._startInputChangeEventHandler = handler;
+    } else {
+      this._endInputChangeEventHandler = handler;
+    }
+  }
+
+  /**
+   * Returns the thumb position of the given event target.
+   * Returns null if the given event target does not correspond to a slider thumb input.
+   */
+  private _getInputThumbPosition(target: EventTarget | null): Thumb | null {
+    if (target === this._delegate._getInputElement(Thumb.END)) {
+      return Thumb.END;
+    }
+    if (this._delegate._isRange() && target === this._delegate._getInputElement(Thumb.START)) {
+      return Thumb.START;
+    }
+    return null;
+  }
 
   // We manually assign functions instead of using prototype methods because
   // MDC clobbers the values otherwise.
@@ -901,29 +975,16 @@ class SliderAdapter implements MDCSliderAdapter {
   }
   registerInputEventHandler = <K extends EventType>
     (thumbPosition: Thumb, evtType: K, handler: SpecificEventListener<K>): void => {
-      if (evtType === 'change' || evtType === 'input') {
-        this.changeSubscription = this._delegate._globalChangeAndInputListener
-          .listen(evtType as 'change'|'input', (event: Event) => {
-            // We block all real change and input events and emit fake events from #emitChangeEvent
-            // and #emitInputEvent, instead. We do this because interacting with the MDC slider
-            // won't trigger all of the correct change and input events, but it will call
-            // #emitChangeEvent and #emitInputEvent at the correct times. This allows users to
-            // listen for these events directly on the slider input as they would with a native
-            // range input.
-            if (event.target === this._delegate._getInputElement(thumbPosition)) {
-              if ((event as any).isFake) { return; }
-              event.stopImmediatePropagation();
-              handler(event as GlobalEventHandlersEventMap[K]);
-            }
-        });
+      if (evtType === 'change') {
+        this._saveChangeEventHandler(thumbPosition, handler as SpecificEventListener<EventType>);
       } else {
         this._delegate._getInputElement(thumbPosition).addEventListener(evtType, handler);
       }
   }
   deregisterInputEventHandler = <K extends EventType>
     (thumbPosition: Thumb, evtType: K, handler: SpecificEventListener<K>): void => {
       if (evtType === 'change') {
-        this.changeSubscription.unsubscribe();
+        this._globalEventSubscriptions.unsubscribe();
       } else {
         this._delegate._getInputElement(thumbPosition).removeEventListener(evtType, handler);
       }