From 24a762f4f736ce4a33ab5cc3c6127c8960434efc Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 28 Apr 2017 22:52:17 +0200 Subject: [PATCH] fix(select): tab opening multiple select and space scrolling page (#4210) * fix(select): tab opening multiple select and space scrolling page * Fixes pressing tab (or any other key, except arrows) opening `md-select` in `multiple` mode. * Fixes the page getting scrolled down when opening a select by pressing space. * Adds missing test coverage for opening a multiple select with the arrow keys. * fix: defaultPrevented not working on IE * chore: cleaner dispatching of fake events --- src/lib/core/testing/dispatch-events.ts | 24 ++++++------ src/lib/core/testing/event-objects.ts | 11 ++++-- src/lib/select/select.spec.ts | 45 ++++++++++++++++++++- src/lib/select/select.ts | 52 +++++++++++++++---------- 4 files changed, 97 insertions(+), 35 deletions(-) diff --git a/src/lib/core/testing/dispatch-events.ts b/src/lib/core/testing/dispatch-events.ts index 7e137b96704..523989b87b3 100644 --- a/src/lib/core/testing/dispatch-events.ts +++ b/src/lib/core/testing/dispatch-events.ts @@ -1,20 +1,22 @@ -import { - createFakeEvent, - createKeyboardEvent, - createMouseEvent -} from './event-objects'; +import {createFakeEvent, createKeyboardEvent, createMouseEvent} from './event-objects'; + +/** Utility to dispatch any event on a Node. */ +export function dispatchEvent(node: Node | Window, event: Event): Event { + node.dispatchEvent(event); + return event; +} /** Shorthand to dispatch a fake event on a specified node. */ -export function dispatchFakeEvent(node: Node | Window, type: string) { - node.dispatchEvent(createFakeEvent(type)); +export function dispatchFakeEvent(node: Node | Window, type: string): Event { + return dispatchEvent(node, createFakeEvent(type)); } /** Shorthand to dispatch a keyboard event with a specified key code. */ -export function dispatchKeyboardEvent(node: Node, type: string, keyCode: number) { - node.dispatchEvent(createKeyboardEvent(type, keyCode)); +export function dispatchKeyboardEvent(node: Node, type: string, keyCode: number): KeyboardEvent { + return dispatchEvent(node, createKeyboardEvent(type, keyCode)) as KeyboardEvent; } /** Shorthand to dispatch a mouse event on the specified coordinates. */ -export function dispatchMouseEvent(node: Node, type: string, x = 0, y = 0) { - node.dispatchEvent(createMouseEvent(type, x, y)); +export function dispatchMouseEvent(node: Node, type: string, x = 0, y = 0): MouseEvent { + return dispatchEvent(node, createMouseEvent(type, x, y)) as MouseEvent; } diff --git a/src/lib/core/testing/event-objects.ts b/src/lib/core/testing/event-objects.ts index 909ac27c773..819d943e794 100644 --- a/src/lib/core/testing/event-objects.ts +++ b/src/lib/core/testing/event-objects.ts @@ -26,14 +26,19 @@ export function createKeyboardEvent(type: string, keyCode: number) { let event = document.createEvent('KeyboardEvent') as any; // Firefox does not support `initKeyboardEvent`, but supports `initKeyEvent`. let initEventFn = (event.initKeyEvent || event.initKeyboardEvent).bind(event); + let originalPreventDefault = event.preventDefault; initEventFn(type, true, true, window, 0, 0, 0, 0, 0, keyCode); // Webkit Browsers don't set the keyCode when calling the init function. // See related bug https://bugs.webkit.org/show_bug.cgi?id=16735 - Object.defineProperty(event, 'keyCode', { - get: function() { return keyCode; } - }); + Object.defineProperty(event, 'keyCode', { get: () => keyCode }); + + // IE won't set `defaultPrevented` on synthetic events so we need to do it manually. + event.preventDefault = function() { + Object.defineProperty(event, 'defaultPrevented', { get: () => true }); + return originalPreventDefault.apply(this, arguments); + }; return event; } diff --git a/src/lib/select/select.spec.ts b/src/lib/select/select.spec.ts index 8ae27762aac..5d5770557f7 100644 --- a/src/lib/select/select.spec.ts +++ b/src/lib/select/select.spec.ts @@ -16,7 +16,7 @@ import {MdSelect, MdSelectFloatPlaceholderType} from './select'; import {MdSelectDynamicMultipleError, MdSelectNonArrayValueError} from './select-errors'; import {MdOption} from '../core/option/option'; import {Dir} from '../core/rtl/dir'; -import {DOWN_ARROW, UP_ARROW} from '../core/keyboard/keycodes'; +import {DOWN_ARROW, UP_ARROW, ENTER, SPACE} from '../core/keyboard/keycodes'; import { ControlValueAccessor, FormControl, FormsModule, NG_VALUE_ACCESSOR, ReactiveFormsModule } from '@angular/forms'; @@ -1370,6 +1370,26 @@ describe('MdSelect', () => { 'Expected value from second option to have been set on the model.'); }); + it('should open the panel when pressing the arrow keys on a closed multiple select', () => { + fixture.destroy(); + + const multiFixture = TestBed.createComponent(MultiSelect); + const instance = multiFixture.componentInstance; + + multiFixture.detectChanges(); + select = multiFixture.debugElement.query(By.css('md-select')).nativeElement; + + const initialValue = instance.control.value; + + expect(instance.select.panelOpen).toBe(false, 'Expected panel to be closed.'); + + const event = dispatchKeyboardEvent(select, 'keydown', DOWN_ARROW); + + expect(instance.select.panelOpen).toBe(true, 'Expected panel to be open.'); + expect(instance.control.value).toBe(initialValue, 'Expected value to stay the same.'); + expect(event.defaultPrevented).toBe(true, 'Expected default to be prevented.'); + }); + it('should do nothing if the key manager did not change the active item', () => { const formControl = fixture.componentInstance.control; @@ -1418,6 +1438,29 @@ describe('MdSelect', () => { expect(lastOption.selected).toBe(true, 'Expected last option to stay selected.'); }); + it('should not open a multiple select when tabbing through', () => { + fixture.destroy(); + + const multiFixture = TestBed.createComponent(MultiSelect); + + multiFixture.detectChanges(); + select = multiFixture.debugElement.query(By.css('md-select')).nativeElement; + + expect(multiFixture.componentInstance.select.panelOpen) + .toBe(false, 'Expected panel to be closed initially.'); + + dispatchKeyboardEvent(select, 'keydown', TAB); + + expect(multiFixture.componentInstance.select.panelOpen) + .toBe(false, 'Expected panel to stay closed.'); + }); + + it('should prevent the default action when pressing space', () => { + let event = dispatchKeyboardEvent(select, 'keydown', SPACE); + + expect(event.defaultPrevented).toBe(true); + }); + }); describe('for options', () => { diff --git a/src/lib/select/select.ts b/src/lib/select/select.ts index 54bd2692e89..8738c844da2 100644 --- a/src/lib/select/select.ts +++ b/src/lib/select/select.ts @@ -18,7 +18,7 @@ import { OnInit, } from '@angular/core'; import {MdOption, MdOptionSelectionChange} from '../core/option/option'; -import {ENTER, SPACE} from '../core/keyboard/keycodes'; +import {ENTER, SPACE, UP_ARROW, DOWN_ARROW} from '../core/keyboard/keycodes'; import {FocusKeyManager} from '../core/a11y/focus-key-manager'; import {Dir} from '../core/rtl/dir'; import {Observable} from 'rxjs/Observable'; @@ -480,27 +480,14 @@ export class MdSelect implements AfterContentInit, OnDestroy, OnInit, ControlVal this._triggerWidth = this._getTriggerRect().width; } - /** Ensures the panel opens if activated by the keyboard. */ + /** Handles the keyboard interactions of a closed select. */ _handleKeydown(event: KeyboardEvent): void { - if (event.keyCode === ENTER || event.keyCode === SPACE) { - this.open(); - } else if (!this.disabled) { - let prevActiveItem = this._keyManager.activeItem; - - // Cycle though the select options even when the select is closed, - // matching the behavior of the native select element. - // TODO(crisbeto): native selects also cycle through the options with left/right arrows, - // however the key manager only supports up/down at the moment. - this._keyManager.onKeydown(event); - - let currentActiveItem = this._keyManager.activeItem as MdOption; - - if (this._multiple) { + if (!this.disabled) { + if (event.keyCode === ENTER || event.keyCode === SPACE) { + event.preventDefault(); // prevents the page from scrolling down when pressing space this.open(); - } else if (currentActiveItem !== prevActiveItem) { - this._clearSelection(); - this._setSelectionByValue(currentActiveItem.value); - this._propagateChanges(); + } else if (event.keyCode === UP_ARROW || event.keyCode === DOWN_ARROW) { + this._handleArrowKey(event); } } } @@ -975,6 +962,31 @@ export class MdSelect implements AfterContentInit, OnDestroy, OnInit, ControlVal private _floatPlaceholderState(): string { return this._isRtl() ? 'floating-rtl' : 'floating-ltr'; } + + /** Handles the user pressing the arrow keys on a closed select. */ + private _handleArrowKey(event: KeyboardEvent): void { + if (this._multiple) { + event.preventDefault(); + this.open(); + } else { + const prevActiveItem = this._keyManager.activeItem; + + // Cycle though the select options even when the select is closed, + // matching the behavior of the native select element. + // TODO(crisbeto): native selects also cycle through the options with left/right arrows, + // however the key manager only supports up/down at the moment. + this._keyManager.onKeydown(event); + + const currentActiveItem = this._keyManager.activeItem as MdOption; + + if (currentActiveItem !== prevActiveItem) { + this._clearSelection(); + this._setSelectionByValue(currentActiveItem.value); + this._propagateChanges(); + } + } + } + } /** Clamps a value n between min and max values. */