♿️(frontend) focus export modal on format select#2421
Open
Ovgodd wants to merge 1 commit into
Open
Conversation
Focus export modal on format select for ARIA dialog compliance
Ovgodd
force-pushed
the
fix/a11y-2343-first-focusble-element
branch
from
77e14f2 to
55b5088
Compare
Contributor
|
Size Change: 0 B Total Size: 4.33 MB 📦 View Changed
|
WalkthroughThis PR adds focus management to the export modal's format selector. The ModalExport component is enhanced with 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 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: 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-export/components/ModalExport.tsx`:
- Around line 70-78: The focus-selector in ModalExport's useEffect is too broad
and may hit unintended nested buttons; update the query inside the
requestAnimationFrame callback to only target the Select component via
selectRef.current?.querySelector<HTMLButtonElement>('[role="combobox"]') (keep
the existing requestAnimationFrame/cancelAnimationFrame logic and the useEffect
as-is) so only the format Select gains focus; locate this change in the
useEffect inside the ModalExport component where selectRef is used.
🪄 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: 449627ad-1dc0-42dc-bafa-43497f0630e4
📒 Files selected for processing (2)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx
Comment on lines
+70
to
+78
| useEffect(() => { | ||
| const frameId = requestAnimationFrame(() => { | ||
| const button = selectRef.current?.querySelector<HTMLButtonElement>( | ||
| 'button, [role="combobox"]', | ||
| ); | ||
| button?.focus(); | ||
| }); | ||
| return () => cancelAnimationFrame(frameId); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for Modal lifecycle props in Cunningham or similar focus patterns in the codebase
# Search for Modal usage with lifecycle callbacks
rg -n -C3 'from.*cunningham.*Modal' --type=tsx --type=ts
# Look for other focus management patterns after modal open
rg -n -C3 'requestAnimationFrame.*focus' --type=tsx --type=tsRepository: suitenumerique/docs
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact focus effect in the target file
echo "=== ModalExport.tsx (lines ~50-95) ==="
nl -ba src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx | sed -n '45,105p'
# 2) Find how Cunningham Modal is imported/used in this repo (tsx/js)
echo "=== Cunningham Modal imports/usages ==="
rg -n --glob='*.tsx' --glob='*.ts' -C2 'from\s+["'\''].*cunningham-react.*Modal["'\'']' || true
rg -n --glob='*.tsx' --glob='*.ts' -C2 '\bModal\b' src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx || true
# 3) Look for common lifecycle props/handlers around Modal
echo "=== Modal lifecycle prop names in TS/TSX ==="
rg -n --glob='*.tsx' --glob='*.ts' -C2 '\b(onAfterOpen|onOpened|onOpen|onAfterClose|onClosed|onClose)\b' src/frontend/apps/impress/src/features/docs/doc-export/components || true
rg -n --glob='*.tsx' --glob='*.ts' -C2 '\b(onAfterOpen|onOpened|onOpen|onAfterClose|onClosed|onClose)\b' src/frontend/apps/impress/src || true
# 4) Check for other requestAnimationFrame + focus patterns in the repo
echo "=== requestAnimationFrame .* focus patterns ==="
rg -n --glob='*.tsx' --glob='*.ts' -C2 'requestAnimationFrame.*focus|focus.*requestAnimationFrame' src/frontend/apps/impress/src || true
# 5) Ensure we didn't miss other Cunningham Modal references elsewhere
echo "=== Any Cunningham Modal usage outside doc-export ==="
rg -n --glob='*.tsx' --glob='*.ts' -C2 'cunningham-react' src/frontend/apps/impress/src || trueRepository: suitenumerique/docs
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx"
# 1) Inspect the exact focus effect in the target file (with line numbers via awk)
echo "=== ModalExport.tsx (lines 50-105) ==="
awk 'NR>=45 && NR<=105 {printf "%6d %s\n", NR, $0}' "$FILE" | sed -n '1,200p'
# 2) Find how Cunningham Modal is imported/used in this repo
echo "=== Cunningham Modal imports/usages (impress app) ==="
rg -n --glob='*.tsx' --glob='*.ts' -C2 'from\s+["'\''\`].*cunningham-react.*["'\''\`]' src/frontend/apps/impress/src || true
echo "=== Cunningham Modal usage in doc-export components ==="
rg -n --glob='*.tsx' --glob='*.ts' -C2 '\bModal\b' src/frontend/apps/impress/src/features/docs/doc-export/components || true
# 3) Look for common lifecycle prop names around Modal usage
echo "=== Modal lifecycle prop names (impress app) ==="
rg -n --glob='*.tsx' --glob='*.ts' -C2 '\b(onAfterOpen|onOpened|onOpen|onAfterClose|onClosed|onClose)\b' src/frontend/apps/impress/src || true
# 4) Check for other requestAnimationFrame + focus patterns in the repo
echo "=== requestAnimationFrame .* focus patterns (impress app) ==="
rg -n --glob='*.tsx' --glob='*.ts' -C2 'requestAnimationFrame.*focus|focus.*requestAnimationFrame' src/frontend/apps/impress/src || trueRepository: suitenumerique/docs
Length of output: 50509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx"
DIR="src/frontend/apps/impress/src/features/docs/doc-export/components"
echo "=== ModalExport.tsx imports (top of file) ==="
awk 'NR>=1 && NR<=60 {printf "%6d %s\n", NR, $0}' "$FILE"
echo "=== Find cunningham-react usage (impress app) ==="
rg -n --glob='*.ts' --glob='*.tsx' 'cunningham-react' src/frontend/apps/impress/src || true
echo "=== Search modal lifecycle prop names ==="
rg -n --glob='*.ts' --glob='*.tsx' '\bonAfterOpen\b|\bonOpened\b|\bonAfterClose\b|\bonClosed\b|\bonOpen\b|\bonClose\b' src/frontend/apps/impress/src/features || true
echo "=== requestAnimationFrame + focus patterns in doc-export/components ==="
rg -n --glob='*.ts' --glob='*.tsx' 'requestAnimationFrame\([^\)]*\).*focus|focus.*requestAnimationFrame' "$DIR" || trueRepository: suitenumerique/docs
Length of output: 27415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx"
# Print the part where selectRef is assigned and the Select is rendered
awk 'NR>=60 && NR<=150 {printf "%6d %s\n", NR, $0}' "$FILE"
awk 'NR>=200 && NR<=290 {printf "%6d %s\n", NR, $0}' "$FILE"Repository: suitenumerique/docs
Length of output: 6653
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx"
echo "=== Find selectRef usage ==="
rg -n --glob='*.tsx' 'selectRef' "$FILE"
echo "=== Show around the Select rendering ==="
rg -n --glob='*.tsx' '(<Select\b|role="combobox")' "$FILE"
# Print a couple sections around first select usage
LINE=$(rg -n '(<Select\b|role="combobox")' "$FILE" | head -n1 | cut -d: -f1)
START=$((LINE-25))
END=$((LINE+60))
awk -v s="$START" -v e="$END" 'NR>=s && NR<=e {printf "%6d %s\n", NR, $0}' "$FILE"Repository: suitenumerique/docs
Length of output: 3610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/frontend/apps/impress/src/features/docs/doc-export/components/ModalExport.tsx"
# Print around the selectRef attachment and the Select component
awk 'NR>=280 && NR<=360 {printf "%6d %s\n", NR, $0}' "$FILE"
# Check for autoFocus usage in this file
echo "=== autoFocus in ModalExport.tsx ==="
rg -n --glob='*.tsx' '\bautoFocus\b' "$FILE" || trueRepository: suitenumerique/docs
Length of output: 1606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for requestAnimationFrame-based focus anywhere in impress app
rg -n --glob='*.ts' --glob='*.tsx' 'requestAnimationFrame\([^)]*\).*focus|focus.*requestAnimationFrame' src/frontend/apps/impress/src || true
# Search for autoFocus usage (often used for deterministic initial focus)
rg -n --glob='*.ts' --glob='*.tsx' '\bautoFocus\b' src/frontend/apps/impress/src || true
# Search for useModal usage (may provide focus/restore helpers)
rg -n --glob='*.ts' --glob='*.tsx' '\buseModal\b' src/frontend/apps/impress/src/features || trueRepository: suitenumerique/docs
Length of output: 3594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect a few modal components that use autoFocus/useModal/FocusScope for initial focus behavior
FILES=(
"src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteToolBar/ModalConfirmDownloadUnsafe.tsx"
"src/frontend/apps/impress/src/features/docs/doc-versioning/components/ModalSelectVersion.tsx"
"src/frontend/apps/impress/src/features/docs/doc-management/components/ModalRemoveDoc.tsx"
"src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx"
"src/frontend/apps/impress/src/components/modal/AlertModal.tsx"
)
for f in "${FILES[@]}"; do
echo "===== $f ====="
# Print relevant region around the first autoFocus occurrence
line=$(rg -n --glob='*.tsx' 'autoFocus' "$f" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-20))
end=$((line+40))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d %s\n", NR, $0}' "$f" | sed -n '1,200p'
else
# fallback: print top part
awk 'NR>=1 && NR<=120 {printf "%6d %s\n", NR, $0}' "$f" | sed -n '1,200p'
fi
done
# Specifically inspect how useModal is used in DocMoveModal (focus restore patterns)
echo "===== DocMoveModal useModal snippet ====="
DOCMOV="src/frontend/apps/impress/src/features/docs/docs-grid/components/DocMoveModal.tsx"
awk 'NR>=60 && NR<=180 {printf "%6d %s\n", NR, $0}' "$DOCMOV" | sed -n '1,220p'Repository: suitenumerique/docs
Length of output: 14753
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find other code focusing a combobox/select inside modals
rg -n --glob='*.ts' --glob='*.tsx' '\[role="combobox"\]' src/frontend/apps/impress/src || true
# Look for the same focus-by-querySelector pattern
rg -n --glob='*.ts' --glob='*.tsx' 'querySelector<.*>\(.*role="combobox"|requestAnimationFrame\([^\)]*\)\s*=>\s*\{[^\}]*querySelector' src/frontend/apps/impress/src || true
# Look for requestAnimationFrame focusing something inside modals
rg -n --glob='*.ts' --glob='*.tsx' 'requestAnimationFrame\([^\)]*\).*\.focus\(\)' src/frontend/apps/impress/src || trueRepository: suitenumerique/docs
Length of output: 185
ModalExport focus timing: no Cunningham lifecycle hooks in-repo; RAF matches existing patterns.
- No
onAfterOpen/onOpened(or similar) CunninghamModallifecycle props are used anywhere in this codebase, so the focus timing can’t be improved by swapping to a supported hook here. - The
requestAnimationFramefocus pattern is consistent with other modals in this repo (e.g.,ModalRemoveDocusesrequestAnimationFramebecause focus restoration from other components can race withautoFocus).
Minor hardening: since selectRef only wraps the format Select, tighten the selector to [role="combobox"] (instead of button, [role="combobox"]) to avoid focusing an unintended nested button.
🤖 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-export/components/ModalExport.tsx`
around lines 70 - 78, The focus-selector in ModalExport's useEffect is too broad
and may hit unintended nested buttons; update the query inside the
requestAnimationFrame callback to only target the Select component via
selectRef.current?.querySelector<HTMLButtonElement>('[role="combobox"]') (keep
the existing requestAnimationFrame/cancelAnimationFrame logic and the useEffect
as-is) so only the format Select gains focus; locate this change in the
useEffect inside the ModalExport component where selectRef is used.
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.
Purpose
Fix keyboard navigation when opening the document export modal. By placing the foczs on the first useful interactive element.
Proposal
autoFocusfrom the Cancel button inModalExportSelectwhen the modal opensrequestAnimationFrameto ensure focus is applied after the modal is rendered in the DOM