fix(ImageUpload): Improve accessibility with focus management for VoiceOver #3820
Conversation
📝 WalkthroughWalkthroughThis PR introduces ref forwarding and focus management enhancements to the ImageUpload component to address WCAG accessibility issues. It adds a utility for merging multiple refs, implements three custom hooks for focus control during image upload and deletion, and refactors the component hierarchy to properly restore focus after user interactions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
framitdavid
added
kind/bug
framitdavid
changed the title
![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
🧹 Nitpick comments (2)
src/app-components/Dropzone/Dropzone.tsx (1)
60-62: Avoid type cast; access ref property safely.Line 61 uses a type cast (
as) which violates the coding guidelines. ThegetInputProps()return type from react-dropzone should provide proper typing for the ref property.Consider refactoring to avoid the type cast:
const inputProps = getInputProps(); - const dropzoneRef = (inputProps as { ref?: React.Ref<HTMLInputElement> }).ref; + const dropzoneRef = 'ref' in inputProps ? inputProps.ref : undefined; const combinedRef = React.useMemo(() => RefsUtils.merge(dropzoneRef, inputRef), [dropzoneRef, inputRef]);Alternatively, if react-dropzone provides proper types, you could import and use them directly.
As per coding guidelines.
src/layout/ImageUpload/hooks/useFocusWhenUploaded.tsx (1)
42-44: Simplify redundant type guard.The
isAttachmentUploadedfunction checks theuploadedfield on a value already typed asUploadedAttachment. By definition,UploadedAttachmentalways hasuploaded: true, so this check is redundant—the function is effectively just checking forundefined.Consider simplifying:
-function isAttachmentUploaded(attachment: UploadedAttachment | undefined): attachment is UploadedAttachment { - return !!attachment?.uploaded; -} +function isAttachmentDefined(attachment: UploadedAttachment | undefined): attachment is UploadedAttachment { + return attachment !== undefined; +}Or inline the check directly in the call sites (lines 16 and 25) as
if (attachment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/app-components/Dropzone/Dropzone.tsx(5 hunks)src/layout/ImageUpload/ImageControllers.tsx(4 hunks)src/layout/ImageUpload/ImageCropper.tsx(4 hunks)src/layout/ImageUpload/ImageDropzone.tsx(2 hunks)src/layout/ImageUpload/hooks/useFocusOnChange.tsx(1 hunks)src/layout/ImageUpload/hooks/useFocusWhenRemoved.tsx(1 hunks)src/layout/ImageUpload/hooks/useFocusWhenUploaded.tsx(1 hunks)src/utils/refs/mergeRefs.test.ts(1 hunks)src/utils/refs/mergeRefs.ts(1 hunks)test/e2e/integration/component-library/image-upload.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
test/e2e/integration/component-library/image-upload.tssrc/layout/ImageUpload/hooks/useFocusWhenUploaded.tsxsrc/layout/ImageUpload/hooks/useFocusWhenRemoved.tsxsrc/utils/refs/mergeRefs.test.tssrc/layout/ImageUpload/hooks/useFocusOnChange.tsxsrc/layout/ImageUpload/ImageControllers.tsxsrc/utils/refs/mergeRefs.tssrc/app-components/Dropzone/Dropzone.tsxsrc/layout/ImageUpload/ImageCropper.tsxsrc/layout/ImageUpload/ImageDropzone.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/utils/refs/mergeRefs.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-03T14:26:18.627Z
Learnt from: olemartinorg
Repo: Altinn/app-frontend-react PR: 3645
File: src/components/wrappers/ProcessWrapper.tsx:78-83
Timestamp: 2025-09-03T14:26:18.627Z
Learning: In ProcessWrapper.tsx, the useIsRunningProcessNext() hook intentionally uses a non-reactive pattern with queryClient.isMutating and local state instead of useIsMutating hook. This design choice is deliberate and should not be changed to a reactive pattern.
Applied to files:
src/layout/ImageUpload/hooks/useFocusOnChange.tsx
🧬 Code graph analysis (7)
test/e2e/integration/component-library/image-upload.ts (1)
test/e2e/support/apps/component-library/uploadImageAndVerify.ts (1)
makeTestFile(1-6)
src/layout/ImageUpload/hooks/useFocusWhenUploaded.tsx (1)
src/features/attachments/index.ts (2)
UploadedAttachment(20-20)isAttachmentUploaded(29-31)
src/utils/refs/mergeRefs.test.ts (1)
src/utils/refs/mergeRefs.ts (1)
RefsUtils(3-34)
src/layout/ImageUpload/ImageControllers.tsx (1)
src/layout/ImageUpload/hooks/useFocusWhenUploaded.tsx (1)
useFocusWhenUploaded(6-40)
src/app-components/Dropzone/Dropzone.tsx (1)
src/utils/refs/mergeRefs.ts (1)
RefsUtils(3-34)
src/layout/ImageUpload/ImageCropper.tsx (2)
src/layout/ImageUpload/hooks/useFocusOnChange.tsx (1)
useFocusOnChange(4-22)src/layout/ImageUpload/hooks/useFocusWhenRemoved.tsx (1)
useFocusWhenRemoved(4-20)
src/layout/ImageUpload/ImageDropzone.tsx (1)
src/app-components/Dropzone/Dropzone.tsx (1)
IDropzoneProps(16-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install
🔇 Additional comments (16)
src/layout/ImageUpload/hooks/useFocusWhenRemoved.tsx (1)
4-20: LGTM! Clean focus-on-removal implementation.The hook correctly tracks value changes and focuses the element when removed. The
requestAnimationFrameensures focus occurs after DOM updates, and the dependency array is appropriate sinceRefObjecthas stable identity.test/e2e/integration/component-library/image-upload.ts (1)
62-62: LGTM! Improved selector specificity.The change from label-based selection to direct input targeting is more robust and aligns with the component refactor that removed the
Button asChild+Labelpattern.src/layout/ImageUpload/ImageDropzone.tsx (1)
16-19: LGTM! Clean ref forwarding implementation.The new
dropzoneInputRefprop is properly typed and forwarded to theDropzonecomponent, enabling external focus control as intended for the accessibility improvements.Also applies to: 38-38
src/layout/ImageUpload/ImageCropper.tsx (1)
34-34: LGTM! Proper focus management implementation.The new hooks and ref forwarding correctly implement the accessibility requirements:
- Focus returns to the canvas after selecting an image (Line 49)
- Focus returns to the dropzone input after deleting/canceling (Lines 51-53)
- The
dropzoneInputRefis properly created and forwarded (Lines 34, 100)Also applies to: 48-53, 100-100
src/utils/refs/mergeRefs.test.ts (1)
7-107: LGTM! Comprehensive test coverage.The test suite thoroughly validates all scenarios:
- Callback refs and RefObjects
- Multiple refs (mixed types)
- Undefined/null edge cases
- Empty refs array
- Element updates and transitions
src/utils/refs/mergeRefs.ts (1)
3-34: LGTM! Solid ref merging utility.The implementation correctly handles both callback refs and RefObjects. The type cast on Line 23 is necessary and follows the standard pattern for ref utilities, as
RefObject.currentis readonly in its type definition but needs to be mutated here.src/layout/ImageUpload/ImageControllers.tsx (2)
45-48: LGTM! Proper focus management for delete button.The new
deleteButtonRefanduseFocusWhenUploadedhook correctly implement the requirement to focus the delete button after successfully saving an image.
67-69: LGTM! Improved accessibility and removed problematic pattern.The refactored button implementation:
- Removes the problematic
Button asChild+Labelpattern that caused VoiceOver issues (issue #3815, point 3)- Adds proper
aria-labelto the hidden file input (Line 136)- Uses
aria-hiddenon the icon (Line 145)- Implements programmatic file selection via
handleFileSelectClick(Line 67-69)This directly addresses the reported VoiceOver inconsistencies.
Also applies to: 136-147
src/app-components/Dropzone/Dropzone.tsx (3)
9-9: LGTM!Clean import of the new ref merging utility.
28-28: LGTM!The
inputRefprop addition enables external components to control and focus the dropzone input, which is essential for the accessibility improvements in this PR.
98-102: LGTM!The input element correctly spreads
inputPropsfirst, then overrides the ref with thecombinedRef. This ensures the merged ref (combining react-dropzone's internal ref with the externalinputRef) is applied while preserving all other input properties.src/layout/ImageUpload/hooks/useFocusWhenUploaded.tsx (5)
6-11: LGTM!The hook signature and state initialization are well-structured. Using
useReffor tracking the previous attachment ID and initial mount flag is appropriate since these values don't need to trigger re-renders.
13-39: LGTM!The effect logic correctly handles both initial mount (storing the attachment ID without focusing) and subsequent uploads (focusing when a new attachment is uploaded). The separation into
handleInitialMountandhandleAttachmentUploadimproves readability.
46-48: LGTM!The
getAttachmentIdaccessor is straightforward and handles the optional attachment correctly.
50-52: LGTM!The
hasAttachmentIdChangedfunction correctly checks that a new ID exists and differs from the previous one.
54-58: LGTM!Using
requestAnimationFrameto defer the focus call is the right approach here. It ensures the focus occurs after DOM updates and any asynchronous operations (like HTTP uploads) complete, which is critical for the accessibility improvements in this PR.
lassopicasso
left a comment
lassopicasso
left a comment
There was a problem hiding this comment.
Superb 🚀 Also tested it on windows.
|
The automatic backport to The release branch Manual backport required: # Checkout the release branch
git checkout release/v4.22
git pull origin release/v4.22
# Create backport branch
git checkout -b backport/3820
# Cherry-pick the merge commit
git cherry-pick d3cd6ed2d8fcc47a3049c486a8e4999a22ba6275
# Resolve conflicts, then:
git add .
git cherry-pick --continue
# Push and create PR
git push origin backport/3820 |
2 participants
Description
Fixes three WCAG/accessibility issues in the ImageUpload component related to VoiceOver focus management on macOS.
Focus management improvements
Button asChild+LabelpatternNew custom hooks
Created three reusable focus management hooks following React best practices:
useFocusOnChange: Focuses an element when a value changes to a new truthy valueuseFocusWhenUploaded: Focuses an element when an attachment completes uploading (domain-specific)useFocusWhenRemoved: Focuses an element when a value is removed/deleted (truthy → falsy)Technical details
Ref merging
Implemented proper ref merging to combine react-dropzone's internal ref with our custom ref without breaking file dialog functionality.
Async timing
Used
requestAnimationFrameto ensure focus calls happen after DOM updates, accounting for async operations like HTTP uploads.Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
Release Notes
New Features
Accessibility
Refactoring