feat: keyboard annotation #7503
Conversation
gmorador-tribu
requested review from
Copilot,
karenrasmussen and
santicomp2014
There was a problem hiding this comment.
Pull request overview
Adds comprehensive keyboard-only interaction patterns to the draw tool (rect + point) to improve accessibility/VPAT compliance, including mode switching, screen reader announcements, and visual feedback.
Changes:
- Introduces keyboard modes (
rect/move/resize) with pinned-corner resizing for rectangles. - Adds new utilities + UI components to render keyboard feedback (surface/indicator) and announce changes to screen readers.
- Wires keyboard mode state between Guest ↔ Sidebar via RPC and exposes state through the toolbar controller, with extensive tests.
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/annotator.ts | Adds KeyboardMode, PinnedCorner, and resize-corner order constant for keyboard resizing. |
| src/sidebar/components/ThreadCard.tsx | Keeps sidebar highlight active when focus moves within a thread card (keyboard navigation). |
| src/annotator/util/test/rect-resize-test.js | Adds unit coverage for rectangle resize keyboard logic. |
| src/annotator/util/test/rect-move-test.js | Adds unit coverage for move keyboard logic for rects/points. |
| src/annotator/util/test/pinned-corner-test.js | Adds coverage for pinned-corner labeling used in a11y strings/indicators. |
| src/annotator/util/test/draw-tool-position-test.js | Adds coverage for initial positioning + viewport clamping helpers. |
| src/annotator/util/rect-resize.ts | Implements pinned-corner resize behavior for rectangles. |
| src/annotator/util/rect-move.ts | Implements keyboard movement logic for rectangles and points. |
| src/annotator/util/pinned-corner.ts | Adds label formatter for pinned corners used in a11y text. |
| src/annotator/util/draw-tool-position.ts | Adds positioning + viewport bounds utilities for keyboard drawing. |
| src/annotator/toolbar.tsx | Extends ToolbarController to track keyboard mode state and callbacks. |
| src/annotator/test/toolbar-test.js | Adds tests for toolbar controller keyboard state and callback wiring. |
| src/annotator/test/sidebar-test.js | Adds tests for sidebar ↔ guest keyboard mode RPC and more branch coverage. |
| src/annotator/test/selection-observer-test.js | Extends coverage for selectionchange (keyboard) debounce behavior. |
| src/annotator/test/range-util-test.js | Extends coverage for range utilities used by selection/adder. |
| src/annotator/test/integration/hypothesis-injector-test.js | Adds integration coverage for “already injected” iframe branch. |
| src/annotator/test/highlight-clusters-test.js | Adds coverage for feature-flag-off branch in cluster updates. |
| src/annotator/test/guest-test.js | Adds extensive tests for new keyboard hotkeys and keyboard mode RPC. |
| src/annotator/test/frame-observer-test.js | Adds coverage for frame discovery and onDocumentReady edge cases. |
| src/annotator/test/draw-tool-test.js | Adds extensive tests for draw tool keyboard modes + a11y behavior. |
| src/annotator/test/adder-test.js | Adds coverage for adder positioning with positioned ancestors. |
| src/annotator/sidebar.tsx | Adds toolbar keyboard-mode cycling callbacks and listens to guest keyboard mode events. |
| src/annotator/guest.ts | Adds global hotkey listener and RPC handlers for keyboard mode controls. |
| src/annotator/draw-tool.tsx | Adds core keyboard mode behavior, surface rendering with mode feedback, announcer, scroll handling. |
| src/annotator/components/test/Toolbar-test.js | Adds UI tests for keyboard-mode titles and Enter-key activation callbacks. |
| src/annotator/components/test/DrawToolSurface-test.js | Adds tests for resize-mode edge indicators and rendering branches. |
| src/annotator/components/test/DrawToolKeyboardIndicator-test.js | Adds tests for the keyboard mode indicator overlay. |
| src/annotator/components/test/DrawToolAnnouncer-test.js | Adds tests for live-region announcements across tools/modes. |
| src/annotator/components/icons/test/icons-test.js | Adds basic render tests for new keyboard mode icons. |
| src/annotator/components/icons/index.ts | Exports new keyboard mode icons. |
| src/annotator/components/icons/ResizeModeIcon.tsx | Adds resize-mode icon SVG. |
| src/annotator/components/icons/MoveModeIcon.tsx | Adds move-mode icon SVG. |
| src/annotator/components/Toolbar.tsx | Updates toolbar UI/behavior for keyboard modes, titles, and Enter-key activation. |
| src/annotator/components/DrawToolSurface.tsx | Extracts surface rendering with resize-mode edge indicators. |
| src/annotator/components/DrawToolKeyboardIndicator.tsx | Adds visible keyboard mode indicator overlay component. |
| src/annotator/components/DrawToolAnnouncer.tsx | Adds live-region announcer component for keyboard interactions. |
| src/annotator/anchoring/test/pdf-test.js | Adds coverage for FakePDFViewerApplication branches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set modeButtonCallbacks(callbacks: { | ||
| onModeClick?: () => void; | ||
| onActivateMoveMode?: () => void; | ||
| onActivatePointMoveMode?: () => void; | ||
| }) { | ||
| this._onModeClick = callbacks.onModeClick; | ||
| this._onActivateMoveMode = callbacks.onActivateMoveMode; | ||
| this._onActivatePointMoveMode = callbacks.onActivatePointMoveMode; | ||
| this.render(); | ||
| } |
There was a problem hiding this comment.
This defines a setter-only property. Reading toolbar.modeButtonCallbacks will yield undefined (no getter), but tests and other code access sidebar.toolbar.modeButtonCallbacks.onModeClick() etc. Add a getter that returns the currently-registered callbacks (or expose a method) so callers can safely access them.
src/annotator/draw-tool.tsx
Outdated
| const containerRect = this._container.getBoundingClientRect(); | ||
| const maxX = containerRect.width; | ||
| const maxY = containerRect.height; | ||
| const viewport = getViewportBounds( | ||
| this._container, | ||
| DrawTool._RESERVED_VIEWPORT_TOP, | ||
| ); | ||
|
|
||
| if (this._shape.type === 'point' && this._keyboardMode === 'move') { | ||
| this._shape = applyMoveArrowKeyToPoint( | ||
| this._shape, | ||
| key, | ||
| increment, | ||
| maxX, | ||
| maxY, | ||
| ); |
There was a problem hiding this comment.
Point movement uses maxX/maxY derived from containerRect.width/height, but point coordinates are in container content coordinates (they include scrollLeft/scrollTop from computeInitialShapePosition). If the container is scrolled, x/y can exceed containerRect.width/height, causing the point to clamp incorrectly and potentially jump. Clamp points using viewport bounds (eg. minLeft/minTop/maxRight/maxBottom from getViewportBounds) rather than [0..width].
src/annotator/util/rect-resize.ts
Outdated
| if (newRight - left >= minWidth) { | ||
| result.right = newRight; | ||
| } | ||
| break; | ||
| } | ||
| case 'ArrowDown': { | ||
| const newBottom = Math.min(top + maxHeight, bottom + increment); | ||
| if (newBottom - top >= minHeight) { | ||
| result.bottom = newBottom; | ||
| } |
There was a problem hiding this comment.
The min-size guard blocks expanding when the rect is currently smaller than minWidth. Example: width 9, increment 10, minWidth 20 ⇒ newRight-left becomes 19, so right never updates and the user can never expand out of the invalid state. Expansion operations (ArrowRight/ArrowDown depending on pinned corner) should generally always apply and only contractions should enforce min size, or you should normalize the rect to at least min size before entering resize mode.
| if (newRight - left >= minWidth) { | |
| result.right = newRight; | |
| } | |
| break; | |
| } | |
| case 'ArrowDown': { | |
| const newBottom = Math.min(top + maxHeight, bottom + increment); | |
| if (newBottom - top >= minHeight) { | |
| result.bottom = newBottom; | |
| } | |
| // Expansion: always apply (subject to maxWidth), even if current width < minWidth. | |
| result.right = newRight; | |
| break; | |
| } | |
| case 'ArrowDown': { | |
| const newBottom = Math.min(top + maxHeight, bottom + increment); | |
| // Expansion: always apply (subject to maxHeight), even if current height < minHeight. | |
| result.bottom = newBottom; |
| const relatedTarget = e.relatedTarget as Node | null; | ||
|
|
||
| // If relatedTarget is null, focus is leaving the document/frame entirely | ||
| // If relatedTarget exists but is not contained in the card, focus is moving outside | ||
| if (!relatedTarget || !cardElement.contains(relatedTarget)) { | ||
| setThreadHovered(null); | ||
| } | ||
| // If relatedTarget exists and is contained in the card, focus is moving | ||
| // to another element within the card (e.g., from a button to a link), | ||
| // so we keep the highlight active |
There was a problem hiding this comment.
FocusEvent.relatedTarget is EventTarget | null, not guaranteed to be a Node. Casting to Node can cause cardElement.contains(relatedTarget) to throw at runtime if relatedTarget is not a Node. Safer pattern: check relatedTarget instanceof Node before calling contains, and treat non-Node targets as leaving the card.
| const relatedTarget = e.relatedTarget as Node | null; | |
| // If relatedTarget is null, focus is leaving the document/frame entirely | |
| // If relatedTarget exists but is not contained in the card, focus is moving outside | |
| if (!relatedTarget || !cardElement.contains(relatedTarget)) { | |
| setThreadHovered(null); | |
| } | |
| // If relatedTarget exists and is contained in the card, focus is moving | |
| // to another element within the card (e.g., from a button to a link), | |
| // so we keep the highlight active | |
| const relatedTarget = e.relatedTarget; | |
| // If relatedTarget is null, focus is leaving the document/frame entirely. | |
| // If relatedTarget exists but is not a Node or is not contained in the card, | |
| // treat it as moving focus outside the card. | |
| if ( | |
| !relatedTarget || | |
| !(relatedTarget instanceof Node) || | |
| !cardElement.contains(relatedTarget) | |
| ) { | |
| setThreadHovered(null); | |
| } | |
| // If relatedTarget exists and is contained in the card, focus is moving | |
| // to another element within the card (e.g., from a button to a link), | |
| // so we keep the highlight active. |
src/annotator/guest.ts
Outdated
| * Set up global keyboard listener for hotkeys that work even when not in annotation mode. | ||
| */ | ||
| private _setupGlobalKeyboardListener() { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| // Check if user is typing in an input field - don't intercept keyboard shortcuts | ||
| if ( | ||
| e.target instanceof HTMLElement && | ||
| ['INPUT', 'TEXTAREA'].includes(e.target.tagName) | ||
| ) { |
There was a problem hiding this comment.
The hotkey guard only excludes INPUT and TEXTAREA. This can still intercept shortcuts while the user is typing in other editable contexts (eg. [contenteditable], elements with role=\"textbox\", or embedded editors). Consider using a shared 'is user typing' helper (if one exists in the codebase) or extend the check to cover contenteditable and ARIA textbox roles to better meet the intent of WCAG 2.1.4.
| * Set up global keyboard listener for hotkeys that work even when not in annotation mode. | |
| */ | |
| private _setupGlobalKeyboardListener() { | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| // Check if user is typing in an input field - don't intercept keyboard shortcuts | |
| if ( | |
| e.target instanceof HTMLElement && | |
| ['INPUT', 'TEXTAREA'].includes(e.target.tagName) | |
| ) { | |
| * Return true if the event target is in an editable context where we should | |
| * not intercept keyboard shortcuts. | |
| */ | |
| private _isEditableEventTarget(target: EventTarget | null): boolean { | |
| if (!(target instanceof HTMLElement)) { | |
| return false; | |
| } | |
| const tagName = target.tagName; | |
| if (tagName === 'INPUT' || tagName === 'TEXTAREA') { | |
| return true; | |
| } | |
| // Includes elements or ancestors with `contenteditable`. | |
| if (target.isContentEditable) { | |
| return true; | |
| } | |
| // ARIA text-input widgets (eg. custom editors). | |
| const roleEditable = target.closest( | |
| '[role="textbox"],[role="searchbox"],[role="combobox"]', | |
| ); | |
| return !!roleEditable; | |
| } | |
| /** | |
| * Set up global keyboard listener for hotkeys that work even when not in annotation mode. | |
| */ | |
| private _setupGlobalKeyboardListener() { | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| // Check if user is typing in an editable field - don't intercept keyboard shortcuts | |
| if (this._isEditableEventTarget(e.target)) { |
| setOnKeyboardModeChange( | ||
| callback: (state: { | ||
| keyboardActive: boolean; | ||
| keyboardMode: KeyboardMode; | ||
| }) => void, | ||
| ) { | ||
| this._onKeyboardModeChange = callback; | ||
| } |
There was a problem hiding this comment.
This API doesn't allow consumers to clear the callback (eg. setOnKeyboardModeChange(undefined)), but tests and typical usage patterns may want to unregister listeners. Consider changing the signature to accept callback?: ... and set _onKeyboardModeChange = callback.
src/annotator/draw-tool.tsx
Outdated
| /** Callback to activate annotation mode when hotkey is pressed outside of drawing mode */ | ||
| private _onActivateAnnotationMode?: (mode: 'move' | 'resize') => void; |
There was a problem hiding this comment.
_onActivateAnnotationMode and setOnActivateAnnotationMode are added but not used anywhere in this file (no invocations). If this hook is no longer needed (hotkeys are handled in guest.ts), remove it to avoid dead API surface; otherwise, wire it up or add a TODO explaining intended usage.
src/annotator/draw-tool.tsx
Outdated
| /** | ||
| * Set callback to activate annotation mode when hotkey is pressed. | ||
| */ | ||
| setOnActivateAnnotationMode(callback: (mode: 'move' | 'resize' | 'rect') => void) { | ||
| this._onActivateAnnotationMode = callback; | ||
| } |
There was a problem hiding this comment.
_onActivateAnnotationMode and setOnActivateAnnotationMode are added but not used anywhere in this file (no invocations). If this hook is no longer needed (hotkeys are handled in guest.ts), remove it to avoid dead API surface; otherwise, wire it up or add a TODO explaining intended usage.
| export function DrawToolSurface({ | ||
| shape, | ||
| tool, | ||
| waitingForSecondClick, | ||
| firstClickPoint, | ||
| keyboardMode, | ||
| keyboardActive, | ||
| pinnedCorner, | ||
| }: DrawToolSurfaceProps) { |
There was a problem hiding this comment.
tool is destructured but not used anywhere in this component. If it is not needed, remove it from the destructuring (and potentially from DrawToolSurfaceProps) to avoid unused-parameter churn and simplify the API.
src/types/annotator.ts
Outdated
| | 'bottom-left' | ||
| | 'bottom-right'; |
There was a problem hiding this comment.
The doc comment specifies the cycling order includes bottom-right → bottom-left, but the PinnedCorner union lists bottom-left before bottom-right. Union order is not semantic, but swapping these two entries to match the documented order reduces confusion when scanning types.
| | 'bottom-left' | |
| | 'bottom-right'; | |
| | 'bottom-right' | |
| | 'bottom-left'; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7503 +/- ##
==========================================
+ Coverage 99.51% 99.61% +0.10%
==========================================
Files 273 283 +10
Lines 11263 11859 +596
Branches 2701 2899 +198
==========================================
+ Hits 11208 11813 +605
+ Misses 55 46 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
santicomp2014
left a comment
santicomp2014
left a comment
There was a problem hiding this comment.
Added a couple comments, some minor ones and nice to have.
the critical ones are already fixed thank you.
santicomp2014
left a comment
santicomp2014
left a comment
There was a problem hiding this comment.
Approved, it very large, but we have this under a feature flag.
we can roll it back if we see the slightest issue.
for future reference we could split this up into mini MRs and/or use feature/vpat-keyboard --> feature/vpat-keyboard-utils etc.. instead of one MR.
Just an idea for future iterations.
Great job
2 participants
Add keyboard navigation modes for draw tool (VPAT compliance)
Summary
This PR adds comprehensive keyboard navigation support to the draw tool (rectangle and point annotations), enabling users to create and modify annotations using only the keyboard. This improves accessibility and VPAT compliance for keyboard-only users and screen reader users.
✨ Features
Three Keyboard Modes
'rect': Default rectangle mode (no keyboard movement)'move': Move shapes with arrow keys'resize': Resize rectangles with arrow keys (with pinned corner support)Keyboard Controls
Visual and Screen Reader Feedback
DrawToolKeyboardIndicator: Visual overlay showing current mode and instructionsDrawToolAnnouncer: Live region announcements for screen readers with position/size updatesDrawToolSurface: Visual indicators for active edges in resize modeMode Switching
rect→move→resize→rectsetKeyboardMode()🔧 Technical Changes
New Types
KeyboardMode:'move' | 'resize' | 'rect' | nullNew Utility Modules
rect-move.ts: Arrow key movement logic for rectangles and pointsrect-resize.ts: Arrow key resizing with pinned corner supportpinned-corner.ts: Corner labeling utilities for accessibilitydraw-tool-position.ts: Position calculation helpersNew React Components
DrawToolSurface.tsx: SVG surface with visual feedback for resize modeDrawToolKeyboardIndicator.tsx: Mode indicator overlay componentDrawToolAnnouncer.tsx: Screen reader announcement componentEnhanced Classes
DrawTool: Added keyboard mode state management, arrow key handlers, and mode switchingGuest: Added RPC handlers for keyboard mode synchronizationSidebar: Added mode button callback for cycling through modesToolbar: Added keyboard mode state display and iconsRPC Communication
keyboardModeChanged: Event emitted when keyboard mode state changessetKeyboardMode: Command to programmatically set keyboard mode♿ Accessibility Improvements
🧪 Testing
Comprehensive test coverage added for:
getKeyboardModeState,setKeyboardMode)