♿️(frontend) limit share modal opening announcement for screen readers#2452
♿️(frontend) limit share modal opening announcement for screen readers#2452Ovgodd wants to merge 4 commits into
Conversation
065c3b1 to
49c6f8e
Compare
|
Size Change: -1 B (0%) Total Size: 4.37 MB 📦 View Changed
|
|
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)
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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
`@src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx`:
- Around line 43-44: The constant DEBOUNCE_MS is being used for two unrelated
purposes: query filtering and accessibility re-exposure delay. Create two
separate constants instead - one specifically for search query debouncing and
another for the accessibility re-exposure delay timing. This decouples these
independent concerns so each can be tuned separately without affecting the
other. Find where DEBOUNCE_MS is used at the query filtering location (around
line 125) and the accessibility re-exposure location (around line 176), then
replace the appropriate usages with their respective new constants.
- Around line 167-180: The useEffect hook in DocShareModal only handles the case
when `canShare` is false by scheduling `isContentAccessible` to true, but it
does not reset `isContentAccessible` to false when `canShare` transitions from
false to true. To fix this, add a branch in the useEffect that immediately sets
`isContentAccessible` to false when `canShare` becomes true, ensuring the
content is removed from the accessibility tree during the opening announcement
for every transition of the `canShare` state.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06e60d43-e900-4cc3-9140-39504c2bd9d2
📒 Files selected for processing (2)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx
Autofocus close and hide modal body from SR when search input is hidden.
49c6f8e to
8f41648
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx (2)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDecouple search debounce timing from accessibility timing.
DEBOUNCE_MScurrently drives both query filtering and the accessibility re-exposure delay. This couples unrelated UX behaviors and makes future tuning risky.♻️ Suggested refactor
-const DEBOUNCE_MS = 300; +const SEARCH_DEBOUNCE_MS = 300; +const A11Y_CONTENT_REVEAL_DELAY_MS = 300;Update the usage in
useDebouncedCallback(around line 125):const onFilter = useDebouncedCallback((str: string) => { setUserQuery(str); }, SEARCH_DEBOUNCE_MS);Update the usage in the
useEffecthook (around line 176):const id = window.setTimeout(() => { setIsContentAccessible(true); }, A11Y_CONTENT_REVEAL_DELAY_MS);🤖 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 `@src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx` around lines 43 - 44, Split the shared DEBOUNCE_MS constant into distinct SEARCH_DEBOUNCE_MS and A11Y_CONTENT_REVEAL_DELAY_MS constants in DocShareModal. Update onFilter’s useDebouncedCallback to use SEARCH_DEBOUNCE_MS and the accessibility useEffect timeout to use A11Y_CONTENT_REVEAL_DELAY_MS, preserving the existing timing values.
167-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnsure
isContentAccessibleis reset on everycanSharetransition.The effect only handles the
canShare === falsebranch by schedulingtrue, but it does not resetisContentAccessibletofalsewhencanSharetransitions from true to false after mount. In that transition, the content can remain exposed to assistive tech, bypassing the announcement-limiting behavior.🐛 Suggested fix
// When the search input is hidden, keep the modal content out of the // accessibility tree during the opening announcement, then restore it. useEffect(() => { - if (canShare) { - return; - } + setIsContentAccessible(canShare); + if (canShare) return; const id = window.setTimeout(() => { setIsContentAccessible(true); }, DEBOUNCE_MS); return () => window.clearTimeout(id); }, [canShare]);🤖 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 `@src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx` around lines 167 - 180, Update the useEffect keyed by canShare to immediately reset isContentAccessible to false whenever canShare is false, then schedule restoring it to true after DEBOUNCE_MS. Preserve timeout cleanup and the existing canShare true behavior so every false transition re-applies the accessibility suppression.
🤖 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.
Duplicate comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx`:
- Around line 43-44: Split the shared DEBOUNCE_MS constant into distinct
SEARCH_DEBOUNCE_MS and A11Y_CONTENT_REVEAL_DELAY_MS constants in DocShareModal.
Update onFilter’s useDebouncedCallback to use SEARCH_DEBOUNCE_MS and the
accessibility useEffect timeout to use A11Y_CONTENT_REVEAL_DELAY_MS, preserving
the existing timing values.
- Around line 167-180: Update the useEffect keyed by canShare to immediately
reset isContentAccessible to false whenever canShare is false, then schedule
restoring it to true after DEBOUNCE_MS. Preserve timeout cleanup and the
existing canShare true behavior so every false transition re-applies the
accessibility suppression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f498e66-859f-465a-b7ec-6a85c64fa1d7
📒 Files selected for processing (2)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx
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
`@src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx`:
- Around line 170-183: Update the DocShareModal focus and visibility logic
around the canShare-driven useEffect: synchronously derive the
hidden/accessibility state from canShare, and when canShare transitions from
true to false, move focus to the close button before the share content becomes
aria-hidden. Preserve the delayed re-exposure behavior and cleanup for the
accessibility timeout.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cb9a398-c983-4d13-b5ab-8a4ac2fe2989
📒 Files selected for processing (1)
src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx
| useEffect(() => { | ||
| if (canShare) { | ||
| setIsContentAccessible(true); | ||
| return; | ||
| } | ||
|
|
||
| setIsContentAccessible(false); | ||
|
|
||
| const id = window.setTimeout(() => { | ||
| setIsContentAccessible(true); | ||
| }, ACCESSIBILITY_REEXPOSURE_DELAY_MS); | ||
|
|
||
| return () => window.clearTimeout(id); | ||
| }, [canShare]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx'
echo '== outline =='
ast-grep outline "$file" --view expanded || true
echo
echo '== line count =='
wc -l "$file"
echo
echo '== relevant ranges =='
sed -n '1,280p' "$file" | cat -nRepository: suitenumerique/docs
Length of output: 11433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx'
nl -ba "$file" | sed -n '1,320p'Repository: suitenumerique/docs
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx'
echo '== focus-related symbols =='
grep -nE 'focus|autoFocus|ref=' "$file" || true
echo
echo '== canShare / isContentAccessible usage =='
grep -nE 'canShare|isContentAccessible|aria-hidden' "$file" || true
echo
echo '== lower half of file =='
sed -n '280,421p' "$file" | cat -nRepository: suitenumerique/docs
Length of output: 6166
Move focus out of the share content before canShare hides it. autoFocus={!canShare} only runs on mount, so a true → false transition can leave focus inside an aria-hidden subtree. Shift focus to the close button on that transition and make the hidden state update synchronously from canShare instead of waiting for the effect.
🤖 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
`@src/frontend/apps/impress/src/features/docs/doc-share/components/DocShareModal.tsx`
around lines 170 - 183, Update the DocShareModal focus and visibility logic
around the canShare-driven useEffect: synchronously derive the
hidden/accessibility state from canShare, and when canShare transitions from
true to false, move focus to the close button before the share content becomes
aria-hidden. Preserve the delayed re-exposure behavior and cleanup for the
accessibility timeout.
| > | ||
| <ShareModalStyle /> | ||
| <Box | ||
| aria-hidden={isContentAccessible ? undefined : true} |
There was a problem hiding this comment.
I am not sure to understand why do we do that ? Seems strange to hide a part of the modal a few milliseconds, without apparent reason.
There was a problem hiding this comment.
Yes good point, I should have added a comment, just pushed one!
I first tried with just autoFocus on the close button but it was not working, NVDA was still reading all the modal body on open (Suggestions, link settings, Copy link…). After several tests I found that this temporary aria-hidden (300ms) is the only way to block this initial reading. After the timeout it's removed and everything works normally with Tab navigation.
2 participants
Purpose
Limit the screen reader announcement when opening the share modal without the search input.
Proposal