fix(select): don't prevent enter and space keys if a modifier is pressed (#14090)

* Only handles the space and enter keys, and prevents their default actions, if none of the modifier keys are pressed.
* Moves some tests that were testing `mat-option` logic into the `mat-option` specs.

Author: crisbeto

src/lib/core/BUILD.bazel

@@ -118,6 +118,7 @@ ng_test_library(
     "@angular//packages/platform-browser/animations",
     "//src/cdk/platform",
     "//src/cdk/testing",
+    "//src/cdk/keycodes",
     "//src/lib/core",
   ]
 )

src/lib/core/option/option.spec.ts

@@ -1,7 +1,13 @@
 import {async, ComponentFixture, TestBed} from '@angular/core/testing';
 import {Component, DebugElement} from '@angular/core';
 import {By} from '@angular/platform-browser';
-import {dispatchFakeEvent} from '@angular/cdk/testing';
+import {
+  dispatchFakeEvent,
+  dispatchKeyboardEvent,
+  createKeyboardEvent,
+  dispatchEvent,
+} from '@angular/cdk/testing';
+import {SPACE, ENTER} from '@angular/cdk/keycodes';
 import {MatOption, MatOptionModule} from './index';
 
 describe('MatOption component', () => {
@@ -82,6 +88,65 @@ describe('MatOption component', () => {
     expect(optionInstance.id).toBe('custom-option');
   });
 
+  it('should select the option when pressing space', () => {
+    const fixture = TestBed.createComponent(BasicOption);
+    fixture.detectChanges();
+
+    const optionDebugElement = fixture.debugElement.query(By.directive(MatOption));
+    const optionNativeElement: HTMLElement = optionDebugElement.nativeElement;
+    const optionInstance: MatOption = optionDebugElement.componentInstance;
+    const spy = jasmine.createSpy('selection change spy');
+    const subscription = optionInstance.onSelectionChange.subscribe(spy);
+
+    const event = dispatchKeyboardEvent(optionNativeElement, 'keydown', SPACE);
+    fixture.detectChanges();
+
+    expect(spy).toHaveBeenCalled();
+    expect(event.defaultPrevented).toBe(true);
+    subscription.unsubscribe();
+  });
+
+  it('should select the option when pressing enter', () => {
+    const fixture = TestBed.createComponent(BasicOption);
+    fixture.detectChanges();
+
+    const optionDebugElement = fixture.debugElement.query(By.directive(MatOption));
+    const optionNativeElement: HTMLElement = optionDebugElement.nativeElement;
+    const optionInstance: MatOption = optionDebugElement.componentInstance;
+    const spy = jasmine.createSpy('selection change spy');
+    const subscription = optionInstance.onSelectionChange.subscribe(spy);
+
+    const event = dispatchKeyboardEvent(optionNativeElement, 'keydown', ENTER);
+    fixture.detectChanges();
+
+    expect(spy).toHaveBeenCalled();
+    expect(event.defaultPrevented).toBe(true);
+    subscription.unsubscribe();
+  });
+
+  it('should not do anything when pressing the selection keys with a modifier', () => {
+    const fixture = TestBed.createComponent(BasicOption);
+    fixture.detectChanges();
+
+    const optionDebugElement = fixture.debugElement.query(By.directive(MatOption));
+    const optionNativeElement: HTMLElement = optionDebugElement.nativeElement;
+    const optionInstance: MatOption = optionDebugElement.componentInstance;
+    const spy = jasmine.createSpy('selection change spy');
+    const subscription = optionInstance.onSelectionChange.subscribe(spy);
+
+    [ENTER, SPACE].forEach(key => {
+      const event = createKeyboardEvent('keydown', key);
+      Object.defineProperty(event, 'shiftKey', {get: () => true});
+      dispatchEvent(optionNativeElement, event);
+      fixture.detectChanges();
+
+      expect(event.defaultPrevented).toBe(false);
+    });
+
+    expect(spy).not.toHaveBeenCalled();
+    subscription.unsubscribe();
+  });
+
   describe('ripples', () => {
     let fixture: ComponentFixture<BasicOption>;
     let optionDebugElement: DebugElement;

src/lib/core/option/option.ts

@@ -7,7 +7,7 @@
  */
 
 import {coerceBooleanProperty} from '@angular/cdk/coercion';
-import {ENTER, SPACE} from '@angular/cdk/keycodes';
+import {ENTER, SPACE, hasModifierKey} from '@angular/cdk/keycodes';
 import {
   AfterViewChecked,
   ChangeDetectionStrategy,
@@ -200,7 +200,7 @@ export class MatOption implements AfterViewChecked, OnDestroy {
 
   /** Ensures the option is selected when activated from the keyboard. */
   _handleKeydown(event: KeyboardEvent): void {
-    if (event.keyCode === ENTER || event.keyCode === SPACE) {
+    if ((event.keyCode === ENTER || event.keyCode === SPACE) && !hasModifierKey(event)) {
       this._selectViaInteraction();
 
       // Prevent the page from scrolling down and form submits.

src/lib/select/select.spec.ts

@@ -669,6 +669,21 @@ describe('MatSelect', () => {
           expect(event.defaultPrevented).toBe(true);
         }));
 
+        it('should prevent the default action when pressing enter', fakeAsync(() => {
+          const event = dispatchKeyboardEvent(select, 'keydown', ENTER);
+          expect(event.defaultPrevented).toBe(true);
+        }));
+
+        it('should not prevent the default actions on selection keys when pressing a modifier',
+          fakeAsync(() => {
+            [ENTER, SPACE].forEach(key => {
+              const event = createKeyboardEvent('keydown', key);
+              Object.defineProperty(event, 'shiftKey', {get: () => true});
+              expect(event.defaultPrevented).toBe(false);
+            });
+
+          }));
+
         it('should consider the selection a result of a user action when closed', fakeAsync(() => {
           const option = fixture.componentInstance.options.first;
           const spy = jasmine.createSpy('option selection spy');
@@ -1067,26 +1082,6 @@ describe('MatSelect', () => {
         expect(panel.classList).toContain('custom-two');
       }));
 
-      it('should prevent the default action when pressing SPACE on an option', fakeAsync(() => {
-        trigger.click();
-        fixture.detectChanges();
-
-        const option = overlayContainerElement.querySelector('mat-option')!;
-        const event = dispatchKeyboardEvent(option, 'keydown', SPACE);
-
-        expect(event.defaultPrevented).toBe(true);
-      }));
-
-      it('should prevent the default action when pressing ENTER on an option', fakeAsync(() => {
-        trigger.click();
-        fixture.detectChanges();
-
-        const option = overlayContainerElement.querySelector('mat-option')!;
-        const event = dispatchKeyboardEvent(option, 'keydown', ENTER);
-
-        expect(event.defaultPrevented).toBe(true);
-      }));
-
       it('should update disableRipple properly on each option', fakeAsync(() => {
         const options = fixture.componentInstance.options.toArray();
 

src/lib/select/select.ts

@@ -20,6 +20,7 @@ import {
   RIGHT_ARROW,
   SPACE,
   UP_ARROW,
+  hasModifierKey,
 } from '@angular/cdk/keycodes';
 import {CdkConnectedOverlay, Overlay, ScrollStrategy} from '@angular/cdk/overlay';
 import {ViewportRuler} from '@angular/cdk/scrolling';
@@ -695,7 +696,7 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
     const manager = this._keyManager;
 
     // Open the select on ALT + arrow key to match the native <select>
-    if (isOpenKey || ((this.multiple || event.altKey) && isArrowKey)) {
+    if ((isOpenKey && !hasModifierKey(event)) || ((this.multiple || event.altKey) && isArrowKey)) {
       event.preventDefault(); // prevents the page from scrolling down when pressing space
       this.open();
     } else if (!this.multiple) {
@@ -721,7 +722,8 @@ export class MatSelect extends _MatSelectMixinBase implements AfterContentInit,
       // Close the select on ALT + arrow key to match the native <select>
       event.preventDefault();
       this.close();
-    } else if ((keyCode === ENTER || keyCode === SPACE) && manager.activeItem) {
+    } else if ((keyCode === ENTER || keyCode === SPACE) && manager.activeItem &&
+      !hasModifierKey(event)) {
       event.preventDefault();
       manager.activeItem._selectViaInteraction();
     } else if (this._multiple && keyCode === A && event.ctrlKey) {