Fix export format select keyboard focus#2366
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe ModalExport component now imports useEffect and useRef, wraps the format Select in a ref-bound container, and adds a useEffect that finds the select's internal open/toggle button and sets aria-hidden="true" and tabindex="-1". The e2e test was extended to assert the toggle is hidden from assistive tech and removed from the tab order. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e40f7840-29f4-43bc-9e7e-9b19bb347f15
📒 Files selected for processing (1)
src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx
| useEffect(() => { | ||
| const toggleButton = formatSelectRef.current?.querySelector<HTMLElement>( | ||
| '.c__select__inner__actions__open', | ||
| ); | ||
|
|
||
| toggleButton?.setAttribute('aria-hidden', 'true'); | ||
| toggleButton?.setAttribute('tabindex', '-1'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find export-modal tests and any assertions on the arrow/select attributes
fd -t f . | rg -i 'export' | rg -i 'spec|test|e2e'
rg -nP -C3 'c__select__inner__actions__open|aria-hidden|tabindex' \
--glob '*.spec.*' --glob '*.test.*' --glob '*e2e*' --glob '*cypress*' --glob '*playwright*'Repository: suitenumerique/docs
Length of output: 3831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check whether the internal Cunningham class is referenced anywhere in tests/code
rg -n --hidden --no-ignore-vcs 'c__select__inner__actions__open' . || true
# 2) Check for aria-hidden/tabindex assertions specifically in doc-export related e2e tests
rg -n --hidden --no-ignore-vcs \
'doc-export|utils-export|app-impress/doc-export|ModalExport|Export' \
src/frontend/apps/e2e/__tests__ || true
rg -n --hidden --no-ignore-vcs \
'aria-hidden\s*=\s*["'\'']true["'\'']|aria-hidden|toHaveAttribute\(.+aria-hidden|tabindex\s*=\s*["'\'']-1["'\'']|toHaveAttribute\(.+tabindex' \
src/frontend/apps/e2e/__tests__/app-impress/*.spec.* || true
# 3) Also check in app-impress tests/utils-export (if any)
rg -n --hidden --no-ignore-vcs \
'aria-hidden\s*=\s*["'\'']true["'\'']|toHaveAttribute\(.+aria-hidden|tabindex\s*=\s*["'\'']-1["'\'']|toHaveAttribute\(.+tabindex' \
src/frontend/apps/e2e/__tests__/app-impress/** || trueRepository: suitenumerique/docs
Length of output: 4640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore-vcs \
"toHaveAttribute\\(.+aria-hidden|aria-hidden\\s*=\\s*['\"]true['\"]|toHaveAttribute\\(.+tabindex\\s*,\\s*['\"]-1['\"]|tabindex\\s*=\\s*['\"]-1['\"]" \
src/frontend/apps/e2e/__tests__/app-impress/**/*.spec.* \
|| true
rg -n --hidden --no-ignore-vcs \
"aria-hidden|tabindex" \
src/frontend/apps/e2e/__tests__/app-impress/**/*.spec.* \
|| trueRepository: suitenumerique/docs
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
base="src/frontend/apps/e2e/__tests__/app-impress"
rg -n --hidden --no-ignore-vcs \
--glob '*.spec.*' --glob '*.test.*' --glob '*e2e*' --glob '*cypress*' --glob '*playwright*' \
'toHaveAttribute\(.+aria-hidden|aria-hidden\s*=\s*['\'']true['\'']|toHaveAttribute\(.+tabindex\s*,\s*['\'']-1['\'']|tabindex\s*=\s*['\'']-1['\'']' \
"$base" || true
rg -n --hidden --no-ignore-vcs \
--glob '*.spec.*' --glob '*.test.*' --glob '*e2e*' --glob '*cypress*' --glob '*playwright*' \
'(aria-hidden|tabindex)' \
"$base" || trueRepository: suitenumerique/docs
Length of output: 603
Guard Cunningham internal Select class + add a11y regression test
ModalExport.tsx sets aria-hidden="true" and tabindex="-1" by querying Cunningham’s internal DOM node via .c__select__inner__actions__open. If Cunningham changes the markup/class, both setAttribute calls will silently no-op and the toggle can re-enter the accessibility/focus flow.
- Add an e2e/RTL regression test that asserts the toggle element has
aria-hidden="true"andtabindex="-1"in the export modal. - Prefer a supported Cunningham
SelectAPI/prop to hide the toggle from a11y (or add a guard/fallback), rather than relying on the internal class.
Keeping the effect without a dependency array is reasonable so the attributes are re-applied across Cunningham re-renders.
|
Updated after review feedback:
Local validation: Prettier check passed for the updated e2e spec. GitHub checks are currently green. |
1 participant
Summary
Why
In the export modal, keyboard and screen reader users currently encounter a separate focus stop for the dropdown arrow even though the combobox itself is the interactive control. This adds noise and makes the modal harder to navigate.
Fixes #2342.
Checks
npx -y prettier@3.8.3 --check apps/impress/src/features/docs/doc-export/components/ModalExport.tsxaria-hidden="true"andtabindex="-1"Note: I did not run the full app test suite because dependencies are not installed in this checkout.