♿️(frontend) limit share modal opening announcement for screen readers#2452
♿️(frontend) limit share modal opening announcement for screen readers#2452Ovgodd wants to merge 1 commit into
Conversation
Autofocus close and hide modal body from SR when search input is hidden.
Ovgodd
force-pushed
the
fix/a11y-2334-share-modal-access-names
branch
from
065c3b1 to
49c6f8e
Compare
|
Size Change: +1 B (0%) Total Size: 4.33 MB 📦 View Changed
|
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
| const DEBOUNCE_MS = 300; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Decouple search debounce timing from accessibility timing.
DEBOUNCE_MS currently drives both query filtering (Line 125) and accessibility re-exposure delay (Line 176). This couples unrelated UX knobs and makes future tuning risky.
Suggested refactor
-const DEBOUNCE_MS = 300;
+const SEARCH_DEBOUNCE_MS = 300;
+const A11Y_CONTENT_REVEAL_DELAY_MS = 300;
...
- }, DEBOUNCE_MS);
+ }, SEARCH_DEBOUNCE_MS);
...
- }, DEBOUNCE_MS);
+ }, A11Y_CONTENT_REVEAL_DELAY_MS);Also applies to: 125-125, 176-176
🤖 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, 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.
| // 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; | ||
| } | ||
|
|
||
| const id = window.setTimeout(() => { | ||
| setIsContentAccessible(true); | ||
| }, DEBOUNCE_MS); | ||
|
|
||
| return () => window.clearTimeout(id); | ||
| }, [canShare]); | ||
|
|
There was a problem hiding this comment.
Ensure isContentAccessible is reset on every canShare transition.
Line 169 only handles the canShare === false branch by scheduling true, but it never forces false when canShare flips from true to false after mount. In that transition, Line 227 can keep content exposed to assistive tech, so the announcement-limiting behavior is skipped.
Suggested fix
useEffect(() => {
- if (canShare) {
- return;
- }
+ setIsContentAccessible(canShare);
+ if (canShare) return;
const id = window.setTimeout(() => {
setIsContentAccessible(true);
}, DEBOUNCE_MS);
return () => window.clearTimeout(id);
}, [canShare]);Also applies to: 227-227
🤖 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, 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.
1 participant
Purpose
Limit the screen reader announcement when opening the share modal without the search input.
Proposal