fix(a11y): correct dropdown combobox/listbox structure (D3) - #15
Merged
Conversation
Use the APG select-only combobox pattern: combobox toggle with aria-haspopup=listbox and aria-controls, presentational list wrapper so options are owned by the listbox, and non-button option hosts. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe dropdown toggle now uses select-only combobox semantics and links to a unique listbox. Menu items use focusable Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/dropdown/dropdown.js`:
- Around line 121-125: Update the dropdown focus management around the option
elements created in the dropdown initialization so focusout closes the popup
when focus moves outside both this.toggle and this.menu, including Tab
navigation to page controls; preserve it when focus remains within the dropdown.
Register the handler consistently and remove the same listener in destroy(),
then add coverage for Tab and the intended Shift+Tab behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2e8a815-aaec-42d4-93d3-18274a3ae8e4
📒 Files selected for processing (3)
components/dropdown/README.mdcomponents/dropdown/dropdown.csscomponents/dropdown/dropdown.js
Listen for focusout on the toggle and menu so Tab to page controls dismisses the open listbox, while Shift+Tab from an option back to the toggle keeps it open. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
Addressed the Tab focusout finding in 5b467d9:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Changed files in the reported commit ==='
git show --stat --oneline --decorate --no-renames 5b467d9
printf '%s\n' '=== Changed diff ==='
git show --format=fuller --find-renames --find-copies --no-ext-diff 5b467d9 -- \
components/dropdown/dropdown.js \
components/dropdown/README.md \
components/dropdown/dropdown.css
printf '%s\n' '=== Dropdown-related files ==='
fd . components/dropdown -t f 2>/dev/null | sort
printf '%s\n' '=== Test files mentioning dropdown or focus behavior ==='
rg -n -i -C 3 '(dropdown|focusout|Shift\+Tab|ArrowDown|listbox|combobox)' \
-g '*.{js,ts,mjs,cjs}' \
-g '!node_modules' \
. | head -n 320Length of output: 28231 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Pull request metadata and changed files ==='
gh pr view 15 --repo CodeSignal/learn_bespoke-design-system \
--json baseRefName,headRefName,commits,files \
--jq '{baseRefName, headRefName, commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline}], files: [.files[] | {path, additions, deletions}]}'
printf '%s\n' '=== Worktree status ==='
git status --short
printf '%s\n' '=== Dropdown source map ==='
ast-grep outline components/dropdown/dropdown.js --items all --view expanded
printf '%s\n' '=== Focus, close, portal, and destroy implementation ==='
nl -ba components/dropdown/dropdown.js | sed -n '1,125p;180,375p;620,655p'
printf '%s\n' '=== Playwright configuration and test inventory ==='
fd -HI -t f \
-e js -e ts -e mjs -e cjs \
-g '*playwright*' -g '*spec*' -g '*test*' \
. | sort | head -n 300
printf '%s\n' '=== Test definitions for dropdown focus behavior ==='
rg -n -i -C 6 \
'(test|it)\s*\(.*(dropdown|Tab|Shift\+Tab|focusout)|dropdown.*(Tab|Shift\+Tab|focusout)|page\.keyboard\.press|keyboard\.press' \
-g '*.{js,ts,mjs,cjs}' \
-g '!node_modules' \
. | head -n 400Length of output: 3038 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Dropdown focus and lifecycle implementation ==='
sed -n '1,125p;180,375p;620,655p' components/dropdown/dropdown.js
printf '%s\n' '=== All candidate test and Playwright files ==='
find . \
-path './.git' -prune -o \
-path './node_modules' -prune -o \
-type f \( \
-iname '*playwright*' -o \
-iname '*spec.js' -o -iname '*spec.ts' -o \
-iname '*test.js' -o -iname '*test.ts' \
\) -print | sort | head -n 300
printf '%s\n' '=== Repository references to Playwright and dropdown keyboard checks ==='
rg -n -i -C 5 \
'(playwright|`@playwright/test`|page\.keyboard\.press|keyboard\.press|Shift\+Tab|focusout)' \
-g '*.{js,ts,mjs,cjs,json,yml,yaml}' \
-g '!node_modules' \
. | head -n 400
printf '%s\n' '=== Full pull request patch summary ==='
git diff --check main...HEAD
git diff --stat main...HEADLength of output: 17101
I found one remaining issue: PR
|
Track Tab / Shift+Tab open-state regressions in tests/dropdown-focus.spec.js and run them on PRs via a Playwright workflow. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
Added a tracked Playwright suite for the Tab / Shift+Tab regression checks:
Local: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr.yml:
- Around line 10-11: Update the actions/checkout step to set persist-credentials
to false, preventing the checkout token from being stored in local Git
configuration while preserving the existing repository checkout behavior.
In `@tests/dropdown-focus.spec.js`:
- Around line 45-122: Add light-mode and dark-mode coverage to the dropdown
focus test suite, preserving the existing focus-transition assertions in the
tests under dropdown focus. Add axe accessibility validation for the dropdown in
each relevant theme, using the repository’s established theme setup and
accessibility-checking utilities.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd02eb81-453f-4664-ab21-95f5a7fdfe06
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/pr.yml.gitignorecomponents/dropdown/README.mdpackage.jsonplaywright.config.mjstests/dropdown-focus.spec.js
Disable persisted checkout credentials in PR CI, run Tab/Shift+Tab cases under both color schemes, and add a scoped axe helper. Name the combobox via aria-labelledby so axe button-name/4.1.2 passes. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
Addressed the remaining review findings:
@coderabbitai review |
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/dropdown/dropdown.js (1)
127-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExpose the selected state on each option.
The new
role="option"elements only use.selectedand anaria-hiddencheckmark. Setaria-selected="true"on the selected option and"false"on the others. Keep the attribute synchronized inselectItem(). Otherwise, assistive technology cannot report the current selection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/dropdown/dropdown.js` around lines 127 - 131, Update the option creation logic around menuItem and selectItem() so every role="option" element initializes aria-selected to "true" for the selected item and "false" otherwise, then synchronize that attribute whenever selectItem() changes the selection. Keep the existing .selected and aria-hidden checkmark behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@components/dropdown/dropdown.js`:
- Around line 127-131: Update the option creation logic around menuItem and
selectItem() so every role="option" element initializes aria-selected to "true"
for the selected item and "false" otherwise, then synchronize that attribute
whenever selectItem() changes the selection. Keep the existing .selected and
aria-hidden checkmark behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d170d4d-dc1c-4c66-a110-a756b53f3e45
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/pr.ymlcomponents/dropdown/README.mdcomponents/dropdown/dropdown.jspackage.jsontests/dropdown-focus.spec.jstests/helpers/a11y.js
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/pr.yml
- components/dropdown/README.md
- package.json
- tests/dropdown-focus.spec.js
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #8 ([a11y][D3]). The dropdown now exposes a recognizable select-only combobox → listbox pattern instead of a button popup with button-hosted options.
Changes
In
components/dropdown/:role="combobox",aria-haspopup="listbox",aria-controls→ listbox id (base component, not only PortalDropdown)role="listbox";.dropdown-menu-listisrole="presentation"so options stay owned by the listboxdiv[role=option][tabindex=-1](not<button>); decorative checkmarksaria-hiddenD4 (
aria-selected) intentionally untouched — sequenced follow-up. No app cooperating change required;PortalDropdownkeeps working becausearia-controlsstill targetsthis.menu.Test plan
components/dropdown/test.html(light + dark): toggleRole=combobox, aria-haspopup=listbox, aria-controls resolves, menuRole=listbox, listRole=presentation, optionTag=DIV, no button options, ArrowDown still focuses an optionMade with Cursor