♿️(frontend) improve presenter mode screen reader and keyboard support#2383
♿️(frontend) improve presenter mode screen reader and keyboard support#2383Ovgodd wants to merge 8 commits into
Conversation
Announce slide position via react-aria on change; drop bar live region.
Ovgodd
changed the title
Ovgodd
force-pushed
the
fix/a11y-presenter
branch
3 times, most recently
from
75fafb5 to
c4efdac
Compare
|
Size Change: +2.54 kB (+0.06%) Total Size: 4.33 MB 📦 View Changed
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR enhances presenter mode accessibility: it adds slide title extraction, announces slide changes via a polite live announcer, wraps the presenter in a FocusScope, adjusts ProseMirror DOM attributes to avoid edit announcements, replaces an sr-only slide-position region with visible text, and records/restores focus for the Present dropdown button. Component APIs are unchanged. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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-header/components/DocToolBox.tsx`:
- Around line 138-141: The callback passed to the action should defensively
guard access to optionsButtonRef.current before calling addLastFocus: update the
callback in the component where you set callback (the function that calls
addLastFocus(optionsButtonRef.current) and setIsPresenterOpen(true)) to check
that optionsButtonRef.current is non-null (or use optional chaining) before
calling addLastFocus, leaving the setIsPresenterOpen(true) call as-is; reference
the callback definition, addLastFocus, optionsButtonRef, and setIsPresenterOpen
when making the change.
In
`@src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsx`:
- Around line 131-137: The FocusScope currently uses contain which traps focus
but doesn’t move initial focus into the dialog; update the FocusScope component
(the one wrapping Box with overlayCss and role="dialog"/aria-modal="true") to
include the autoFocus prop so focus is moved to the first focusable element on
mount, and if there isn’t a natural focusable child inside PresenterOverlay add
one (e.g., a dedicated focusable element or set a ref on a button/close control
and ensure it is focusable) so autoFocus has a target.
🪄 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: a16cff44-136c-4c50-b105-15aac4ede5f8
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frontend/apps/impress/src/features/docs/doc-header/components/DocToolBox.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterFloatingBar.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterOverlay.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterSlide.tsxsrc/frontend/apps/impress/src/features/docs/doc-presenter/hooks/useSlides.ts
💤 Files with no reviewable changes (1)
- src/frontend/apps/impress/src/features/docs/doc-presenter/components/PresenterFloatingBar.tsx
Ovgodd
force-pushed
the
fix/a11y-presenter
branch
from
ba3414a to
a2416f8
Compare
Trap focus with FocusScope; remove editor role from slides for better SR sprt.
Restore focus to menu trigger after closing presenter mode.
Ovgodd
force-pushed
the
fix/a11y-presenter
branch
from
a2416f8 to
f94f27a
Compare
Ovgodd
force-pushed
the
fix/a11y-presenter
branch
from
f94f27a to
81f0177
Compare
| const extractInlineText = (content: unknown): string => { | ||
| if (typeof content === 'string') { | ||
| return content; | ||
| } | ||
| if (!Array.isArray(content)) { | ||
| return ''; | ||
| } | ||
| return content | ||
| .map((node) => { | ||
| if (typeof node === 'string') { | ||
| return node; | ||
| } | ||
| if (node && typeof node === 'object') { | ||
| const inline = node as { text?: unknown; content?: unknown }; | ||
| if (typeof inline.text === 'string') { | ||
| return inline.text; | ||
| } | ||
| if (inline.content !== undefined) { | ||
| return extractInlineText(inline.content); | ||
| } | ||
| } | ||
| return ''; | ||
| }) | ||
| .join(''); | ||
| }; | ||
|
|
||
| /** First heading text, or first block with text, for SR announcements. */ | ||
| export const getSlideTitle = ( | ||
| blocks: { type: string; content?: unknown }[], | ||
| ): string => { | ||
| const heading = blocks.find((block) => block.type === 'heading'); | ||
| const source = | ||
| heading ?? | ||
| blocks.find((block) => extractInlineText(block.content).trim().length > 0); | ||
|
|
||
| return source ? extractInlineText(source.content).trim() : ''; | ||
| }; |
There was a problem hiding this comment.
Blocknote does not have a funtion to do it ?
Could you add a test e2e to assert that it is working as expected on this part ?
There was a problem hiding this comment.
I found this discussion on BlockNote about a blocksToPlainTextLossy API, it was requested but never implemented.
we could use getText() from tiptapEditor but it returns the full document text and we need the title of each individual slide, I did not find another function for this.
As for the test, did you mean a unit test for getSlideTitle / extractInlineText? I've added one in useSlides.spec.ts covering heading priority, paragraph fallback, nested inline content (links), empty slides, and whitespace trimming (9 cases, all green).
Or did you mean just asserting the title inside the existing e2e test "announces the slide position through a live region"? I've updated that too, it now checks the full message including the title (e.g. "Slide 1 of 3: Slide one" )
2 participants
Purpose
Make the presenter mode accessible to keyboard and screen reader users:
announce slide changes, trap focus inside the overlay, and restore focus
to the trigger button on close.
Proposal
politelive region (@react-aria/live-announcer).<span>label: the live announcer now handles it, so keeping thearia-labelmade screen readers read the same info twice. The visual counter is hidden witharia-hidden="true".FocusScope contain.textbox/contenteditable,tabindex="-1") so slides aren't announced as editable, keeping links tabbable.role="group".