From ad3bdabae8482bd4559ae4ec38b2302b54a7f7c6 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 16 Feb 2018 13:53:06 +0100 Subject: [PATCH 1/3] fix(list-key-manager): infinite loop if all items are disabled * Currently if the `ListKeyManager` is being used in wrap mode, and all items are disabled, pressing the down arrow key will cause the key manager to go into an infinite loop that searches for the next item. Related #9963 --- .../a11y/key-manager/list-key-manager.spec.ts | 10 ++++++ src/cdk/a11y/key-manager/list-key-manager.ts | 36 ++++++++++--------- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/cdk/a11y/key-manager/list-key-manager.spec.ts b/src/cdk/a11y/key-manager/list-key-manager.spec.ts index ace4a24520d1..93e32ddd4d25 100644 --- a/src/cdk/a11y/key-manager/list-key-manager.spec.ts +++ b/src/cdk/a11y/key-manager/list-key-manager.spec.ts @@ -467,6 +467,16 @@ describe('Key managers', () => { expect(keyManager.setActiveItem).toHaveBeenCalledWith(0); }); + // This test should pass if all items are disabled and the down arrow key got pressed. + // If the test setup crashes or this test times out, this test can be considered as failed. + it('should not get into an infinite loop if all items are disabled', () => { + keyManager.withWrap(); + keyManager.setActiveItem(0); + + itemList.items.forEach(item => item.disabled = true); + + keyManager.onKeydown(fakeKeyEvents.downArrow); + }); }); describe('typeahead mode', () => { diff --git a/src/cdk/a11y/key-manager/list-key-manager.ts b/src/cdk/a11y/key-manager/list-key-manager.ts index d11782a0d8bf..51c964cb248c 100644 --- a/src/cdk/a11y/key-manager/list-key-manager.ts +++ b/src/cdk/a11y/key-manager/list-key-manager.ts @@ -259,7 +259,7 @@ export class ListKeyManager { * currently active item and the new active item. It will calculate differently * depending on whether wrap mode is turned on. */ - private _setActiveItemByDelta(delta: number, items = this._items.toArray()): void { + private _setActiveItemByDelta(delta: -1 | 1, items = this._items.toArray()): void { this._wrap ? this._setActiveInWrapMode(delta, items) : this._setActiveInDefaultMode(delta, items); } @@ -269,16 +269,15 @@ export class ListKeyManager { * down the list until it finds an item that is not disabled, and it will wrap if it * encounters either end of the list. */ - private _setActiveInWrapMode(delta: number, items: T[]): void { - // when active item would leave menu, wrap to beginning or end - this._activeItemIndex = - (this._activeItemIndex + delta + items.length) % items.length; - - // skip all disabled menu items recursively until an enabled one is reached - if (items[this._activeItemIndex].disabled) { - this._setActiveInWrapMode(delta, items); - } else { - this.setActiveItem(this._activeItemIndex); + private _setActiveInWrapMode(delta: -1 | 1, items: T[]): void { + for (let i = 1; i <= items.length; i++) { + const index = (this._activeItemIndex + delta * i + items.length) % items.length; + const item = items[index]; + + if (!item.disabled) { + this.setActiveItem(index); + break; + } } } @@ -287,7 +286,7 @@ export class ListKeyManager { * continue to move down the list until it finds an item that is not disabled. If * it encounters either end of the list, it will stop and not wrap. */ - private _setActiveInDefaultMode(delta: number, items: T[]): void { + private _setActiveInDefaultMode(delta: -1 | 1, items: T[]): void { this._setActiveItemByIndex(this._activeItemIndex + delta, delta, items); } @@ -296,13 +295,18 @@ export class ListKeyManager { * item is disabled, it will move in the fallbackDelta direction until it either * finds an enabled item or encounters the end of the list. */ - private _setActiveItemByIndex(index: number, fallbackDelta: number, - items = this._items.toArray()): void { - if (!items[index]) { return; } + private _setActiveItemByIndex(index: number, fallbackDelta: -1 | 1, + items = this._items.toArray()): void { + if (!items[index]) { + return; + } while (items[index].disabled) { index += fallbackDelta; - if (!items[index]) { return; } + + if (!items[index]) { + return; + } } this.setActiveItem(index); From 5a92ca2ab8b093d7af42c7dd1c7e2b5325f2704f Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 16 Feb 2018 14:04:27 +0100 Subject: [PATCH 2/3] Use `return` instead of `break` --- src/cdk/a11y/key-manager/list-key-manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cdk/a11y/key-manager/list-key-manager.ts b/src/cdk/a11y/key-manager/list-key-manager.ts index 51c964cb248c..a10b7330340a 100644 --- a/src/cdk/a11y/key-manager/list-key-manager.ts +++ b/src/cdk/a11y/key-manager/list-key-manager.ts @@ -276,7 +276,7 @@ export class ListKeyManager { if (!item.disabled) { this.setActiveItem(index); - break; + return; } } } From 8b0c6b3e50ec73bfe4b6b87b509927228a342197 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 17 Feb 2018 16:03:35 +0100 Subject: [PATCH 3/3] Add parentheses around multiplication --- src/cdk/a11y/key-manager/list-key-manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cdk/a11y/key-manager/list-key-manager.ts b/src/cdk/a11y/key-manager/list-key-manager.ts index a10b7330340a..c3178d4e0450 100644 --- a/src/cdk/a11y/key-manager/list-key-manager.ts +++ b/src/cdk/a11y/key-manager/list-key-manager.ts @@ -271,7 +271,7 @@ export class ListKeyManager { */ private _setActiveInWrapMode(delta: -1 | 1, items: T[]): void { for (let i = 1; i <= items.length; i++) { - const index = (this._activeItemIndex + delta * i + items.length) % items.length; + const index = (this._activeItemIndex + (delta * i) + items.length) % items.length; const item = items[index]; if (!item.disabled) {