From 29ec3cd22256c5f13045a80247d55215df6594e8 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 22 Apr 2024 13:28:59 -0500 Subject: [PATCH 01/34] Change Select to not update value as you navigate options in dropdown. --- .../nimble-components/src/select/index.ts | 144 +++++++++++++----- .../src/select/testing/select.pageobject.ts | 27 +++- .../select/tests/select.foundation.spec.ts | 16 +- .../src/select/tests/select.spec.ts | 46 +++++- 4 files changed, 184 insertions(+), 49 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index dce1e7557b..58ed4d0404 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -18,6 +18,7 @@ import { DelegatesARIASelect } from '@microsoft/fast-foundation'; import { + findLastIndex, keyArrowDown, keyArrowUp, keyEnd, @@ -197,6 +198,7 @@ export class Select private _value = ''; private forcedPosition = false; private indexWhenOpened?: number; + private openActiveIndex?: number; /** * @internal @@ -338,8 +340,11 @@ export class Select super.clickHandler(e); this.open = this.collapsible && !this.open; - - if (!this.open && this.indexWhenOpened !== this.selectedIndex) { + if ( + !this.open + && this.selectedIndex !== -1 + && this.indexWhenOpened !== this.selectedIndex + ) { this.updateValue(true); } } @@ -476,11 +481,11 @@ export class Select o => !o.disabled ); if (enabledOptions.length > 0) { - enabledOptions[0]!.selected = true; + enabledOptions[0]!.ariaSelected = 'true'; + this.openActiveIndex = this.options.indexOf(enabledOptions[0]!); } else { // only filtered option is disabled - this.selectedOptions = []; - this.selectedIndex = -1; + this.openActiveIndex = -1; } } else if (this.committedSelectedOption) { this.committedSelectedOption.selected = true; @@ -510,13 +515,19 @@ export class Select } if (!this.options?.includes(focusTarget as ListboxOption)) { + let currentActiveIndex = this.openActiveIndex ?? this.selectedIndex; this.open = false; - if (this.selectedIndex === -1) { + if (currentActiveIndex === -1) { this.selectedIndex = this.indexWhenOpened!; + currentActiveIndex = this.selectedIndex; + if (this.selectedIndex >= 0) { + this.options[this.selectedIndex]!.ariaSelected = 'true'; + } } - if (this.indexWhenOpened !== this.selectedIndex) { + if (this.indexWhenOpened !== currentActiveIndex) { this.updateValue(true); + this.selectedIndex = currentActiveIndex; } } return true; @@ -532,6 +543,7 @@ export class Select return true; } + let currentActiveIndex = this.openActiveIndex ?? this.selectedIndex; switch (key) { case keySpace: { // when dropdown is open allow user to enter a space for filter text @@ -576,20 +588,34 @@ export class Select this.open = false; } - if (this.selectedIndex !== this.indexWhenOpened!) { - this.options[this.selectedIndex]!.selected = false; - this.selectedIndex = this.indexWhenOpened!; + if (currentActiveIndex !== this.indexWhenOpened!) { + if (currentActiveIndex >= 0) { + this.options[currentActiveIndex]!.ariaSelected = 'false'; + } + this.options[this.indexWhenOpened!]!.ariaSelected = 'true'; + currentActiveIndex = this.indexWhenOpened!; } this.focus(); break; } case keyTab: { if (this.collapsible && this.open) { - e.preventDefault(); - this.open = false; + if ( + this.filteredOptions.length === 0 + || this.filteredOptions.every(o => o.disabled) + ) { + if (currentActiveIndex >= 0) { + this.options[currentActiveIndex]!.ariaSelected = 'false'; + } + this.options[this.indexWhenOpened!]!.ariaSelected = 'true'; + currentActiveIndex = this.indexWhenOpened!; + this.open = false; + return true; + } } - return true; + this.open = false; + break; } default: { @@ -597,9 +623,11 @@ export class Select } } - if (!this.open && this.indexWhenOpened !== this.selectedIndex) { + if (!this.open && this.indexWhenOpened !== currentActiveIndex) { + this.selectedIndex = currentActiveIndex; + this.options[this.selectedIndex]!.ariaSelected = 'true'; this.updateValue(true); - this.indexWhenOpened = this.selectedIndex; + this.indexWhenOpened = undefined; } return !(key === keyArrowDown || key === keyArrowUp); @@ -658,31 +686,58 @@ export class Select public override selectNextOption(): void { // don't call super.selectNextOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) - for (let i = this.selectedIndex + 1; i < this.options.length; i++) { - const listOption = this.options[i]!; - if ( - isNimbleListOption(listOption) - && isOptionSelectable(listOption) - ) { - this.selectedIndex = i; - break; + const startIndex = this.openActiveIndex ?? this.selectedIndex; + const newActiveOptionFunc = (): number => { + for (let i = startIndex + 1; i < this.options.length; i++) { + const listOption = this.options[i]!; + if ( + isNimbleListOption(listOption) + && isOptionSelectable(listOption) + ) { + return i; + } } - } + return -1; + }; + this.toggleNewActiveOption(newActiveOptionFunc); } public override selectPreviousOption(): void { // don't call super.selectPreviousOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) - for (let i = this.selectedIndex - 1; i >= 0; i--) { - const listOption = this.options[i]!; - if ( - isNimbleListOption(listOption) - && isOptionSelectable(listOption) - ) { - this.selectedIndex = i; - break; + const startIndex = this.openActiveIndex ?? this.selectedIndex; + const newActiveOptionFunc = (): number => { + for (let i = startIndex - 1; i >= 0; i--) { + const listOption = this.options[i]!; + if ( + isNimbleListOption(listOption) + && isOptionSelectable(listOption) + ) { + return i; + } } - } + return -1; + }; + this.toggleNewActiveOption(newActiveOptionFunc); + } + + public override selectFirstOption(): void { + const newActiveOptionFunc = (): number => { + return this.options.findIndex( + o => isNimbleListOption(o) && isOptionSelectable(o) + ); + }; + this.toggleNewActiveOption(newActiveOptionFunc); + } + + public override selectLastOption(): void { + const newActiveOptionFunc = (): number => { + return findLastIndex( + this.options, + o => isNimbleListOption(o) && isOptionSelectable(o) + ); + }; + this.toggleNewActiveOption(newActiveOptionFunc); } /** @@ -757,11 +812,10 @@ export class Select if (this.open) { this.initializeOpenState(); - this.indexWhenOpened = this.selectedIndex; - return; } + this.openActiveIndex = undefined; this.filter = ''; if (this.filterInput) { this.filterInput.value = ''; @@ -837,6 +891,22 @@ export class Select this.committedSelectedOption = options[this.selectedIndex]; } + private toggleNewActiveOption(newActiveIndexFunc: () => number): void { + const selectedOption = this.options[ + this.openActiveIndex ?? this.selectedIndex + ] as ListOption; + const activeIndex = newActiveIndexFunc(); + if (activeIndex >= 0) { + this.options[activeIndex]!.ariaSelected = 'true'; + selectedOption.ariaSelected = 'false'; + if (this.open) { + this.openActiveIndex = activeIndex; + } else { + this.selectedIndex = activeIndex; + } + } + } + private committedSelectedOptionChanged(): void { this.updateDisplayValue(); } @@ -948,7 +1018,7 @@ export class Select private clearSelection(): void { this.options.forEach(option => { - option.selected = false; + option.ariaSelected = 'false'; }); } @@ -967,6 +1037,8 @@ export class Select return; } + this.indexWhenOpened = this.selectedIndex; + this.openActiveIndex = this.selectedIndex; this.committedSelectedOption = this.options[this.selectedIndex]; this.ariaControls = this.listboxId; this.ariaExpanded = 'true'; diff --git a/packages/nimble-components/src/select/testing/select.pageobject.ts b/packages/nimble-components/src/select/testing/select.pageobject.ts index 4ea4cda6db..74478517c9 100644 --- a/packages/nimble-components/src/select/testing/select.pageobject.ts +++ b/packages/nimble-components/src/select/testing/select.pageobject.ts @@ -3,7 +3,8 @@ import { keyEscape, keyArrowDown, keyArrowUp, - keySpace + keySpace, + keyTab } from '@microsoft/fast-web-utilities'; import type { Select } from '..'; import type { ListOption } from '../../list-option'; @@ -53,6 +54,10 @@ export class SelectPageObject { } public getSelectedOption(): ListOption | null { + if (this.selectElement.open) { + return this.getSelectedDropdownOption(); + } + return (this.selectElement.selectedOptions[0] as ListOption) ?? null; } @@ -68,7 +73,11 @@ export class SelectPageObject { throw new Error('Select must be open to click selectedItem'); } - this.clickOption(this.selectElement.selectedIndex); + const selectedOption = this.getSelectedDropdownOption(); + if (!selectedOption) { + throw new Error('No option is selected to click'); + } + this.clickOption(this.selectElement.options.indexOf(selectedOption)); } public async clickFilterInput(): Promise { @@ -126,6 +135,12 @@ export class SelectPageObject { ); } + public pressTabKey(): void { + this.selectElement.dispatchEvent( + new KeyboardEvent('keydown', { key: keyTab }) + ); + } + public pressArrowDownKey(): void { this.selectElement.dispatchEvent( new KeyboardEvent('keydown', { key: keyArrowDown }) @@ -206,4 +221,12 @@ export class SelectPageObject { '.filter-input' ); } + + private getSelectedDropdownOption(): ListOption | null { + return ( + (this.selectElement.options.find( + o => o.ariaSelected === 'true' + ) as ListOption) ?? null + ); + } } diff --git a/packages/nimble-components/src/select/tests/select.foundation.spec.ts b/packages/nimble-components/src/select/tests/select.foundation.spec.ts index df84a45042..8c606fa850 100644 --- a/packages/nimble-components/src/select/tests/select.foundation.spec.ts +++ b/packages/nimble-components/src/select/tests/select.foundation.spec.ts @@ -298,7 +298,7 @@ describe('Select', () => { await disconnect(); }); - describe("should NOT emit a 'change' event when the value changes by user input while open", () => { + describe("should NOT emit a 'change' event while open", () => { it('via arrow down key', async () => { const { element, connect, disconnect } = await setup(); @@ -322,7 +322,7 @@ describe('Select', () => { expect(wasChanged).toBeFalse(); - expect(element.value).toEqual('two'); + expect(element.value).toEqual('one'); await disconnect(); }); @@ -354,7 +354,7 @@ describe('Select', () => { expect(wasChanged).toBeFalse(); - expect(element.value).toEqual('one'); + expect(element.value).toEqual('two'); await disconnect(); }); @@ -412,13 +412,13 @@ describe('Select', () => { expect(wasChanged).toBeFalse(); - expect(element.value).toEqual('three'); + expect(element.value).toEqual('one'); await disconnect(); }); }); - describe("should NOT emit an 'input' event when the value changes by user input while open", () => { + describe("should NOT emit an 'input' event while open", () => { it('via arrow down key', async () => { const { element, connect, disconnect } = await setup(); @@ -442,7 +442,7 @@ describe('Select', () => { expect(wasInput).toBeFalse(); - expect(element.value).toEqual('two'); + expect(element.value).toEqual('one'); await disconnect(); }); @@ -474,7 +474,7 @@ describe('Select', () => { expect(wasInput).toBeFalse(); - expect(element.value).toEqual('one'); + expect(element.value).toEqual('two'); await disconnect(); }); @@ -532,7 +532,7 @@ describe('Select', () => { expect(wasInput).toBeFalse(); - expect(element.value).toEqual('three'); + expect(element.value).toEqual('one'); await disconnect(); }); diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index d4cd1fb97c..2752f9eb3d 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -184,8 +184,6 @@ describe('Select', () => { pageObject.clickSelect(); pageObject.pressArrowDownKey(); await waitForUpdatesAsync(); - expect(element.selectedIndex).toBe(1); - pageObject.pressEscapeKey(); await waitForUpdatesAsync(); @@ -569,6 +567,14 @@ describe('Select', () => { pageObject.pressEnterKey(); expect(document.activeElement).toBe(element); }); + + it('after closing dropdown by committing a value with , activeElement is not Select element', () => { + element.filterMode = testData.filter; + pageObject.clickSelect(); + pageObject.pressArrowDownKey(); + pageObject.pressTabKey(); + expect(document.activeElement).not.toBe(element); + }); }); }); }); @@ -664,6 +670,17 @@ describe('Select', () => { expect(element.open).toBeFalse(); }); + it('filtering out current selected item and then pressing changes value and closes popup', async () => { + const currentSelection = pageObject.getSelectedOption(); + expect(currentSelection?.text).toBe('One'); + expect(element.value).toBe('one'); + + await pageObject.openAndSetFilterText('T'); // Matches 'Two' and 'Three' + pageObject.pressTabKey(); + expect(element.value).toBe('two'); // 'Two' is first option in list so it should be selected now + expect(element.open).toBeFalse(); + }); + it('filtering out current selected item and then clicking selected option changes value and closes popup', async () => { const currentSelection = pageObject.getSelectedOption(); expect(currentSelection?.text).toBe('One'); @@ -754,6 +771,29 @@ describe('Select', () => { expect(element.value).toBe('one'); }); + it('filtering to only disabled item, then pressing closes popup and does not change value or selectedIndex', async () => { + await pageObject.openAndSetFilterText('Disabled'); + pageObject.pressEscapeKey(); + expect(element.open).toBeFalse(); + expect(element.value).toBe('one'); + expect(element.selectedIndex).toBe(0); + }); + + it('filtering to only disabled item, then pressing closes popup and does not change value or selectedIndex', async () => { + await pageObject.openAndSetFilterText('Disabled'); + pageObject.pressTabKey(); + expect(element.open).toBeFalse(); + expect(element.value).toBe('one'); + expect(element.selectedIndex).toBe(0); + }); + + it('filtering to no available options, then pressing does not close popup or change value', async () => { + await pageObject.openAndSetFilterText('abc'); + pageObject.pressEnterKey(); + expect(element.open).toBeTrue(); + expect(element.value).toBe('one'); + }); + it('filtering to no available options, then pressing does not close popup or change value', async () => { await pageObject.openAndSetFilterText('abc'); pageObject.pressEnterKey(); @@ -838,7 +878,7 @@ describe('Select', () => { expect(currentSelection?.value).toBe('one'); }); - it('cant not select option that has been filtered out pressing arrowDown', async () => { + it('can not select option that has been filtered out pressing arrowDown', async () => { await pageObject.openAndSetFilterText('tw'); pageObject.pressArrowDownKey(); pageObject.pressEnterKey(); From cd06c39d7ed9e2abc56938c8b83071be7bcf3736 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 22 Apr 2024 13:29:34 -0500 Subject: [PATCH 02/34] Change files --- ...le-components-9da2496e-e544-4019-a666-d7fca3046771.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json diff --git a/change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json b/change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json new file mode 100644 index 0000000000..b88159e586 --- /dev/null +++ b/change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json @@ -0,0 +1,7 @@ +{ + "type": "major", + "comment": "Change Select to not update value as you navigate options in dropdown.", + "packageName": "@ni/nimble-components", + "email": "26874831+atmgrifter00@users.noreply.github.com", + "dependentChangeType": "patch" +} From 26700f0ec39c277e466ff4b50ccf7a32fd811e0e Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 22 Apr 2024 14:35:32 -0500 Subject: [PATCH 03/34] Slight renaming. --- .../nimble-components/src/select/index.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 58ed4d0404..284ece553a 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -687,7 +687,7 @@ export class Select // don't call super.selectNextOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) const startIndex = this.openActiveIndex ?? this.selectedIndex; - const newActiveOptionFunc = (): number => { + const getNewActiveOptionIndex = (): number => { for (let i = startIndex + 1; i < this.options.length; i++) { const listOption = this.options[i]!; if ( @@ -699,14 +699,14 @@ export class Select } return -1; }; - this.toggleNewActiveOption(newActiveOptionFunc); + this.toggleNewActiveOption(getNewActiveOptionIndex); } public override selectPreviousOption(): void { // don't call super.selectPreviousOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) const startIndex = this.openActiveIndex ?? this.selectedIndex; - const newActiveOptionFunc = (): number => { + const getNewActiveOptionIndex = (): number => { for (let i = startIndex - 1; i >= 0; i--) { const listOption = this.options[i]!; if ( @@ -718,26 +718,26 @@ export class Select } return -1; }; - this.toggleNewActiveOption(newActiveOptionFunc); + this.toggleNewActiveOption(getNewActiveOptionIndex); } public override selectFirstOption(): void { - const newActiveOptionFunc = (): number => { + const getNewActiveOptionIndex = (): number => { return this.options.findIndex( o => isNimbleListOption(o) && isOptionSelectable(o) ); }; - this.toggleNewActiveOption(newActiveOptionFunc); + this.toggleNewActiveOption(getNewActiveOptionIndex); } public override selectLastOption(): void { - const newActiveOptionFunc = (): number => { + const getNewActiveOptionIndex = (): number => { return findLastIndex( this.options, o => isNimbleListOption(o) && isOptionSelectable(o) ); }; - this.toggleNewActiveOption(newActiveOptionFunc); + this.toggleNewActiveOption(getNewActiveOptionIndex); } /** @@ -891,11 +891,11 @@ export class Select this.committedSelectedOption = options[this.selectedIndex]; } - private toggleNewActiveOption(newActiveIndexFunc: () => number): void { + private toggleNewActiveOption(getNewActiveIndex: () => number): void { const selectedOption = this.options[ this.openActiveIndex ?? this.selectedIndex ] as ListOption; - const activeIndex = newActiveIndexFunc(); + const activeIndex = getNewActiveIndex(); if (activeIndex >= 0) { this.options[activeIndex]!.ariaSelected = 'true'; selectedOption.ariaSelected = 'false'; From 2984113cdffcfbc7e317a9c46a3969fd65b5120d Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 22 Apr 2024 14:57:57 -0500 Subject: [PATCH 04/34] Minor change with test. --- packages/nimble-components/src/select/index.ts | 7 +++++-- .../src/select/tests/select.spec.ts | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 284ece553a..91942b18a4 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -894,11 +894,14 @@ export class Select private toggleNewActiveOption(getNewActiveIndex: () => number): void { const selectedOption = this.options[ this.openActiveIndex ?? this.selectedIndex - ] as ListOption; + ]; const activeIndex = getNewActiveIndex(); if (activeIndex >= 0) { this.options[activeIndex]!.ariaSelected = 'true'; - selectedOption.ariaSelected = 'false'; + if (selectedOption) { + selectedOption.ariaSelected = 'false'; + } + if (this.open) { this.openActiveIndex = activeIndex; } else { diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 2752f9eb3d..769925c8ea 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -329,6 +329,21 @@ describe('Select', () => { await disconnect(); }); + fit('after forcing Select to be blank, user can arrow down to first available option in dropdown', async () => { + const { element, connect, disconnect } = await setup(); + const pageObject = new SelectPageObject(element); + await connect(); + await waitForUpdatesAsync(); + element.selectedIndex = -1; + await waitForUpdatesAsync(); + + pageObject.clickSelect(); + pageObject.pressArrowDownKey(); + expect(pageObject.getSelectedOption()?.value).toBe('one') + + await disconnect(); + }) + it('option added directly to DOM synchronously registers with Select', async () => { const { element, connect, disconnect } = await setup(); await connect(); From 6b089fa7d303b1b6879958abd464088387404915 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 22 Apr 2024 15:07:26 -0500 Subject: [PATCH 05/34] Remove fit. --- packages/nimble-components/src/select/tests/select.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 769925c8ea..92305f39a0 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -329,7 +329,7 @@ describe('Select', () => { await disconnect(); }); - fit('after forcing Select to be blank, user can arrow down to first available option in dropdown', async () => { + it('after forcing Select to be blank, user can arrow down to first available option in dropdown', async () => { const { element, connect, disconnect } = await setup(); const pageObject = new SelectPageObject(element); await connect(); From 2a65f92cc579d81e3fc309629ee0157835f24c8f Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 22 Apr 2024 15:20:22 -0500 Subject: [PATCH 06/34] Linting --- packages/nimble-components/src/select/tests/select.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 92305f39a0..583c93bb0e 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -339,10 +339,10 @@ describe('Select', () => { pageObject.clickSelect(); pageObject.pressArrowDownKey(); - expect(pageObject.getSelectedOption()?.value).toBe('one') + expect(pageObject.getSelectedOption()?.value).toBe('one'); await disconnect(); - }) + }); it('option added directly to DOM synchronously registers with Select', async () => { const { element, connect, disconnect } = await setup(); From 652642f8018b2fd75322064b23c1755a1a85358e Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 22 Apr 2024 15:35:10 -0500 Subject: [PATCH 07/34] prettier --- packages/nimble-components/src/select/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 91942b18a4..f6c47d8972 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -892,9 +892,7 @@ export class Select } private toggleNewActiveOption(getNewActiveIndex: () => number): void { - const selectedOption = this.options[ - this.openActiveIndex ?? this.selectedIndex - ]; + const selectedOption = this.options[this.openActiveIndex ?? this.selectedIndex]; const activeIndex = getNewActiveIndex(); if (activeIndex >= 0) { this.options[activeIndex]!.ariaSelected = 'true'; From a4709e3e7e84e47306d7adcdeb50e4c28fd758ad Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Tue, 23 Apr 2024 17:48:48 -0500 Subject: [PATCH 08/34] Fix issue with Listbox typeahead changing selectedIndex. --- ...-9da2496e-e544-4019-a666-d7fca3046771.json | 2 +- .../nimble-components/src/select/index.ts | 44 ++++++++++---- .../src/select/testing/select.pageobject.ts | 23 +++++++ .../src/select/tests/select.spec.ts | 60 +++++++++++++++++-- 4 files changed, 112 insertions(+), 17 deletions(-) diff --git a/change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json b/change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json index b88159e586..3cdc357bd2 100644 --- a/change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json +++ b/change/@ni-nimble-components-9da2496e-e544-4019-a666-d7fca3046771.json @@ -1,5 +1,5 @@ { - "type": "major", + "type": "patch", "comment": "Change Select to not update value as you navigate options in dropdown.", "packageName": "@ni/nimble-components", "email": "26874831+atmgrifter00@users.noreply.github.com", diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index f6c47d8972..d164133602 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -481,8 +481,10 @@ export class Select o => !o.disabled ); if (enabledOptions.length > 0) { - enabledOptions[0]!.ariaSelected = 'true'; - this.openActiveIndex = this.options.indexOf(enabledOptions[0]!); + const selectedOption = enabledOptions.find(o => o === this.committedSelectedOption) + ?? enabledOptions[0]!; + selectedOption.ariaSelected = 'true'; + this.openActiveIndex = this.options.indexOf(selectedOption); } else { // only filtered option is disabled this.openActiveIndex = -1; @@ -588,13 +590,12 @@ export class Select this.open = false; } - if (currentActiveIndex !== this.indexWhenOpened!) { - if (currentActiveIndex >= 0) { - this.options[currentActiveIndex]!.ariaSelected = 'false'; - } - this.options[this.indexWhenOpened!]!.ariaSelected = 'true'; - currentActiveIndex = this.indexWhenOpened!; + if (currentActiveIndex !== this.indexWhenOpened! && currentActiveIndex >= 0) { + this.options[currentActiveIndex]!.ariaSelected = 'false'; } + + this.options[this.indexWhenOpened!]!.ariaSelected = 'true'; + currentActiveIndex = this.indexWhenOpened!; this.focus(); break; } @@ -643,14 +644,23 @@ export class Select */ public override selectedIndexChanged( _: number | undefined, - __: number + next: number ): void { // Don't call super.selectedIndexChanged as this will disallow disabled options // from being valid initial selected values. Our setDefaultSelectedOption // implementation handles skipping non-selected disabled options for the initial // selected value. - this.setSelectedOptions(); - this.updateValue(); + if (!this.open) { + this.setSelectedOptions(); + this.updateValue(); + } else { + // Prohibit changing selectedIndex when dropdown is open. Instead, update + // openActiveIndex to what selectedIndex was being set to. + if (next !== this.indexWhenOpened) { + this.selectedIndex = this.indexWhenOpened ?? -1; + } + this.toggleNewActiveOption(() => { return next; }); + } } /** @@ -683,6 +693,9 @@ export class Select } } + /** + * @internal + */ public override selectNextOption(): void { // don't call super.selectNextOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) @@ -702,6 +715,9 @@ export class Select this.toggleNewActiveOption(getNewActiveOptionIndex); } + /** + * @internal + */ public override selectPreviousOption(): void { // don't call super.selectPreviousOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) @@ -721,6 +737,9 @@ export class Select this.toggleNewActiveOption(getNewActiveOptionIndex); } + /** + * @internal + */ public override selectFirstOption(): void { const getNewActiveOptionIndex = (): number => { return this.options.findIndex( @@ -730,6 +749,9 @@ export class Select this.toggleNewActiveOption(getNewActiveOptionIndex); } + /** + * @internal + */ public override selectLastOption(): void { const getNewActiveOptionIndex = (): number => { return findLastIndex( diff --git a/packages/nimble-components/src/select/testing/select.pageobject.ts b/packages/nimble-components/src/select/testing/select.pageobject.ts index 74478517c9..f284f786da 100644 --- a/packages/nimble-components/src/select/testing/select.pageobject.ts +++ b/packages/nimble-components/src/select/testing/select.pageobject.ts @@ -61,6 +61,12 @@ export class SelectPageObject { return (this.selectElement.selectedOptions[0] as ListOption) ?? null; } + public getDisplayText(): string { + const displayText = this.selectElement.shadowRoot?.querySelector('.selected-value') + ?.textContent ?? ''; + return displayText.trim(); + } + /** * Either opens or closes the dropdown depending on its current state */ @@ -153,6 +159,23 @@ export class SelectPageObject { ); } + public pressCharacterKey(character: string) { + if (character.length !== 1) { + throw new Error('character parameter must contain only a single character'); + } + + if (this.selectElement.open && this.selectElement.filterMode !== FilterMode.none) { + const filterInput = this.getFilterInput(); + filterInput!.value = filterInput!.value + character; + } + this.selectElement.dispatchEvent( + new InputEvent('input') + ); + this.selectElement.dispatchEvent( + new KeyboardEvent('keydown', { key: character }) + ); + }; + public async pressSpaceKey(): Promise { const alreadyOpen = this.selectElement.open; this.selectElement.dispatchEvent( diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 583c93bb0e..2402148499 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -199,7 +199,7 @@ describe('Select', () => { pageObject.pressArrowDownKey(); await waitForUpdatesAsync(); - expect(element.displayValue).toBe('One'); + expect(pageObject.getDisplayText()).toBe('One'); await disconnect(); }); @@ -324,7 +324,7 @@ describe('Select', () => { const selectedOption = pageObject.getSelectedOption(); selectedOption!.textContent = 'foo'; await waitForUpdatesAsync(); - expect(element.displayValue).toBe('foo'); + expect(pageObject.getDisplayText()).toBe('foo'); await disconnect(); }); @@ -344,6 +344,34 @@ describe('Select', () => { await disconnect(); }); + it('selecting option via typing character while dropdown is closed changes value', async () => { + const { element, connect, disconnect } = await setup(); + const pageObject = new SelectPageObject(element); + await connect(); + await waitForUpdatesAsync(); + pageObject.pressCharacterKey('t'); + await waitForUpdatesAsync(); + + expect(element.value).toBe('two'); + + await disconnect(); + }); + + it('selecting option via typing character while dropdown is open with no filter does not change value', async () => { + const { element, connect, disconnect } = await setup(); + const pageObject = new SelectPageObject(element); + await connect(); + await waitForUpdatesAsync(); + pageObject.clickSelect(); + pageObject.pressCharacterKey('t'); + await waitForUpdatesAsync(); + + expect(element.value).toBe('one'); + expect(pageObject.getSelectedOption()?.value).toBe('two'); + + await disconnect(); + }); + it('option added directly to DOM synchronously registers with Select', async () => { const { element, connect, disconnect } = await setup(); await connect(); @@ -907,6 +935,28 @@ describe('Select', () => { currentSelection = pageObject.getSelectedOption(); expect(currentSelection?.value).toBe('three'); }); + + fit('when dropdown is closed, entering text executes typeahead and sets value', async () => { + pageObject.pressCharacterKey('t'); + expect(element.value).toBe('two'); + }); + + fit('opening dropdown after pressing during filter text entry, maintains original display text', async () => { + await clickAndWaitForOpen(element); + pageObject.pressCharacterKey('t'); + pageObject.pressEscapeKey(); + await pageObject.clickSelect(); + await waitForUpdatesAsync(); + + expect(pageObject.getDisplayText()).toBe('One'); + }); + + fit('filtering options does not change selected option in dropdown', async () => { + element.value = 'three'; + await pageObject.openAndSetFilterText('t'); // filters to 'Two' and 'Three' + + expect(pageObject.getSelectedOption()?.value).toBe('three'); + }) }); describe('placeholder', () => { @@ -937,12 +987,12 @@ describe('Select', () => { }); it('selecting option will replace placeholder text with selected option text', async () => { - expect(element.displayValue).toBe('One'); + expect(pageObject.getDisplayText()).toBe('One'); await clickAndWaitForOpen(element); pageObject.clickOption(1); await waitForUpdatesAsync(); - expect(element.displayValue).toBe('Two'); + expect(pageObject.getDisplayText()).toBe('Two'); }); it('placeholder can be changed to another option programmatically', async () => { @@ -953,7 +1003,7 @@ describe('Select', () => { element.options[1]!.selected = true; await waitForUpdatesAsync(); - expect(element.displayValue).toBe('Two'); + expect(pageObject.getDisplayText()).toBe('Two'); expect(element.value).toBe('two'); await clickAndWaitForOpen(element); expect(pageObject.isOptionVisible(0)).toBeTrue(); From 11c2611adf3cce2641091e6efbce8cb7f9e6d10e Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Tue, 23 Apr 2024 18:01:12 -0500 Subject: [PATCH 09/34] Linting. --- packages/nimble-components/src/select/index.ts | 14 ++++++++++---- .../src/select/testing/select.pageobject.ts | 17 ++++++++++------- .../src/select/tests/select.spec.ts | 10 +++++----- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index d164133602..ede6b8cefd 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -481,8 +481,9 @@ export class Select o => !o.disabled ); if (enabledOptions.length > 0) { - const selectedOption = enabledOptions.find(o => o === this.committedSelectedOption) - ?? enabledOptions[0]!; + const selectedOption = enabledOptions.find( + o => o === this.committedSelectedOption + ) ?? enabledOptions[0]!; selectedOption.ariaSelected = 'true'; this.openActiveIndex = this.options.indexOf(selectedOption); } else { @@ -590,7 +591,10 @@ export class Select this.open = false; } - if (currentActiveIndex !== this.indexWhenOpened! && currentActiveIndex >= 0) { + if ( + currentActiveIndex !== this.indexWhenOpened! + && currentActiveIndex >= 0 + ) { this.options[currentActiveIndex]!.ariaSelected = 'false'; } @@ -659,7 +663,9 @@ export class Select if (next !== this.indexWhenOpened) { this.selectedIndex = this.indexWhenOpened ?? -1; } - this.toggleNewActiveOption(() => { return next; }); + this.toggleNewActiveOption(() => { + return next; + }); } } diff --git a/packages/nimble-components/src/select/testing/select.pageobject.ts b/packages/nimble-components/src/select/testing/select.pageobject.ts index f284f786da..d47a6a0467 100644 --- a/packages/nimble-components/src/select/testing/select.pageobject.ts +++ b/packages/nimble-components/src/select/testing/select.pageobject.ts @@ -159,22 +159,25 @@ export class SelectPageObject { ); } - public pressCharacterKey(character: string) { + public pressCharacterKey(character: string): void { if (character.length !== 1) { - throw new Error('character parameter must contain only a single character'); + throw new Error( + 'character parameter must contain only a single character' + ); } - if (this.selectElement.open && this.selectElement.filterMode !== FilterMode.none) { + if ( + this.selectElement.open + && this.selectElement.filterMode !== FilterMode.none + ) { const filterInput = this.getFilterInput(); filterInput!.value = filterInput!.value + character; } - this.selectElement.dispatchEvent( - new InputEvent('input') - ); + this.selectElement.dispatchEvent(new InputEvent('input')); this.selectElement.dispatchEvent( new KeyboardEvent('keydown', { key: character }) ); - }; + } public async pressSpaceKey(): Promise { const alreadyOpen = this.selectElement.open; diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 2402148499..291fce8d6e 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -936,27 +936,27 @@ describe('Select', () => { expect(currentSelection?.value).toBe('three'); }); - fit('when dropdown is closed, entering text executes typeahead and sets value', async () => { + it('when dropdown is closed, entering text executes typeahead and sets value', () => { pageObject.pressCharacterKey('t'); expect(element.value).toBe('two'); }); - fit('opening dropdown after pressing during filter text entry, maintains original display text', async () => { + it('opening dropdown after pressing during filter text entry, maintains original display text', async () => { await clickAndWaitForOpen(element); pageObject.pressCharacterKey('t'); pageObject.pressEscapeKey(); - await pageObject.clickSelect(); + pageObject.clickSelect(); await waitForUpdatesAsync(); expect(pageObject.getDisplayText()).toBe('One'); }); - fit('filtering options does not change selected option in dropdown', async () => { + it('filtering options does not change selected option in dropdown', async () => { element.value = 'three'; await pageObject.openAndSetFilterText('t'); // filters to 'Two' and 'Three' expect(pageObject.getSelectedOption()?.value).toBe('three'); - }) + }); }); describe('placeholder', () => { From 25b92134f534270934e7429db63af48409e40dc0 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Wed, 24 Apr 2024 10:44:56 -0500 Subject: [PATCH 10/34] Revert previous solution for dealing with typeahead, and instead provide override for typeaheadBufferChanged. --- .../nimble-components/src/select/index.ts | 59 ++++++++++++++----- .../src/select/tests/select.spec.ts | 11 ++++ 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index ede6b8cefd..561265cd9d 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -648,24 +648,39 @@ export class Select */ public override selectedIndexChanged( _: number | undefined, - next: number + __: number ): void { // Don't call super.selectedIndexChanged as this will disallow disabled options // from being valid initial selected values. Our setDefaultSelectedOption // implementation handles skipping non-selected disabled options for the initial // selected value. - if (!this.open) { - this.setSelectedOptions(); - this.updateValue(); - } else { - // Prohibit changing selectedIndex when dropdown is open. Instead, update - // openActiveIndex to what selectedIndex was being set to. - if (next !== this.indexWhenOpened) { - this.selectedIndex = this.indexWhenOpened ?? -1; + this.setSelectedOptions(); + this.updateValue(); + } + + /** + * @internal + * Fork of Listbox implementation, so that the selectedIndex is not changed while the dropdown + * is open. + */ + public override typeaheadBufferChanged(_: string, __: string): void { + if (this.$fastController.isConnected) { + const typeaheadMatches = this.getTypeaheadMatches(); + + if (typeaheadMatches.length) { + const selectedIndex = this.options.indexOf( + typeaheadMatches[0] as ListOption + ); + if (selectedIndex > -1 && !this.open) { + this.selectedIndex = selectedIndex; + } else { + this.toggleNewActiveOption(() => { + return selectedIndex; + }); + } } - this.toggleNewActiveOption(() => { - return next; - }); + + this.typeaheadExpired = false; } } @@ -805,6 +820,20 @@ export class Select } } + /** + * Fork of Listbox implementation to prevent placeholder option from being selected. + */ + protected override getTypeaheadMatches(): ListboxOption[] { + const pattern = this.typeaheadBuffer.replace( + /[.*+\-?^${}()|[\]\\]/g, + '\\$&' + ); + const re = new RegExp(`^${pattern}`, 'gi'); + return this.options.filter( + (o: ListboxOption) => !o.hidden && !o.disabled && o.text.trim().match(re) + ); + } + protected positionChanged( _: SelectPosition | undefined, next: SelectPosition | undefined @@ -920,12 +949,12 @@ export class Select } private toggleNewActiveOption(getNewActiveIndex: () => number): void { - const selectedOption = this.options[this.openActiveIndex ?? this.selectedIndex]; + const selectedIndex = this.openActiveIndex ?? this.selectedIndex; const activeIndex = getNewActiveIndex(); if (activeIndex >= 0) { this.options[activeIndex]!.ariaSelected = 'true'; - if (selectedOption) { - selectedOption.ariaSelected = 'false'; + if (selectedIndex !== activeIndex && selectedIndex > -1) { + this.options[selectedIndex]!.ariaSelected = 'false'; } if (this.open) { diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 291fce8d6e..034bfc7f0a 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -975,6 +975,7 @@ describe('Select', () => { element.filterMode = FilterMode.standard; await connect(); pageObject = new SelectPageObject(element); + await waitForUpdatesAsync(); }); afterEach(async () => { @@ -1009,6 +1010,16 @@ describe('Select', () => { expect(pageObject.isOptionVisible(0)).toBeTrue(); expect(pageObject.isOptionVisible(1)).toBeFalse(); }); + + it('selecting option via typing will not select placeholder', async () => { + const newOptions = element.options.map(o => o as ListOption); + newOptions.push(new ListOption('One one', 'one one')); + await pageObject.setOptions(newOptions); + expect(pageObject.getDisplayText()).toBe('One'); + pageObject.pressCharacterKey('o'); + await waitForUpdatesAsync(); + expect(pageObject.getDisplayText()).toBe('One one'); + }); }); describe('PageObject', () => { From 2a1fd878a8333b54cbb7e88b4d1358fd0281bf18 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Wed, 24 Apr 2024 10:48:39 -0500 Subject: [PATCH 11/34] Test doc update. --- .../src/select/tests/select.foundation.spec.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/nimble-components/src/select/tests/select.foundation.spec.ts b/packages/nimble-components/src/select/tests/select.foundation.spec.ts index 8c606fa850..8405d03cf5 100644 --- a/packages/nimble-components/src/select/tests/select.foundation.spec.ts +++ b/packages/nimble-components/src/select/tests/select.foundation.spec.ts @@ -1,4 +1,8 @@ -// Based on tests in FAST repo: https://github.com/microsoft/fast/blob/085cb27d348ed6f59d080c167fa62aeaa1e3940e/packages/web-components/fast-foundation/src/select/select.spec.ts +/** + * Based on tests in FAST repo: https://github.com/microsoft/fast/blob/085cb27d348ed6f59d080c167fa62aeaa1e3940e/packages/web-components/fast-foundation/src/select/select.spec.ts + * One notable change to the tests is that we no longer expect the selectedIndex to change for the Select as the user + * navigates the dropdown menu. As as result, some test names, and the relevant expect assertions have changed. + */ import { keyArrowDown, keyArrowUp, From 8f69548bea3ae430bb0af84fe4e004db57306568 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Wed, 24 Apr 2024 11:05:32 -0500 Subject: [PATCH 12/34] Code clean up. --- .../nimble-components/src/select/index.ts | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 561265cd9d..4319057bde 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -586,19 +586,16 @@ export class Select if (!this.open) { break; } + + this.toggleNewActiveOption(() => { + return this.indexWhenOpened ?? this.selectedIndex + }); + if (this.collapsible && this.open) { e.preventDefault(); this.open = false; } - if ( - currentActiveIndex !== this.indexWhenOpened! - && currentActiveIndex >= 0 - ) { - this.options[currentActiveIndex]!.ariaSelected = 'false'; - } - - this.options[this.indexWhenOpened!]!.ariaSelected = 'true'; currentActiveIndex = this.indexWhenOpened!; this.focus(); break; @@ -609,10 +606,9 @@ export class Select this.filteredOptions.length === 0 || this.filteredOptions.every(o => o.disabled) ) { - if (currentActiveIndex >= 0) { - this.options[currentActiveIndex]!.ariaSelected = 'false'; - } - this.options[this.indexWhenOpened!]!.ariaSelected = 'true'; + this.toggleNewActiveOption(() => { + return this.indexWhenOpened ?? this.selectedIndex + }); currentActiveIndex = this.indexWhenOpened!; this.open = false; return true; From 08e2f7c5903a13f798d2ff59ce80ecf001360a81 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Wed, 24 Apr 2024 11:17:01 -0500 Subject: [PATCH 13/34] Linting. --- packages/nimble-components/src/select/index.ts | 4 ++-- packages/nimble-components/src/select/tests/select.spec.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 4319057bde..baa55095de 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -588,7 +588,7 @@ export class Select } this.toggleNewActiveOption(() => { - return this.indexWhenOpened ?? this.selectedIndex + return this.indexWhenOpened ?? this.selectedIndex; }); if (this.collapsible && this.open) { @@ -607,7 +607,7 @@ export class Select || this.filteredOptions.every(o => o.disabled) ) { this.toggleNewActiveOption(() => { - return this.indexWhenOpened ?? this.selectedIndex + return this.indexWhenOpened ?? this.selectedIndex; }); currentActiveIndex = this.indexWhenOpened!; this.open = false; diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 034bfc7f0a..84015b2267 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -353,7 +353,7 @@ describe('Select', () => { await waitForUpdatesAsync(); expect(element.value).toBe('two'); - + await disconnect(); }); @@ -368,7 +368,7 @@ describe('Select', () => { expect(element.value).toBe('one'); expect(pageObject.getSelectedOption()?.value).toBe('two'); - + await disconnect(); }); From 2cde4ea449f1fe93e019d60ab8c4cf7ca9493fda Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Wed, 24 Apr 2024 17:24:34 -0500 Subject: [PATCH 14/34] Handle PR feedback. --- .../src/list-option/index.ts | 11 ++ .../src/list-option/styles.ts | 4 +- .../nimble-components/src/select/index.ts | 111 +++++++++--------- .../src/select/testing/select.pageobject.ts | 22 ++-- .../src/select/tests/select.spec.ts | 29 ++--- 5 files changed, 93 insertions(+), 84 deletions(-) diff --git a/packages/nimble-components/src/list-option/index.ts b/packages/nimble-components/src/list-option/index.ts index 6f60d343e2..a529ebd1d8 100644 --- a/packages/nimble-components/src/list-option/index.ts +++ b/packages/nimble-components/src/list-option/index.ts @@ -41,6 +41,17 @@ export class ListOption extends FoundationListboxOption { @attr({ attribute: 'visually-hidden', mode: 'boolean' }) public visuallyHidden = false; + /** + * @internal + * This attribute is used to control the visual selected state of an option. This + * is handled independently of the public 'selected' attribute, as 'selected' is + * representative of the current value of the container control. However, while + * a dropdown is open users can navigate through the options (requiring visual + * updates) without changing the value of the container control. + */ + @attr({ attribute: 'active-option', mode: 'boolean' }) + public activeOption = false; + /** @internal */ @observable public hasOverflow = false; diff --git a/packages/nimble-components/src/list-option/styles.ts b/packages/nimble-components/src/list-option/styles.ts index 54d499448c..4199d82c59 100644 --- a/packages/nimble-components/src/list-option/styles.ts +++ b/packages/nimble-components/src/list-option/styles.ts @@ -22,13 +22,13 @@ export const styles = css` height: ${controlHeight}; } - :host([aria-selected='true']) { + :host([active-option]) { box-shadow: none; outline: none; background-color: ${fillSelectedColor}; } - :host([aria-selected='true']:hover) { + :host([active-option]:hover) { background-color: ${fillHoverSelectedColor}; } diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index baa55095de..5eede01849 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -54,7 +54,7 @@ declare global { // eslint-disable-next-line @typescript-eslint/no-invalid-void-type type BooleanOrVoid = boolean | void; -const isNimbleListOption = (el: Element): el is ListOption => { +const isNimbleListOption = (el: Element | undefined): el is ListOption => { return el instanceof ListOption; }; @@ -484,7 +484,7 @@ export class Select const selectedOption = enabledOptions.find( o => o === this.committedSelectedOption ) ?? enabledOptions[0]!; - selectedOption.ariaSelected = 'true'; + (selectedOption as ListOption).activeOption = true; this.openActiveIndex = this.options.indexOf(selectedOption); } else { // only filtered option is disabled @@ -524,7 +524,9 @@ export class Select this.selectedIndex = this.indexWhenOpened!; currentActiveIndex = this.selectedIndex; if (this.selectedIndex >= 0) { - this.options[this.selectedIndex]!.ariaSelected = 'true'; + ( + this.options[this.selectedIndex]! as ListOption + ).activeOption = true; } } @@ -587,9 +589,9 @@ export class Select break; } - this.toggleNewActiveOption(() => { - return this.indexWhenOpened ?? this.selectedIndex; - }); + this.toggleNewActiveOption( + this.indexWhenOpened ?? this.selectedIndex + ); if (this.collapsible && this.open) { e.preventDefault(); @@ -606,9 +608,9 @@ export class Select this.filteredOptions.length === 0 || this.filteredOptions.every(o => o.disabled) ) { - this.toggleNewActiveOption(() => { - return this.indexWhenOpened ?? this.selectedIndex; - }); + this.toggleNewActiveOption( + this.indexWhenOpened ?? this.selectedIndex + ); currentActiveIndex = this.indexWhenOpened!; this.open = false; return true; @@ -670,9 +672,7 @@ export class Select if (selectedIndex > -1 && !this.open) { this.selectedIndex = selectedIndex; } else { - this.toggleNewActiveOption(() => { - return selectedIndex; - }); + this.toggleNewActiveOption(selectedIndex); } } @@ -717,7 +717,7 @@ export class Select // don't call super.selectNextOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) const startIndex = this.openActiveIndex ?? this.selectedIndex; - const getNewActiveOptionIndex = (): number => { + const newActiveOptionIndex = (): number => { for (let i = startIndex + 1; i < this.options.length; i++) { const listOption = this.options[i]!; if ( @@ -729,7 +729,7 @@ export class Select } return -1; }; - this.toggleNewActiveOption(getNewActiveOptionIndex); + this.toggleNewActiveOption(newActiveOptionIndex()); } /** @@ -739,7 +739,7 @@ export class Select // don't call super.selectPreviousOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) const startIndex = this.openActiveIndex ?? this.selectedIndex; - const getNewActiveOptionIndex = (): number => { + const newActiveOptionIndex = (): number => { for (let i = startIndex - 1; i >= 0; i--) { const listOption = this.options[i]!; if ( @@ -751,32 +751,28 @@ export class Select } return -1; }; - this.toggleNewActiveOption(getNewActiveOptionIndex); + this.toggleNewActiveOption(newActiveOptionIndex()); } /** * @internal */ public override selectFirstOption(): void { - const getNewActiveOptionIndex = (): number => { - return this.options.findIndex( - o => isNimbleListOption(o) && isOptionSelectable(o) - ); - }; - this.toggleNewActiveOption(getNewActiveOptionIndex); + const newActiveOptionIndex = this.options.findIndex( + o => isNimbleListOption(o) && isOptionSelectable(o) + ); + this.toggleNewActiveOption(newActiveOptionIndex); } /** * @internal */ public override selectLastOption(): void { - const getNewActiveOptionIndex = (): number => { - return findLastIndex( - this.options, - o => isNimbleListOption(o) && isOptionSelectable(o) - ); - }; - this.toggleNewActiveOption(getNewActiveOptionIndex); + const newActiveOptionIndex = findLastIndex( + this.options, + o => isNimbleListOption(o) && isOptionSelectable(o) + ); + this.toggleNewActiveOption(newActiveOptionIndex); } /** @@ -816,18 +812,10 @@ export class Select } } - /** - * Fork of Listbox implementation to prevent placeholder option from being selected. - */ protected override getTypeaheadMatches(): ListboxOption[] { - const pattern = this.typeaheadBuffer.replace( - /[.*+\-?^${}()|[\]\\]/g, - '\\$&' - ); - const re = new RegExp(`^${pattern}`, 'gi'); - return this.options.filter( - (o: ListboxOption) => !o.hidden && !o.disabled && o.text.trim().match(re) - ); + const matches = super.getTypeaheadMatches(); + // Don't allow placeholder to be matched + return matches.filter(o => !o.hidden && !o.disabled); } protected positionChanged( @@ -944,20 +932,39 @@ export class Select this.committedSelectedOption = options[this.selectedIndex]; } - private toggleNewActiveOption(getNewActiveIndex: () => number): void { + private toggleNewActiveOption(newActiveIndex: number): void { const selectedIndex = this.openActiveIndex ?? this.selectedIndex; - const activeIndex = getNewActiveIndex(); - if (activeIndex >= 0) { - this.options[activeIndex]!.ariaSelected = 'true'; - if (selectedIndex !== activeIndex && selectedIndex > -1) { - this.options[selectedIndex]!.ariaSelected = 'false'; + const activeOption = this.options[newActiveIndex]; + if (newActiveIndex >= 0 && isNimbleListOption(activeOption)) { + activeOption.activeOption = true; + if (selectedIndex !== newActiveIndex && selectedIndex > -1) { + (this.options[selectedIndex]! as ListOption).activeOption = false; } if (this.open) { - this.openActiveIndex = activeIndex; + this.openActiveIndex = newActiveIndex; } else { - this.selectedIndex = activeIndex; + this.selectedIndex = newActiveIndex; } + this.focusAndScrollActiveOptionIntoView(); + } + } + + private focusAndScrollActiveOptionIntoView(): void { + const optionToFocus = this.options[this.openActiveIndex ?? this.selectedIndex]; + // Copied from FAST: To ensure that the browser handles both `focus()` and + // `scrollIntoView()`, the timing here needs to guarantee that they happen on + // different frames. Since this function is typically called from the `openChanged` + // observer, `DOM.queueUpdate` causes the calls to be grouped into the same frame. + // To prevent this, `requestAnimationFrame` is used instead of `DOM.queueUpdate`. + if ( + this.contains(document.activeElement) + && optionToFocus !== undefined + ) { + optionToFocus.focus(); + requestAnimationFrame(() => { + optionToFocus.scrollIntoView({ block: 'nearest' }); + }); } } @@ -1072,7 +1079,7 @@ export class Select private clearSelection(): void { this.options.forEach(option => { - option.ariaSelected = 'false'; + (option as ListOption).activeOption = false; }); } @@ -1085,12 +1092,6 @@ export class Select } private initializeOpenState(): void { - if (!this.open) { - this.ariaExpanded = 'false'; - this.ariaControls = ''; - return; - } - this.indexWhenOpened = this.selectedIndex; this.openActiveIndex = this.selectedIndex; this.committedSelectedOption = this.options[this.selectedIndex]; diff --git a/packages/nimble-components/src/select/testing/select.pageobject.ts b/packages/nimble-components/src/select/testing/select.pageobject.ts index d47a6a0467..9006922d8f 100644 --- a/packages/nimble-components/src/select/testing/select.pageobject.ts +++ b/packages/nimble-components/src/select/testing/select.pageobject.ts @@ -54,13 +54,17 @@ export class SelectPageObject { } public getSelectedOption(): ListOption | null { - if (this.selectElement.open) { - return this.getSelectedDropdownOption(); - } - return (this.selectElement.selectedOptions[0] as ListOption) ?? null; } + public getActiveOption(): ListOption | null { + return ( + (this.selectElement.options.find( + o => (o as ListOption).activeOption === true + ) as ListOption) ?? null + ); + } + public getDisplayText(): string { const displayText = this.selectElement.shadowRoot?.querySelector('.selected-value') ?.textContent ?? ''; @@ -79,7 +83,7 @@ export class SelectPageObject { throw new Error('Select must be open to click selectedItem'); } - const selectedOption = this.getSelectedDropdownOption(); + const selectedOption = this.getActiveOption(); if (!selectedOption) { throw new Error('No option is selected to click'); } @@ -247,12 +251,4 @@ export class SelectPageObject { '.filter-input' ); } - - private getSelectedDropdownOption(): ListOption | null { - return ( - (this.selectElement.options.find( - o => o.ariaSelected === 'true' - ) as ListOption) ?? null - ); - } } diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 84015b2267..ac01f89e02 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -308,10 +308,10 @@ describe('Select', () => { await waitForUpdatesAsync(); await clickAndWaitForOpen(element); pageObject.pressArrowDownKey(); - expect(pageObject.getSelectedOption()?.value).toBe('three'); + expect(pageObject.getActiveOption()?.value).toBe('three'); pageObject.pressArrowUpKey(); - expect(pageObject.getSelectedOption()?.value).toBe('one'); + expect(pageObject.getActiveOption()?.value).toBe('one'); await disconnect(); }); @@ -339,7 +339,7 @@ describe('Select', () => { pageObject.clickSelect(); pageObject.pressArrowDownKey(); - expect(pageObject.getSelectedOption()?.value).toBe('one'); + expect(pageObject.getActiveOption()?.value).toBe('one'); await disconnect(); }); @@ -367,7 +367,7 @@ describe('Select', () => { await waitForUpdatesAsync(); expect(element.value).toBe('one'); - expect(pageObject.getSelectedOption()?.value).toBe('two'); + expect(pageObject.getActiveOption()?.value).toBe('two'); await disconnect(); }); @@ -680,12 +680,12 @@ describe('Select', () => { expect(element.value).toBe('one'); await pageObject.openAndSetFilterText('T'); // Matches 'Two' and 'Three' - currentSelection = pageObject.getSelectedOption(); + currentSelection = pageObject.getActiveOption(); expect(currentSelection?.text).toBe('Two'); pageObject.pressEscapeKey(); pageObject.clickSelect(); - currentSelection = pageObject.getSelectedOption(); + currentSelection = pageObject.getActiveOption(); expect(currentSelection?.text).toBe('One'); }); @@ -698,7 +698,7 @@ describe('Select', () => { pageObject.pressEnterKey(); pageObject.clickSelect(); - currentSelection = pageObject.getSelectedOption(); + currentSelection = pageObject.getActiveOption(); expect(currentSelection?.selected).toBeTrue(); }); @@ -759,7 +759,7 @@ describe('Select', () => { it('allows to be used as part of filter text', async () => { await pageObject.openAndSetFilterText(' '); // Matches 'Has Space' - const currentSelection = pageObject.getSelectedOption(); + const currentSelection = pageObject.getActiveOption(); expect(currentSelection?.text).toBe('Has Space'); expect(element.open).toBeTrue(); }); @@ -804,7 +804,7 @@ describe('Select', () => { currentFilteredOptions ); expect(element.open).toBeTrue(); - expect(pageObject.getSelectedOption()?.text).toBe('Two'); + expect(pageObject.getActiveOption()?.text).toBe('Two'); }); it('filtering to only disabled item, then pressing does not close popup or change value', async () => { @@ -853,7 +853,7 @@ describe('Select', () => { it('filtering to only disabled item does not select item', async () => { await pageObject.openAndSetFilterText('Disabled'); - expect(pageObject.getSelectedOption()).toBeNull(); + expect(pageObject.getActiveOption()).toBeNull(); }); it('updating slottedOptions while open applies filter to new options', async () => { @@ -897,11 +897,11 @@ describe('Select', () => { await pageObject.setOptions(newOptions); await pageObject.openAndSetFilterText('tw'); pageObject.pressArrowDownKey(); - let currentSelection = pageObject.getSelectedOption(); + let currentSelection = pageObject.getActiveOption(); expect(currentSelection?.value).toBe('twenty'); pageObject.pressArrowUpKey(); - currentSelection = pageObject.getSelectedOption(); + currentSelection = pageObject.getActiveOption(); expect(currentSelection?.value).toBe('two'); }); @@ -955,7 +955,7 @@ describe('Select', () => { element.value = 'three'; await pageObject.openAndSetFilterText('t'); // filters to 'Two' and 'Three' - expect(pageObject.getSelectedOption()?.value).toBe('three'); + expect(pageObject.getActiveOption()?.value).toBe('three'); }); }); @@ -1006,7 +1006,8 @@ describe('Select', () => { expect(pageObject.getDisplayText()).toBe('Two'); expect(element.value).toBe('two'); - await clickAndWaitForOpen(element); + pageObject.clickSelect(); + await waitForUpdatesAsync(); expect(pageObject.isOptionVisible(0)).toBeTrue(); expect(pageObject.isOptionVisible(1)).toBeFalse(); }); From b162f8b4ebb657ecd0df9dfff54ada59e341c245 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Thu, 25 Apr 2024 08:09:01 -0500 Subject: [PATCH 15/34] Code clean up. --- .../nimble-components/src/select/index.ts | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 5eede01849..f5a5d1b0d6 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -717,19 +717,18 @@ export class Select // don't call super.selectNextOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) const startIndex = this.openActiveIndex ?? this.selectedIndex; - const newActiveOptionIndex = (): number => { - for (let i = startIndex + 1; i < this.options.length; i++) { - const listOption = this.options[i]!; - if ( - isNimbleListOption(listOption) - && isOptionSelectable(listOption) - ) { - return i; - } + let newActiveOptionIndex = -1; + for (let i = startIndex + 1; i < this.options.length; i++) { + const listOption = this.options[i]!; + if ( + isNimbleListOption(listOption) + && isOptionSelectable(listOption) + ) { + newActiveOptionIndex = i; + break; } - return -1; - }; - this.toggleNewActiveOption(newActiveOptionIndex()); + } + this.toggleNewActiveOption(newActiveOptionIndex); } /** @@ -739,19 +738,18 @@ export class Select // don't call super.selectPreviousOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) const startIndex = this.openActiveIndex ?? this.selectedIndex; - const newActiveOptionIndex = (): number => { - for (let i = startIndex - 1; i >= 0; i--) { - const listOption = this.options[i]!; - if ( - isNimbleListOption(listOption) - && isOptionSelectable(listOption) - ) { - return i; - } + let newActiveOptionIndex = -1; + for (let i = startIndex - 1; i >= 0; i--) { + const listOption = this.options[i]!; + if ( + isNimbleListOption(listOption) + && isOptionSelectable(listOption) + ) { + newActiveOptionIndex = i; + break; } - return -1; - }; - this.toggleNewActiveOption(newActiveOptionIndex()); + } + this.toggleNewActiveOption(newActiveOptionIndex); } /** From d74b6ee6a87696e55386eddbae2b651a965dd020 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Thu, 25 Apr 2024 08:54:42 -0500 Subject: [PATCH 16/34] Reverting a change. --- packages/nimble-components/src/select/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index f5a5d1b0d6..81627c6058 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -797,7 +797,7 @@ export class Select if (this.open && this.selectedIndex === -1) { return; } - + this.toggleNewActiveOption(this.selectedIndex); super.setSelectedOptions(); } From 9f6c6d69dda193b96212c615c579e8c63b5745fa Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Thu, 25 Apr 2024 11:35:00 -0500 Subject: [PATCH 17/34] Fixes and tests. --- packages/nimble-components/src/select/index.ts | 11 +++++------ .../src/select/tests/select.spec.ts | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 81627c6058..10b1e13c25 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -797,7 +797,6 @@ export class Select if (this.open && this.selectedIndex === -1) { return; } - this.toggleNewActiveOption(this.selectedIndex); super.setSelectedOptions(); } @@ -941,10 +940,12 @@ export class Select if (this.open) { this.openActiveIndex = newActiveIndex; + this.focusAndScrollActiveOptionIntoView(); } else { this.selectedIndex = newActiveIndex; } - this.focusAndScrollActiveOptionIntoView(); + + this.ariaActiveDescendant = this.options[newActiveIndex]?.id ?? ''; } } @@ -955,10 +956,7 @@ export class Select // different frames. Since this function is typically called from the `openChanged` // observer, `DOM.queueUpdate` causes the calls to be grouped into the same frame. // To prevent this, `requestAnimationFrame` is used instead of `DOM.queueUpdate`. - if ( - this.contains(document.activeElement) - && optionToFocus !== undefined - ) { + if (optionToFocus !== undefined && this.contains(optionToFocus)) { optionToFocus.focus(); requestAnimationFrame(() => { optionToFocus.scrollIntoView({ block: 'nearest' }); @@ -1093,6 +1091,7 @@ export class Select this.indexWhenOpened = this.selectedIndex; this.openActiveIndex = this.selectedIndex; this.committedSelectedOption = this.options[this.selectedIndex]; + this.toggleNewActiveOption(this.openActiveIndex ?? this.selectedIndex); this.ariaControls = this.listboxId; this.ariaExpanded = 'true'; diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index ac01f89e02..65e72d24e2 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -372,6 +372,20 @@ describe('Select', () => { await disconnect(); }); + it('while navigating dropdown, ariaActiveDescendant is set to active option', async () => { + const { element, connect, disconnect } = await setup(); + const pageObject = new SelectPageObject(element); + await connect(); + await waitForUpdatesAsync(); + pageObject.clickSelect(); + pageObject.pressArrowDownKey(); + expect(element.ariaActiveDescendant).toBe( + pageObject.getActiveOption()!.id + ); + + await disconnect(); + }); + it('option added directly to DOM synchronously registers with Select', async () => { const { element, connect, disconnect } = await setup(); await connect(); From d9626e4bfc24ae2c5e1c5025fa4d9e9ec08abe69 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Thu, 25 Apr 2024 12:34:10 -0500 Subject: [PATCH 18/34] Fix tests. --- packages/nimble-components/src/select/index.ts | 4 +++- packages/nimble-components/src/select/tests/select.spec.ts | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 10b1e13c25..1fee1d70ca 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -206,7 +206,9 @@ export class Select public override connectedCallback(): void { super.connectedCallback(); this.forcedPosition = !!this.positionAttribute; - this.initializeOpenState(); + if (this.open) { + this.initializeOpenState(); + } } public override get value(): string { diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 65e72d24e2..8715d3da0f 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -1020,8 +1020,7 @@ describe('Select', () => { expect(pageObject.getDisplayText()).toBe('Two'); expect(element.value).toBe('two'); - pageObject.clickSelect(); - await waitForUpdatesAsync(); + await clickAndWaitForOpen(element); expect(pageObject.isOptionVisible(0)).toBeTrue(); expect(pageObject.isOptionVisible(1)).toBeFalse(); }); From f096b79f9d79073a4dbfe866ee9ef7787598a3b5 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Thu, 25 Apr 2024 13:39:30 -0500 Subject: [PATCH 19/34] Fix matrix tests. --- packages/nimble-components/src/select/index.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 1fee1d70ca..76954b1f4c 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -655,6 +655,9 @@ export class Select // implementation handles skipping non-selected disabled options for the initial // selected value. this.setSelectedOptions(); + if (this.open) { + this.toggleNewActiveOption(this.selectedIndex); + } this.updateValue(); } From 016026f57cc8585f11d060ea04862f9a67ec7641 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Thu, 25 Apr 2024 15:55:24 -0500 Subject: [PATCH 20/34] Fix styling for other dropdown controls. --- .../src/list-option/styles.ts | 5 ++--- .../nimble-components/src/select/styles.ts | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/nimble-components/src/list-option/styles.ts b/packages/nimble-components/src/list-option/styles.ts index 4199d82c59..34caeae040 100644 --- a/packages/nimble-components/src/list-option/styles.ts +++ b/packages/nimble-components/src/list-option/styles.ts @@ -22,13 +22,12 @@ export const styles = css` height: ${controlHeight}; } - :host([active-option]) { + :host([aria-selected='true']) { box-shadow: none; outline: none; background-color: ${fillSelectedColor}; } - - :host([active-option]:hover) { + :host([aria-selected='true']:hover) { background-color: ${fillHoverSelectedColor}; } diff --git a/packages/nimble-components/src/select/styles.ts b/packages/nimble-components/src/select/styles.ts index d543d15819..0e98e1df9c 100644 --- a/packages/nimble-components/src/select/styles.ts +++ b/packages/nimble-components/src/select/styles.ts @@ -6,6 +6,8 @@ import { borderRgbPartialColor, borderWidth, controlHeight, + fillHoverSelectedColor, + fillSelectedColor, mediumPadding, placeholderFontColor, smallPadding @@ -116,6 +118,22 @@ export const styles = css` overflow: auto; } + ::slotted([role='option'][active-option]) { + box-shadow: none; + outline: none; + background-color: ${fillSelectedColor}; + } + + ::slotted([role='option']:not([active-option])) { + box-shadow: inherit; + outline: inherit; + background-color: inherit; + } + + ::slotted([role='option'][active-option]:hover) { + background-color: ${fillHoverSelectedColor}; + } + .no-results-label { color: ${placeholderFontColor}; height: ${controlHeight}; From 3edd7f5dfcba58fdd788427285d1f84d6750aac8 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Fri, 26 Apr 2024 08:59:39 -0500 Subject: [PATCH 21/34] Update packages/nimble-components/src/select/testing/select.pageobject.ts Co-authored-by: mollykreis <20542556+mollykreis@users.noreply.github.com> --- .../nimble-components/src/select/testing/select.pageobject.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/nimble-components/src/select/testing/select.pageobject.ts b/packages/nimble-components/src/select/testing/select.pageobject.ts index 9006922d8f..fe1bbded1c 100644 --- a/packages/nimble-components/src/select/testing/select.pageobject.ts +++ b/packages/nimble-components/src/select/testing/select.pageobject.ts @@ -174,8 +174,8 @@ export class SelectPageObject { this.selectElement.open && this.selectElement.filterMode !== FilterMode.none ) { - const filterInput = this.getFilterInput(); - filterInput!.value = filterInput!.value + character; + const filterInput = this.getFilterInput()!; + filterInput.value = filterInput.value + character; } this.selectElement.dispatchEvent(new InputEvent('input')); this.selectElement.dispatchEvent( From 3a711449662d7c2e97c9ed8451ea09af8a3083cd Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Fri, 26 Apr 2024 13:39:10 -0500 Subject: [PATCH 22/34] Handle PR feedback. Fixes/tests. --- .../nimble-components/src/select/index.ts | 98 ++++++++----------- .../src/select/testing/select.pageobject.ts | 14 +-- .../src/select/tests/select.spec.ts | 50 ++++++++++ 3 files changed, 98 insertions(+), 64 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 76954b1f4c..518ce535ec 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -197,7 +197,6 @@ export class Select private _value = ''; private forcedPosition = false; - private indexWhenOpened?: number; private openActiveIndex?: number; /** @@ -342,11 +341,7 @@ export class Select super.clickHandler(e); this.open = this.collapsible && !this.open; - if ( - !this.open - && this.selectedIndex !== -1 - && this.indexWhenOpened !== this.selectedIndex - ) { + if (!this.open && this.selectedIndex !== -1) { this.updateValue(true); } } @@ -478,23 +473,21 @@ export class Select this.clearSelection(); this.filterOptions(); - if (this.filteredOptions.length > 0) { - const enabledOptions = this.filteredOptions.filter( - o => !o.disabled - ); - if (enabledOptions.length > 0) { - const selectedOption = enabledOptions.find( - o => o === this.committedSelectedOption - ) ?? enabledOptions[0]!; - (selectedOption as ListOption).activeOption = true; - this.openActiveIndex = this.options.indexOf(selectedOption); - } else { - // only filtered option is disabled - this.openActiveIndex = -1; - } - } else if (this.committedSelectedOption) { - this.committedSelectedOption.selected = true; + const enabledOptions = this.filteredOptions.filter(o => !o.disabled); + let selectedOptionIndex = this.filter !== '' + ? this.openActiveIndex ?? this.selectedIndex + : this.selectedIndex; + + if ( + enabledOptions.length > 0 + && (this.selectedIndex < 0 + || !enabledOptions.find(o => o === this.committedSelectedOption)) + ) { + selectedOptionIndex = this.options.indexOf(enabledOptions[0]!); + } else if (enabledOptions.length === 0) { + selectedOptionIndex = -1; } + this.setActiveOption(selectedOptionIndex); if (e.inputType.includes('deleteContent') || !this.filter.length) { return true; @@ -523,7 +516,6 @@ export class Select let currentActiveIndex = this.openActiveIndex ?? this.selectedIndex; this.open = false; if (currentActiveIndex === -1) { - this.selectedIndex = this.indexWhenOpened!; currentActiveIndex = this.selectedIndex; if (this.selectedIndex >= 0) { ( @@ -532,9 +524,9 @@ export class Select } } - if (this.indexWhenOpened !== currentActiveIndex) { - this.updateValue(true); + if (this.selectedIndex !== currentActiveIndex) { this.selectedIndex = currentActiveIndex; + this.updateValue(true); } } return true; @@ -544,6 +536,7 @@ export class Select * @internal */ public override keydownHandler(e: KeyboardEvent): BooleanOrVoid { + const initialSelectedIndex = this.selectedIndex; super.keydownHandler(e); const key = e.key; if (e.ctrlKey || e.shiftKey) { @@ -591,16 +584,14 @@ export class Select break; } - this.toggleNewActiveOption( - this.indexWhenOpened ?? this.selectedIndex - ); + this.setActiveOption(this.selectedIndex); if (this.collapsible && this.open) { e.preventDefault(); this.open = false; } - currentActiveIndex = this.indexWhenOpened!; + currentActiveIndex = this.selectedIndex; this.focus(); break; } @@ -610,10 +601,6 @@ export class Select this.filteredOptions.length === 0 || this.filteredOptions.every(o => o.disabled) ) { - this.toggleNewActiveOption( - this.indexWhenOpened ?? this.selectedIndex - ); - currentActiveIndex = this.indexWhenOpened!; this.open = false; return true; } @@ -628,11 +615,12 @@ export class Select } } - if (!this.open && this.indexWhenOpened !== currentActiveIndex) { + if (!this.open && this.selectedIndex !== currentActiveIndex) { this.selectedIndex = currentActiveIndex; - this.options[this.selectedIndex]!.ariaSelected = 'true'; + } + + if (!this.open && initialSelectedIndex !== this.selectedIndex) { this.updateValue(true); - this.indexWhenOpened = undefined; } return !(key === keyArrowDown || key === keyArrowUp); @@ -656,7 +644,7 @@ export class Select // selected value. this.setSelectedOptions(); if (this.open) { - this.toggleNewActiveOption(this.selectedIndex); + this.setActiveOption(this.selectedIndex); } this.updateValue(); } @@ -677,7 +665,7 @@ export class Select if (selectedIndex > -1 && !this.open) { this.selectedIndex = selectedIndex; } else { - this.toggleNewActiveOption(selectedIndex); + this.setActiveOption(selectedIndex); } } @@ -722,18 +710,16 @@ export class Select // don't call super.selectNextOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) const startIndex = this.openActiveIndex ?? this.selectedIndex; - let newActiveOptionIndex = -1; for (let i = startIndex + 1; i < this.options.length; i++) { const listOption = this.options[i]!; if ( isNimbleListOption(listOption) && isOptionSelectable(listOption) ) { - newActiveOptionIndex = i; + this.setActiveOption(i); break; } } - this.toggleNewActiveOption(newActiveOptionIndex); } /** @@ -743,18 +729,16 @@ export class Select // don't call super.selectPreviousOption as that relies on side-effecty // behavior to not select disabled option (which no longer works) const startIndex = this.openActiveIndex ?? this.selectedIndex; - let newActiveOptionIndex = -1; for (let i = startIndex - 1; i >= 0; i--) { const listOption = this.options[i]!; if ( isNimbleListOption(listOption) && isOptionSelectable(listOption) ) { - newActiveOptionIndex = i; + this.setActiveOption(i); break; } } - this.toggleNewActiveOption(newActiveOptionIndex); } /** @@ -764,7 +748,7 @@ export class Select const newActiveOptionIndex = this.options.findIndex( o => isNimbleListOption(o) && isOptionSelectable(o) ); - this.toggleNewActiveOption(newActiveOptionIndex); + this.setActiveOption(newActiveOptionIndex); } /** @@ -775,7 +759,7 @@ export class Select this.options, o => isNimbleListOption(o) && isOptionSelectable(o) ); - this.toggleNewActiveOption(newActiveOptionIndex); + this.setActiveOption(newActiveOptionIndex); } /** @@ -934,24 +918,23 @@ export class Select this.committedSelectedOption = options[this.selectedIndex]; } - private toggleNewActiveOption(newActiveIndex: number): void { + private setActiveOption(newActiveIndex: number): void { const selectedIndex = this.openActiveIndex ?? this.selectedIndex; const activeOption = this.options[newActiveIndex]; - if (newActiveIndex >= 0 && isNimbleListOption(activeOption)) { + if (isNimbleListOption(activeOption)) { activeOption.activeOption = true; if (selectedIndex !== newActiveIndex && selectedIndex > -1) { (this.options[selectedIndex]! as ListOption).activeOption = false; } + } - if (this.open) { - this.openActiveIndex = newActiveIndex; - this.focusAndScrollActiveOptionIntoView(); - } else { - this.selectedIndex = newActiveIndex; - } - - this.ariaActiveDescendant = this.options[newActiveIndex]?.id ?? ''; + if (this.open) { + this.openActiveIndex = newActiveIndex; + this.focusAndScrollActiveOptionIntoView(); + } else { + this.selectedIndex = newActiveIndex; } + this.ariaActiveDescendant = activeOption?.id ?? ''; } private focusAndScrollActiveOptionIntoView(): void { @@ -1093,10 +1076,9 @@ export class Select } private initializeOpenState(): void { - this.indexWhenOpened = this.selectedIndex; this.openActiveIndex = this.selectedIndex; this.committedSelectedOption = this.options[this.selectedIndex]; - this.toggleNewActiveOption(this.openActiveIndex ?? this.selectedIndex); + this.setActiveOption(this.openActiveIndex ?? this.selectedIndex); this.ariaControls = this.listboxId; this.ariaExpanded = 'true'; diff --git a/packages/nimble-components/src/select/testing/select.pageobject.ts b/packages/nimble-components/src/select/testing/select.pageobject.ts index 9006922d8f..a0f3672c35 100644 --- a/packages/nimble-components/src/select/testing/select.pageobject.ts +++ b/packages/nimble-components/src/select/testing/select.pageobject.ts @@ -174,10 +174,14 @@ export class SelectPageObject { this.selectElement.open && this.selectElement.filterMode !== FilterMode.none ) { - const filterInput = this.getFilterInput(); + const filterInput = this.selectElement.filterInput; filterInput!.value = filterInput!.value + character; } - this.selectElement.dispatchEvent(new InputEvent('input')); + const inputElement = this.selectElement.open + && this.selectElement.filterMode !== FilterMode.none + ? this.selectElement.filterInput + : this.selectElement; + inputElement!.dispatchEvent(new InputEvent('input')); this.selectElement.dispatchEvent( new KeyboardEvent('keydown', { key: character }) ); @@ -238,7 +242,7 @@ export class SelectPageObject { } public getFilterInputText(): string { - return this.getFilterInput()?.value ?? ''; + return this.selectElement.filterInput?.value ?? ''; } private getFilterInput(): HTMLInputElement | null | undefined { @@ -247,8 +251,6 @@ export class SelectPageObject { 'Select has filterMode of "none" so there is no filter input' ); } - return this.selectElement.shadowRoot?.querySelector( - '.filter-input' - ); + return this.selectElement.filterInput; } } diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 8715d3da0f..510e9f64db 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -413,6 +413,33 @@ describe('Select', () => { await disconnect(); }); + it('when handling change event, value of select element matches what was selected in dropdown on focusout', async () => { + const { element, connect, disconnect } = await setup(); + await connect(); + await waitForUpdatesAsync(); + const pageObject = new SelectPageObject(element); + await clickAndWaitForOpen(element); + let selectValue = ''; + + await Promise.race([ + new Promise(resolve => { + element.addEventListener('change', () => { + selectValue = element.value; + resolve(true); + }); + pageObject.pressArrowDownKey(); + void (async () => { + await pageObject.clickAway(); + })(); + }), + waitForUpdatesAsync().then(() => false) + ]); + + expect(selectValue).toBe('two'); + + await disconnect(); + }); + describe('with 500 options', () => { async function setup500Options(): Promise> { // prettier-ignore @@ -851,6 +878,11 @@ describe('Select', () => { expect(element.value).toBe('one'); }); + it('filtering to no available options sets ariaActiveDescendent to empty string', async () => { + await pageObject.openAndSetFilterText('abc'); + expect(element.ariaActiveDescendant).toBe(''); + }); + it('filtering to no available options, then pressing does not close popup or change value', async () => { await pageObject.openAndSetFilterText('abc'); pageObject.pressEnterKey(); @@ -971,6 +1003,24 @@ describe('Select', () => { expect(pageObject.getActiveOption()?.value).toBe('three'); }); + + it('dismissing dropdown with after navigation and then filtering to no options, does not update value', async () => { + await clickAndWaitForOpen(element); + pageObject.pressArrowDownKey(); + pageObject.pressCharacterKey('?'); + pageObject.pressTabKey(); + + expect(pageObject.getSelectedOption()?.value).toBe('one'); + }); + + it('dismissing dropdown by clicking away after navigation and then filtering to no options, does not update value', async () => { + await clickAndWaitForOpen(element); + pageObject.pressArrowDownKey(); + pageObject.pressCharacterKey('?'); + await pageObject.clickAway(); + + expect(pageObject.getSelectedOption()?.value).toBe('one'); + }); }); describe('placeholder', () => { From dc3329f37ee47f8e0b60550ba5bf75e97fe9f099 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Fri, 26 Apr 2024 14:21:05 -0500 Subject: [PATCH 23/34] Slight change with test. --- packages/nimble-components/src/select/index.ts | 6 ++++-- .../src/select/testing/select.pageobject.ts | 2 +- .../nimble-components/src/select/tests/select.spec.ts | 9 +++++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index 518ce535ec..bd0ace2d45 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -481,7 +481,9 @@ export class Select if ( enabledOptions.length > 0 && (this.selectedIndex < 0 - || !enabledOptions.find(o => o === this.committedSelectedOption)) + || !enabledOptions.find( + o => o === this.options[selectedOptionIndex] + )) ) { selectedOptionIndex = this.options.indexOf(enabledOptions[0]!); } else if (enabledOptions.length === 0) { @@ -664,7 +666,7 @@ export class Select ); if (selectedIndex > -1 && !this.open) { this.selectedIndex = selectedIndex; - } else { + } else if (this.filterMode === FilterMode.none) { this.setActiveOption(selectedIndex); } } diff --git a/packages/nimble-components/src/select/testing/select.pageobject.ts b/packages/nimble-components/src/select/testing/select.pageobject.ts index a37c7d35e8..d831fb17bd 100644 --- a/packages/nimble-components/src/select/testing/select.pageobject.ts +++ b/packages/nimble-components/src/select/testing/select.pageobject.ts @@ -175,7 +175,7 @@ export class SelectPageObject { && this.selectElement.filterMode !== FilterMode.none ) { const filterInput = this.selectElement.filterInput!; - filterInput.value = filterInput.value + character; + filterInput.value += character; } const inputElement = this.selectElement.open && this.selectElement.filterMode !== FilterMode.none diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 510e9f64db..c2bfd488e4 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -1004,6 +1004,15 @@ describe('Select', () => { expect(pageObject.getActiveOption()?.value).toBe('three'); }); + it('filtering options does not change selected option in dropdown after navigating with arrow keys', async () => { + await clickAndWaitForOpen(element); + pageObject.pressArrowDownKey(); + pageObject.pressArrowDownKey(); // option 'Three' should be active + pageObject.pressCharacterKey('t'); // filters to 'Two' and 'Three' + + expect(pageObject.getActiveOption()?.value).toBe('three'); + }); + it('dismissing dropdown with after navigation and then filtering to no options, does not update value', async () => { await clickAndWaitForOpen(element); pageObject.pressArrowDownKey(); From 98160ffbc27fa3be6a4116fae04e8a2e4acef264 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Fri, 26 Apr 2024 14:24:28 -0500 Subject: [PATCH 24/34] Revert change --- packages/nimble-components/src/list-option/styles.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nimble-components/src/list-option/styles.ts b/packages/nimble-components/src/list-option/styles.ts index 34caeae040..54d499448c 100644 --- a/packages/nimble-components/src/list-option/styles.ts +++ b/packages/nimble-components/src/list-option/styles.ts @@ -27,6 +27,7 @@ export const styles = css` outline: none; background-color: ${fillSelectedColor}; } + :host([aria-selected='true']:hover) { background-color: ${fillHoverSelectedColor}; } From 04f0916966b0152e0fcceef39c07e7aa81936988 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Fri, 26 Apr 2024 17:04:38 -0500 Subject: [PATCH 25/34] Style fix. --- packages/nimble-components/src/select/styles.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/nimble-components/src/select/styles.ts b/packages/nimble-components/src/select/styles.ts index 0e98e1df9c..ebfdfc0507 100644 --- a/packages/nimble-components/src/select/styles.ts +++ b/packages/nimble-components/src/select/styles.ts @@ -125,9 +125,9 @@ export const styles = css` } ::slotted([role='option']:not([active-option])) { - box-shadow: inherit; - outline: inherit; - background-color: inherit; + box-shadow: none; + outline: none; + background-color: none; } ::slotted([role='option'][active-option]:hover) { From a9dc531a6419c189a715bcbaccfdfbd8659aa9ce Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Fri, 26 Apr 2024 17:18:06 -0500 Subject: [PATCH 26/34] Simplify CSS. --- packages/nimble-components/src/select/styles.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/nimble-components/src/select/styles.ts b/packages/nimble-components/src/select/styles.ts index ebfdfc0507..c60feb7f7f 100644 --- a/packages/nimble-components/src/select/styles.ts +++ b/packages/nimble-components/src/select/styles.ts @@ -119,14 +119,10 @@ export const styles = css` } ::slotted([role='option'][active-option]) { - box-shadow: none; - outline: none; background-color: ${fillSelectedColor}; } ::slotted([role='option']:not([active-option])) { - box-shadow: none; - outline: none; background-color: none; } From 042d2b0adf7e5ea5d1ee26530c6dd055f523d9df Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 29 Apr 2024 08:20:52 -0500 Subject: [PATCH 27/34] Fix build. --- packages/nimble-components/src/select/tests/select.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index 2708a18af8..551739b89f 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -652,7 +652,6 @@ describe('Select', () => { }); it('after closing dropdown by committing a value with , activeElement is not Select element', () => { - element.filterMode = testData.filter; pageObject.clickSelect(); pageObject.pressArrowDownKey(); pageObject.pressTabKey(); From 7d4ba9a9a51e15e6fb8116282c525a7b25797dd5 Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 29 Apr 2024 17:31:14 -0500 Subject: [PATCH 28/34] Handle PR feedback. --- .../nimble-components/src/select/index.ts | 43 +++++-------------- .../nimble-components/src/select/styles.ts | 2 +- .../src/select/testing/select.pageobject.ts | 1 + .../src/select/tests/select-matrix.stories.ts | 27 +++++++++++- 4 files changed, 38 insertions(+), 35 deletions(-) diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index bd0ace2d45..380f083b8e 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -26,7 +26,6 @@ import { keyEscape, keyHome, keySpace, - keyTab, uniqueId } from '@microsoft/fast-web-utilities'; import { arrowExpanderDown16X16 } from '@ni/nimble-tokens/dist/icons/js'; @@ -474,22 +473,19 @@ export class Select this.filterOptions(); const enabledOptions = this.filteredOptions.filter(o => !o.disabled); - let selectedOptionIndex = this.filter !== '' + let activeOptionIndex = this.filter !== '' ? this.openActiveIndex ?? this.selectedIndex : this.selectedIndex; if ( enabledOptions.length > 0 - && (this.selectedIndex < 0 - || !enabledOptions.find( - o => o === this.options[selectedOptionIndex] - )) + && !enabledOptions.find(o => o === this.options[activeOptionIndex]) ) { - selectedOptionIndex = this.options.indexOf(enabledOptions[0]!); + activeOptionIndex = this.options.indexOf(enabledOptions[0]!); } else if (enabledOptions.length === 0) { - selectedOptionIndex = -1; + activeOptionIndex = -1; } - this.setActiveOption(selectedOptionIndex); + this.setActiveOption(activeOptionIndex); if (e.inputType.includes('deleteContent') || !this.filter.length) { return true; @@ -519,11 +515,6 @@ export class Select this.open = false; if (currentActiveIndex === -1) { currentActiveIndex = this.selectedIndex; - if (this.selectedIndex >= 0) { - ( - this.options[this.selectedIndex]! as ListOption - ).activeOption = true; - } } if (this.selectedIndex !== currentActiveIndex) { @@ -597,20 +588,6 @@ export class Select this.focus(); break; } - case keyTab: { - if (this.collapsible && this.open) { - if ( - this.filteredOptions.length === 0 - || this.filteredOptions.every(o => o.disabled) - ) { - this.open = false; - return true; - } - } - - this.open = false; - break; - } default: { break; @@ -661,13 +638,13 @@ export class Select const typeaheadMatches = this.getTypeaheadMatches(); if (typeaheadMatches.length) { - const selectedIndex = this.options.indexOf( + const activeOption = this.options.indexOf( typeaheadMatches[0] as ListOption ); - if (selectedIndex > -1 && !this.open) { - this.selectedIndex = selectedIndex; + if (activeOption > -1 && !this.open) { + this.selectedIndex = activeOption; } else if (this.filterMode === FilterMode.none) { - this.setActiveOption(selectedIndex); + this.setActiveOption(activeOption); } } @@ -1080,7 +1057,7 @@ export class Select private initializeOpenState(): void { this.openActiveIndex = this.selectedIndex; this.committedSelectedOption = this.options[this.selectedIndex]; - this.setActiveOption(this.openActiveIndex ?? this.selectedIndex); + this.setActiveOption(this.selectedIndex); this.ariaControls = this.listboxId; this.ariaExpanded = 'true'; diff --git a/packages/nimble-components/src/select/styles.ts b/packages/nimble-components/src/select/styles.ts index c60feb7f7f..dbe7b2b798 100644 --- a/packages/nimble-components/src/select/styles.ts +++ b/packages/nimble-components/src/select/styles.ts @@ -123,7 +123,7 @@ export const styles = css` } ::slotted([role='option']:not([active-option])) { - background-color: none; + background: none; } ::slotted([role='option'][active-option]:hover) { diff --git a/packages/nimble-components/src/select/testing/select.pageobject.ts b/packages/nimble-components/src/select/testing/select.pageobject.ts index d831fb17bd..924545cef6 100644 --- a/packages/nimble-components/src/select/testing/select.pageobject.ts +++ b/packages/nimble-components/src/select/testing/select.pageobject.ts @@ -149,6 +149,7 @@ export class SelectPageObject { this.selectElement.dispatchEvent( new KeyboardEvent('keydown', { key: keyTab }) ); + this.selectElement.dispatchEvent(new FocusEvent('focusout')); } public pressArrowDownKey(): void { diff --git a/packages/nimble-components/src/select/tests/select-matrix.stories.ts b/packages/nimble-components/src/select/tests/select-matrix.stories.ts index 7e88584772..276a0aadd7 100644 --- a/packages/nimble-components/src/select/tests/select-matrix.stories.ts +++ b/packages/nimble-components/src/select/tests/select-matrix.stories.ts @@ -1,5 +1,6 @@ import type { StoryFn, Meta } from '@storybook/html'; import { html, ViewTemplate } from '@microsoft/fast-element'; +import { keyArrowDown } from '@microsoft/fast-web-utilities'; import { createStory } from '../../utilities/tests/storybook'; import { createMatrixThemeStory, @@ -21,9 +22,10 @@ import { menuMinWidth, standardPadding } from '../../theme-provider/design-tokens'; -import { selectTag } from '..'; +import { Select, selectTag } from '..'; import { listOptionTag } from '../../list-option'; import { loremIpsum } from '../../utilities/tests/lorem-ipsum'; +import { waitForUpdatesAsync } from '../../testing/async-helpers'; const appearanceStates = [ ['Underline', DropdownAppearance.underline], @@ -101,6 +103,29 @@ export const blankListOption: StoryFn = createStory( ` ); +const playFunction = async (): Promise => { + await Promise.all( + Array.from(document.querySelectorAll