From 83f7fd9e601eed789e8f6eb61a9d4a87a1d1835b Mon Sep 17 00:00:00 2001 From: Jonathan Meyer <26874831+atmgrifter00@users.noreply.github.com> Date: Wed, 20 Mar 2024 11:26:21 -0500 Subject: [PATCH] Combobox registerOption reversion to address Angular bug (#1949) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request ## ๐Ÿคจ Rationale [AzDO bug 'Nimble combo box | Issue in showing list options'](https://ni.visualstudio.com/DevCentral/_workitems/edit/2687002) ## ๐Ÿ‘ฉโ€๐Ÿ’ป Implementation Simple reversion of problem code. I've also created a [tech debt item](https://github.com/ni/nimble/issues/1948) to track the work needed to ultimately re-align the `Combobox` and `Select` implementations. ## ๐Ÿงช Testing Just removing the related test. ## โœ… Checklist - [ ] I have updated the project documentation to reflect my changes or determined no changes are needed. --- ...-54ec9080-33bb-47a6-a37d-d926c42de859.json | 7 +++++ .../nimble-components/src/combobox/index.ts | 25 ++-------------- .../src/combobox/tests/combobox.spec.ts | 29 +------------------ 3 files changed, 10 insertions(+), 51 deletions(-) create mode 100644 change/@ni-nimble-components-54ec9080-33bb-47a6-a37d-d926c42de859.json diff --git a/change/@ni-nimble-components-54ec9080-33bb-47a6-a37d-d926c42de859.json b/change/@ni-nimble-components-54ec9080-33bb-47a6-a37d-d926c42de859.json new file mode 100644 index 0000000000..897eb39604 --- /dev/null +++ b/change/@ni-nimble-components-54ec9080-33bb-47a6-a37d-d926c42de859.json @@ -0,0 +1,7 @@ +{ + "type": "patch", + "comment": "Remove ListOptionOwner from Combobox to address issue found in Angular", + "packageName": "@ni/nimble-components", + "email": "26874831+atmgrifter00@users.noreply.github.com", + "dependentChangeType": "patch" +} diff --git a/packages/nimble-components/src/combobox/index.ts b/packages/nimble-components/src/combobox/index.ts index 4f607c5439..e3da226ded 100644 --- a/packages/nimble-components/src/combobox/index.ts +++ b/packages/nimble-components/src/combobox/index.ts @@ -17,14 +17,10 @@ import { iconExclamationMarkTag } from '../icons/exclamation-mark'; import { styles } from './styles'; import type { ErrorPattern } from '../patterns/error/types'; -import type { - DropdownPattern, - ListOptionOwner -} from '../patterns/dropdown/types'; +import type { DropdownPattern } 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 { @@ -37,7 +33,7 @@ declare global { */ export class Combobox extends FoundationCombobox - implements DropdownPattern, ErrorPattern, ListOptionOwner { + implements DropdownPattern, ErrorPattern { @attr public appearance: DropdownAppearance = DropdownAppearance.underline; @@ -211,23 +207,6 @@ 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 7b282bd515..15973059fc 100644 --- a/packages/nimble-components/src/combobox/tests/combobox.spec.ts +++ b/packages/nimble-components/src/combobox/tests/combobox.spec.ts @@ -9,7 +9,7 @@ import { waitAnimationFrame } from '../../utilities/tests/component'; import { checkFullyInViewport } from '../../utilities/tests/intersection-observer'; -import { ListOption, listOptionTag } from '../../list-option'; +import { listOptionTag } from '../../list-option'; async function setup( position?: string, @@ -127,33 +127,6 @@ 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); - - // While the option is registered synchronously as shown above, - // properties like selectedIndex will only be correct asynchronously - // See https://github.com/ni/nimble/issues/1915 - 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