Automatically set an ID on active combobox options#99
Automatically set an ID on active combobox options#99henrycatalinismith wants to merge 1 commit intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the ARIA spec pattern of dynamically applying a stable id to the currently active combobox option so aria-activedescendant always points to an element with an id, improving resilience when option markup omits ids (Issue #96).
Changes:
- Auto-assign
${list.id}-selectedas the active option’sidwhen the option lacks one. - Remove the
${list.id}-selectedidfrom non-active options as selection moves.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| target.scrollIntoView(this.scrollIntoViewOptions) | ||
| } else { | ||
| if (el.id === `${this.list.id}-selected`) { | ||
| el.removeAttribute('id'); |
There was a problem hiding this comment.
el.removeAttribute('id'); introduces a stray semicolon; the rest of this file uses a no-semicolons style. Please drop the semicolon for consistency.
| el.removeAttribute('id'); | |
| el.removeAttribute('id') |
| if (!target.id) { | ||
| target.id = `${this.list.id}-selected` | ||
| } | ||
| this.input.setAttribute('aria-activedescendant', target.id) |
There was a problem hiding this comment.
This change introduces new behavior for options without ids (auto-assigning and moving ${list.id}-selected). There are existing tests for aria-activedescendant behavior in test/test.js, but none cover the missing-id path; please add tests to validate id assignment, id movement across options, and cleanup when selection is cleared/stopped.
| } | ||
| this.input.setAttribute('aria-activedescendant', target.id) | ||
| target.setAttribute('aria-selected', 'true') | ||
| fireSelectEvent(target) | ||
| target.scrollIntoView(this.scrollIntoViewOptions) | ||
| } else { | ||
| if (el.id === `${this.list.id}-selected`) { | ||
| el.removeAttribute('id'); |
There was a problem hiding this comment.
The cleanup logic removes the id from any non-target option whose id matches ${this.list.id}-selected. This can inadvertently delete a user-supplied id if their markup already uses that value. Consider marking ids that were auto-assigned (e.g., via a data-* flag or instance state) and only removing the id when it was generated by this library.
| } | |
| this.input.setAttribute('aria-activedescendant', target.id) | |
| target.setAttribute('aria-selected', 'true') | |
| fireSelectEvent(target) | |
| target.scrollIntoView(this.scrollIntoViewOptions) | |
| } else { | |
| if (el.id === `${this.list.id}-selected`) { | |
| el.removeAttribute('id'); | |
| target.setAttribute('data-combobox-generated-id', 'true') | |
| } | |
| this.input.setAttribute('aria-activedescendant', target.id) | |
| target.setAttribute('aria-selected', 'true') | |
| fireSelectEvent(target) | |
| target.scrollIntoView(this.scrollIntoViewOptions) | |
| } else { | |
| if ( | |
| el.id === `${this.list.id}-selected` && | |
| el.getAttribute('data-combobox-generated-id') === 'true' | |
| ) { | |
| el.removeAttribute('id') | |
| el.removeAttribute('data-combobox-generated-id') |
| if (!target.id) { | ||
| target.id = `${this.list.id}-selected` | ||
| } | ||
| this.input.setAttribute('aria-activedescendant', target.id) | ||
| target.setAttribute('aria-selected', 'true') |
There was a problem hiding this comment.
When selection is cleared via clearSelection() / stop() / destroy(), the auto-assigned ${this.list.id}-selected id is not removed from the last selected option, leaving a stale "selected" id in the DOM. To keep the ARIA example pattern consistent (id only on the active option) and avoid confusing consumers that query for that id, consider removing the generated id as part of clearSelection() (and only if it was generated by this library).
| } else { | ||
| if (el.id === `${this.list.id}-selected`) { | ||
| el.removeAttribute('id'); | ||
| } | ||
| el.removeAttribute('aria-selected') |
There was a problem hiding this comment.
In this for loop, it's possible to remove the old ${this.list.id}-selected id (in the else branch) before reaching the new target and updating aria-activedescendant, creating a transient state where aria-activedescendant references a non-existent id. Consider setting the new aria-activedescendant / id first, then clearing the old generated id (e.g., clear-after or two-pass).
|
I did consider handling the kinds of things that the review bot has flagged in the first draft of this, but opted to err on the side of keeping the initial diff small so that it's more approachable for a human reviewer. More than happy to work through the automated feedback if and when we agree on the general approach! |
This is my third and final idea for a solution to #96. It also happens to be my favourite, because it's the approach in https://www.w3.org/TR/wai-aria-1.2/#example-6. In the screen recording below, we can see the ID
list-id-selectedbeing automatically applied to each option in turn as it's selected.selected-id.mp4
It has the same upsides as #97 without the downside of the increased risk of collisions. And since it's a direct copy of the approach in one of the examples in the ARIA spec itself I'm less concerned about the possibility of an issue with JAWS than I was with #97.