From 0d45c3cd2ab3e541e2f4239c6c6b840cf99afdc2 Mon Sep 17 00:00:00 2001
From: crisbeto <crisbeto@abv.bg>
Date: Wed, 10 Oct 2018 13:14:24 +0300
Subject: [PATCH] fix(list): selection list moving focus when an option is
 destroyed

When an option from a selection list is destroyed, the list adjusts the active item index so the user's selection isn't lost, however because we use the `FocusKeyManager`, it means that updating the index will move focus to the new item. This can cause focus to be removed from another element on the page that the user might be interacting with. These changes continue to update the active index, but they only move focus if the destroyed option was focused at the time that it was destroyed.
---
 src/lib/list/selection-list.spec.ts | 21 +++++++++++++++++++++
 src/lib/list/selection-list.ts      | 24 +++++++++++++++++++-----
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/src/lib/list/selection-list.spec.ts b/src/lib/list/selection-list.spec.ts
index 8f3abdd5679a..ff75fb24088e 100644
--- a/src/lib/list/selection-list.spec.ts
+++ b/src/lib/list/selection-list.spec.ts
@@ -214,6 +214,7 @@ describe('MatSelectionList without forms', () => {
     it('should restore focus if active option is destroyed', () => {
       const manager = selectionList.componentInstance._keyManager;
 
+      spyOn(listOptions[2].componentInstance, 'focus').and.callThrough();
       listOptions[3].componentInstance._handleFocus();
 
       expect(manager.activeItemIndex).toBe(3);
@@ -222,8 +223,28 @@ describe('MatSelectionList without forms', () => {
       fixture.detectChanges();
 
       expect(manager.activeItemIndex).toBe(2);
+      expect(listOptions[2].componentInstance.focus).toHaveBeenCalled();
     });
 
+    it('should not attempt to focus the next option when the destroyed option was not focused',
+      () => {
+        const manager = selectionList.componentInstance._keyManager;
+
+        // Focus and blur the option to move the active item index.
+        listOptions[3].componentInstance._handleFocus();
+        listOptions[3].componentInstance._handleBlur();
+
+        spyOn(listOptions[2].componentInstance, 'focus').and.callThrough();
+
+        expect(manager.activeItemIndex).toBe(3);
+
+        fixture.componentInstance.showLastOption = false;
+        fixture.detectChanges();
+
+        expect(manager.activeItemIndex).toBe(2);
+        expect(listOptions[2].componentInstance.focus).not.toHaveBeenCalled();
+      });
+
     it('should focus previous item when press UP ARROW', () => {
       let testListItem = listOptions[2].nativeElement as HTMLElement;
       let UP_EVENT: KeyboardEvent =
diff --git a/src/lib/list/selection-list.ts b/src/lib/list/selection-list.ts
index e62c2c41e13a..dccd28866b40 100644
--- a/src/lib/list/selection-list.ts
+++ b/src/lib/list/selection-list.ts
@@ -98,6 +98,7 @@ export class MatListOption extends _MatListOptionMixinBase
 
   private _selected = false;
   private _disabled = false;
+  private _hasFocus = false;
 
   @ContentChild(MatListAvatarCssMatStyler) _avatar: MatListAvatarCssMatStyler;
   @ContentChild(MatListIconCssMatStyler) _icon: MatListIconCssMatStyler;
@@ -172,7 +173,13 @@ export class MatListOption extends _MatListOptionMixinBase
       Promise.resolve().then(() => this.selected = false);
     }
 
-    this.selectionList._removeOptionFromList(this);
+    const hadFocus = this._hasFocus;
+    const newActiveItem = this.selectionList._removeOptionFromList(this);
+
+    // Only move focus if this option was focused at the time it was destroyed.
+    if (hadFocus && newActiveItem) {
+      newActiveItem.focus();
+    }
   }
 
   /** Toggles the selection state of the option. */
@@ -209,10 +216,12 @@ export class MatListOption extends _MatListOptionMixinBase
 
   _handleFocus() {
     this.selectionList._setFocusedOption(this);
+    this._hasFocus = true;
   }
 
   _handleBlur() {
     this.selectionList._onTouched();
+    this._hasFocus = false;
   }
 
   /** Retrieves the DOM element of the component host. */
@@ -385,18 +394,23 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu
     this._keyManager.updateActiveItemIndex(this._getOptionIndex(option));
   }
 
-  /** Removes an option from the selection list and updates the active item. */
-  _removeOptionFromList(option: MatListOption) {
+  /**
+   * Removes an option from the selection list and updates the active item.
+   * @returns Currently-active item.
+   */
+  _removeOptionFromList(option: MatListOption): MatListOption | null {
     const optionIndex = this._getOptionIndex(option);
 
     if (optionIndex > -1 && this._keyManager.activeItemIndex === optionIndex) {
       // Check whether the option is the last item
       if (optionIndex > 0) {
-        this._keyManager.setPreviousItemActive();
+        this._keyManager.updateActiveItemIndex(optionIndex - 1);
       } else if (optionIndex === 0 && this.options.length > 1) {
-        this._keyManager.setNextItemActive();
+        this._keyManager.updateActiveItemIndex(Math.min(optionIndex + 1, this.options.length - 1));
       }
     }
+
+    return this._keyManager.activeItem;
   }
 
   /** Passes relevant key presses to our key manager. */