From 351d322219d84f941aa1270cffb89bb92a788e7f Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Mon, 11 Mar 2024 11:36:42 -0500 Subject: [PATCH] Add registerOption implementation to Combobox. --- .../nimble-components/src/combobox/index.ts | 25 ++++++++++++++-- .../src/combobox/tests/combobox.spec.ts | 29 ++++++++++++++++++- .../nimble-components/src/select/index.ts | 6 ++++ .../src/select/tests/select.spec.ts | 5 ++++ 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/packages/nimble-components/src/combobox/index.ts b/packages/nimble-components/src/combobox/index.ts index e3da226ded..4f607c5439 100644 --- a/packages/nimble-components/src/combobox/index.ts +++ b/packages/nimble-components/src/combobox/index.ts @@ -17,10 +17,14 @@ import { iconExclamationMarkTag } from '../icons/exclamation-mark'; import { styles } from './styles'; import type { ErrorPattern } from '../patterns/error/types'; -import type { DropdownPattern } from '../patterns/dropdown/types'; +import type { + DropdownPattern, + ListOptionOwner +} from '../patterns/dropdown/types'; import { DropdownAppearance } from '../patterns/dropdown/types'; import type { AnchoredRegion } from '../anchored-region'; import { template } from './template'; +import type { ListOption } from '../list-option'; declare global { interface HTMLElementTagNameMap { @@ -33,7 +37,7 @@ declare global { */ export class Combobox extends FoundationCombobox - implements DropdownPattern, ErrorPattern { + implements DropdownPattern, ErrorPattern, ListOptionOwner { @attr public appearance: DropdownAppearance = DropdownAppearance.underline; @@ -207,6 +211,23 @@ export class Combobox return returnValue; } + /** + * @internal + */ + public registerOption(option: ListOption): void { + if (this.options.includes(option)) { + return; + } + + // Adding an option to the end, ultimately, isn't the correct + // thing to do, as this will mean the option's index in the options, + // at least temporarily, does not match the DOM order. However, it + // is expected that a successive run of `slottedOptionsChanged` will + // correct this order issue. See 'https://github.com/ni/nimble/issues/1915' + // for more info. + this.options.push(option); + } + protected override focusAndScrollOptionIntoView(): void { if (this.open) { super.focusAndScrollOptionIntoView(); diff --git a/packages/nimble-components/src/combobox/tests/combobox.spec.ts b/packages/nimble-components/src/combobox/tests/combobox.spec.ts index 242d147c33..7c95de6980 100644 --- a/packages/nimble-components/src/combobox/tests/combobox.spec.ts +++ b/packages/nimble-components/src/combobox/tests/combobox.spec.ts @@ -2,7 +2,7 @@ import { html, repeat } from '@microsoft/fast-element'; import { keyArrowDown, keyEnter } from '@microsoft/fast-web-utilities'; import { fixture, Fixture } from '../../utilities/tests/fixture'; import { Combobox, comboboxTag } from '..'; -import { listOptionTag } from '../../list-option'; +import { ListOption, listOptionTag } from '../../list-option'; import { ComboboxAutocomplete } from '../types'; import { waitForUpdatesAsync } from '../../testing/async-helpers'; import { @@ -127,6 +127,33 @@ describe('Combobox', () => { await disconnect(); }); + it('option added directly to DOM synchronously registers with Combobox', async () => { + const { element, connect, disconnect } = await setup(); + await connect(); + element.selectedIndex = 0; + await waitForUpdatesAsync(); + const newOption = new ListOption('foo', 'foo'); + const registerOptionSpy = spyOn( + element, + 'registerOption' + ).and.callThrough(); + registerOptionSpy.calls.reset(); + element.insertBefore(newOption, element.options[0]!); + + expect(registerOptionSpy.calls.count()).toBe(1); + expect(element.options).toContain(newOption); + // The below assertion is simply showing a current expected, but + // incorrect, behavior, as the new option was added before the currently + // selected one. See 'https://github.com/ni/nimble/issues/1915' + // for details. + expect(element.selectedIndex).toBe(0); + await waitForUpdatesAsync(); + // This assertion shows that after 'slottedOptionsChanged' runs, the + // 'selectedIndex' state has been corrected to expected DOM order. + expect(element.selectedIndex).toBe(1); + await disconnect(); + }); + const ariaTestData: { attrName: string, propSetter: (x: Combobox, value: string) => void diff --git a/packages/nimble-components/src/select/index.ts b/packages/nimble-components/src/select/index.ts index de8c856c75..5cd9a29f4b 100644 --- a/packages/nimble-components/src/select/index.ts +++ b/packages/nimble-components/src/select/index.ts @@ -681,6 +681,12 @@ export class Select return; } + // Adding an option to the end, ultimately, isn't the correct + // thing to do, as this will mean the option's index in the options, + // at least temporarily, does not match the DOM order. However, it + // is expected that a successive run of `slottedOptionsChanged` will + // correct this order issue. See 'https://github.com/ni/nimble/issues/1915' + // for more info. this.options.push(option); } diff --git a/packages/nimble-components/src/select/tests/select.spec.ts b/packages/nimble-components/src/select/tests/select.spec.ts index e1a1b8161e..81bc5a4fe1 100644 --- a/packages/nimble-components/src/select/tests/select.spec.ts +++ b/packages/nimble-components/src/select/tests/select.spec.ts @@ -295,9 +295,14 @@ describe('Select', () => { expect(registerOptionSpy.calls.count()).toBe(1); expect(element.options).toContain(newOption); + // The below assertion is simply showing a current expected, but + // incorrect, behavior. See 'https://github.com/ni/nimble/issues/1915' + // for details. expect(element.selectedIndex).toBe(0); await waitForUpdatesAsync(); expect(element.value).toBe('one'); + // This assertion shows that after 'slottedOptionsChanged' runs, the + // 'selectedIndex' state has been corrected to expected DOM order. expect(element.selectedIndex).toBe(1); await disconnect(); });