spectrum-web-components icon indicating copy to clipboard operation
spectrum-web-components copied to clipboard

[Bug]: Combobox throws errors when options with numeric values are provided

Open Rocss opened this issue 1 year ago • 3 comments
trafficstars

Code of conduct

  • [X] I agree to follow this project's code of conduct.

Impacted component(s)

sp-combobox

Expected behavior

Given that I have a list of options with numeric values, when I navigate via keyboard through the options, the list should scroll and no errors should be thrown.

Actual behavior

An error is being thrown in the console and the list does not scroll.

Screenshots

https://github.com/adobe/spectrum-web-components/assets/13311865/770c2ef4-95f6-4a9a-933d-2008ba388719

What browsers are you seeing the problem in?

Chrome

How can we reproduce this issue?

  1. Go here: https://opensource.adobe.com/spectrum-web-components/storybook/index.html?path=/story/combobox--controlled
  2. Navigate the options list using the keyboard.
  3. Check console.
  4. Observe the error thrown in the console.

Sample code that illustrates the problem

The Combobox code does

        const activeEl = this.shadowRoot.querySelector(
            `#${this.activeDescendant?.value}`
        ) as HTMLElement;

but for instances where value starts with a digit querySelector on IDs that start with a digit is not a valid selector.

Maybe querying by the value attribute or querying by [id="${this.activeDescendant?.value}"] would work better here?

Logs taken while reproducing problem

Uncaught DOMException: Failed to execute 'querySelector' on 'DocumentFragment': '#55' is not a valid selector. is being thrown

Rocss avatar May 02 '24 10:05 Rocss

In theory, if we change:

 const activeEl = this.shadowRoot.querySelector(
            `#${this.activeDescendant?.value}`
        ) as HTMLElement;

to

 const activeEl = this.shadowRoot.getElementById(
            this.activeDescendant?.value
        ) as HTMLElement;

This would no longer throw and no practical changes would be needed in the template?

Westbrook avatar May 02 '24 12:05 Westbrook

There's the edge case where it tries to get an activeDescendant that doesn't exist, but I'm not sure how practical actually hitting the code path is...

Westbrook avatar May 02 '24 12:05 Westbrook

getElementById works too (even with whitespaces, which is another edge).

Another thing I noticed here is that when I select an option using the keyboard, the change event is not triggered.

Rocss avatar May 02 '24 12:05 Rocss