Add console warning about missing IDs on options#98
Add console warning about missing IDs on options#98henrycatalinismith wants to merge 1 commit intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a conservative runtime diagnostic for invalid combobox markup (options missing IDs) to help downstream consumers catch accessibility issues early, plus README troubleshooting guidance.
Changes:
- Add a constructor-time check that logs a
console.warnwhen any[role="option"]elements lack anid. - Document the warning and its rationale in a new README “Troubleshooting” section.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/index.ts | Adds a DOM check for missing option IDs and emits a console warning with a docs link. |
| README.md | Adds troubleshooting docs explaining the warning and how to resolve it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| list.id = `combobox-${Math.random().toString().slice(2, 6)}` | ||
| } | ||
|
|
||
| if (list.querySelectorAll('[role="option"]:not([id])').length > 0) { |
There was a problem hiding this comment.
Since you're only checking for existence, querySelector('[role="option"]:not([id])') avoids allocating a full NodeList just to read .length and is simpler.
| if (list.querySelectorAll('[role="option"]:not([id])').length > 0) { | |
| if (list.querySelector('[role="option"]:not([id])') !== null) { |
| if (list.querySelectorAll('[role="option"]:not([id])').length > 0) { | ||
| console.warn([ | ||
| 'Combobox options must have unique IDs', | ||
| 'See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids', |
There was a problem hiding this comment.
The docs URL includes the GitHub UI query param ?tab=readme-ov-file, which is not a stable permalink and may break over time. Prefer a stable README anchor URL (e.g., the repo root #... link or a blob/<branch>/README.md#... link).
| 'See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids', | |
| 'See https://github.com/github/combobox-nav#combobox-options-must-have-unique-ids', |
| if (list.querySelectorAll('[role="option"]:not([id])').length > 0) { | ||
| console.warn([ | ||
| 'Combobox options must have unique IDs', | ||
| 'See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids', | ||
| ].join('\n')) | ||
| } |
There was a problem hiding this comment.
This PR introduces a new runtime warning; please add a test that stubs console.warn and asserts it fires when an option is missing an id (and does not fire when all options have valid IDs). This will prevent regressions as the selection logic evolves.
|
|
||
| Without the IDs we lack a unique identifier for [`aria-activedescendant`](https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#kbd_focus_activedescendant) to reference. As a result, screen readers are unable to announce which option is currently selected. | ||
|
|
||
| You can remove this warning by setting IDs on every option in the combobox. |
There was a problem hiding this comment.
This troubleshooting section title/message says options must have "unique IDs", but the described trigger is "options lack IDs". Align the documentation with the actual runtime check (missing id vs missing/empty/non-unique ids), otherwise readers may look for duplicate IDs when the warning is really about absent/invalid IDs.
| You can remove this warning by setting IDs on every option in the combobox. | |
| You can remove this warning by ensuring every option in the combobox has a non-empty `id` attribute (the warning is triggered when options are missing IDs, not when IDs are duplicated). |
| Passing a combobox whose options lack IDs results in the following console warning. | ||
|
|
||
| > Combobox options must have unique IDs | ||
| > See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids |
There was a problem hiding this comment.
The URL in the documented warning includes ?tab=readme-ov-file, which is a GitHub UI-specific query param and not a stable permalink. Consider switching to a stable README anchor link so the troubleshooting reference doesn't rot.
| > See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids | |
| > See https://github.com/github/combobox-nav#combobox-options-must-have-unique-ids |
| if (list.querySelectorAll('[role="option"]:not([id])').length > 0) { | ||
| console.warn([ | ||
| 'Combobox options must have unique IDs', | ||
| 'See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids', | ||
| ].join('\n')) | ||
| } | ||
|
|
There was a problem hiding this comment.
The warning text says options must have "unique IDs", but the check only detects options missing an id attribute (and it won't catch empty id=""). Either update the detection to validate non-empty + uniqueness, or adjust the warning text to match what is actually being checked.
| if (list.querySelectorAll('[role="option"]:not([id])').length > 0) { | |
| console.warn([ | |
| 'Combobox options must have unique IDs', | |
| 'See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids', | |
| ].join('\n')) | |
| } | |
| const optionElements = Array.from(list.querySelectorAll<HTMLElement>('[role="option"]')) | |
| const seenOptionIds = new Set<string>() | |
| let hasOptionIdIssue = false | |
| for (const option of optionElements) { | |
| const id = option.getAttribute('id') | |
| if (!id) { | |
| hasOptionIdIssue = true | |
| break | |
| } | |
| if (seenOptionIds.has(id)) { | |
| hasOptionIdIssue = true | |
| break | |
| } | |
| seenOptionIds.add(id) | |
| } | |
| if (hasOptionIdIssue) { | |
| console.warn( | |
| [ | |
| 'Combobox options must have unique, non-empty IDs', | |
| 'See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids', | |
| ].join('\n'), | |
| ) | |
| } |
| if (list.querySelectorAll('[role="option"]:not([id])').length > 0) { | ||
| console.warn([ | ||
| 'Combobox options must have unique IDs', | ||
| 'See https://github.com/github/combobox-nav/?tab=readme-ov-file#combobox-options-must-have-unique-ids', | ||
| ].join('\n')) | ||
| } |
There was a problem hiding this comment.
This will emit a console warning once per Combobox instance, which can get very noisy on pages that create multiple instances (and can flood error tracking if console output is captured). Consider deduplicating (e.g., a module-level flag / Set) so the warning is logged at most once per page/session (or once per list element).
This is another alternative approach to #96. Instead of attempting a fix, we detect the issue in the constructor and print a console warning with a brief message and a URL linking to further documentation.
The big upside with this approach is its conservative nature. Backwards compatibility is maximised by avoiding any material changes to the behaviour of the component.
One downside is the risk of downstream maintainers ignoring the warning forever and never resolving their accessibility issue. Another is that on high-traffic sites with an error tracking service Sentry, the large volume of sudden new warnings might exhaust some people's quotas, which could be a bit of an unwelcome headache.