fix: WCAG (3.3.1) Add inline error for UserAvatarEditor's URL input#36656
fix: WCAG (3.3.1) Add inline error for UserAvatarEditor's URL input#36656ergot-rp wants to merge 3 commits intoRocketChat:developfrom
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36656 +/- ##
===========================================
- Coverage 70.55% 70.51% -0.04%
===========================================
Files 3271 3271
Lines 116782 116793 +11
Branches 21090 21047 -43
===========================================
- Hits 82393 82360 -33
- Misses 32338 32374 +36
- Partials 2051 2059 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
gabriellsh
changed the title
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
Outdated
Show resolved
Hide resolved
| const handleAvatarFromUrlChange = (event: ChangeEvent<HTMLInputElement>): void => { | ||
| setAvatarFromUrl(event.currentTarget.value); | ||
| const { value } = event.currentTarget; | ||
| setAvatarFromUrl(value); | ||
| if (isURL(value) || !value) { | ||
| setAvatarUrlError(undefined); | ||
| } else { | ||
| setAvatarUrlError(t('error-invalid-image-url')); | ||
| } |
There was a problem hiding this comment.
I'm not sure if we should compute this error every time the user types something. The avatar will only change when clicking Add URL button, which invokes handleAddUrl. Once that happens, the URL that the avatar renders will change, and if the image fails to load, it'll trigger the onError callback passed to UserAvatar.
Do you think it would be enoough to rely on that error from UserAvatar? Otherwise, if you still want to manually check the validity of the URL, I usually use this pattern:
try {
const url = new URL(myUrlString);
} catch {
// URL is invalid
}
So in summary:
- We probably don't need an error update for every keystroke
- We could rely on the UserAvatar
onErrorcallback, if it's enough for this case isUrlshould no be used to validate a URL (I know, terrible helper name)
WalkthroughThe UserAvatarEditor component now includes URL validation for avatar inputs. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant Validator
participant UI
User->>Component: Enter avatar URL
Component->>Component: Clear avatarUrlError
Component->>UI: Update input display
User->>Component: Click "Add URL"
Component->>Validator: Validate URL with isUrl()
alt URL is valid
Validator-->>Component: ✓ Valid
Component->>Component: Set avatar from URL
Component->>UI: Show avatar
else URL is invalid
Validator-->>Component: ✗ Invalid
Component->>Component: Set avatarUrlError
Component->>UI: Render FieldError
Component->>UI: Disable Add button
end
User->>Component: Edit URL input
Component->>Component: Clear avatarUrlError
Component->>UI: Enable Add button
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Hi @ergot-rp looks like you used another account for one of the commits: https://github.com/ergotse Do you mind signing the CLA with that account? |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx (1)
130-143: Add ARIA attributes to TextInput for proper error association.For WCAG compliance, the TextInput should have
aria-describedbyandaria-invalidattributes when an error is present. This ensures screen reader users are properly informed about validation errors.Apply this diff to add the required ARIA attributes:
<TextInput data-qa-id='UserAvatarEditorLink' id={imageUrlField} flexGrow={0} placeholder={t('Use_url_for_avatar')} value={avatarFromUrl} mis={4} onChange={handleAvatarFromUrlChange} + aria-invalid={!!avatarUrlError} + aria-describedby={avatarUrlError ? `${imageUrlField}-error` : undefined} /> {avatarUrlError && ( <FieldError aria-live='assertive' id={`${imageUrlField}-error`}> {avatarUrlError} </FieldError> )}
🧹 Nitpick comments (1)
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx (1)
108-108: Consider the error message clarity.The same error message "Invalid URL" will be displayed both when the URL format is invalid and when a valid URL fails to load the image. This might be slightly confusing to users, but if this aligns with the design requirements, it's acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx(8 hunks)
⏰ 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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx (5)
2-2: LGTM!The
FieldErrorimport is correctly added and used for displaying validation errors.
41-41: LGTM!The error state is properly typed and initialized.
61-68: LGTM!The validation logic correctly checks the URL before setting the avatar and provides appropriate error feedback. This addresses the reviewer's previous feedback.
77-83: LGTM!Clearing the error when the user modifies the input provides good UX, allowing them to correct their mistakes.
119-119: LGTM!The disabled logic correctly prevents submission when there's no URL or when a validation error is present.
| function isUrl(myUrlString: string) { | ||
| try { | ||
| new URL(myUrlString); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add protocol validation to prevent potential security issues.
The URL validation accepts any valid URL protocol, including potentially unsafe ones like javascript: or file:. For avatar URLs, you should restrict to safe protocols only.
Apply this diff to add protocol validation:
function isUrl(myUrlString: string) {
try {
- new URL(myUrlString);
- return true;
+ const url = new URL(myUrlString);
+ return ['https:', 'http:', 'data:'].includes(url.protocol);
} catch {
return false;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isUrl(myUrlString: string) { | |
| try { | |
| new URL(myUrlString); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| function isUrl(myUrlString: string) { | |
| try { | |
| const url = new URL(myUrlString); | |
| return ['https:', 'http:', 'data:'].includes(url.protocol); | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/client/components/avatar/UserAvatarEditor/UserAvatarEditor.tsx
around lines 24-31, the isUrl function currently accepts any URL protocol;
update it to only allow safe protocols (at minimum 'http:' and 'https:') and
optionally allow 'data:' for inline images. Keep the try/catch URL parsing, then
check url.protocol against an allowed set (normalize to lowercase); if allowing
'data:' ensure the pathname or href indicates an image MIME type (e.g.,
data:image/) before returning true; otherwise return false.
juliajforesti
force-pushed
the
192-profile
branch
from
e64161a to
0b3b46c
Compare
5 participants
Inline errors on profile page, more information in Figma https://www.figma.com/design/eCqiUBG2zmN7C0HrFxc7ji/Inline-errors-for-web-forms?node-id=50-2235&t=Qc1iBq6EYwRBeec6-0
URL for avatar: Invalid URL
Name: Name required
Username: Username required
Email: Email required
Summary by CodeRabbit