fix: improve label association in omnichannel unit fields#38738
fix: improve label association in omnichannel unit fields#38738TheRazorbill wants to merge 4 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 |
WalkthroughAdds an aria-labelledby prop to the unit autocomplete and wires explicit label IDs with aria-labelledby on offline message channel and unit controls in the omnichannel department form to improve accessibility (no behavioral changes). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/views/omnichannel/departments/EditDepartment.tsx`:
- Line 128: The native TextInput's aria-labelledby (constructed via
emailFieldLabelId) is redundant because FieldLabel already provides htmlFor/id
association; remove the aria-labelledby attribute from TextInput usage (e.g.,
where emailFieldLabelId is created and passed) and keep aria-labelledby only on
custom components that require it (PaginatedSelectFiltered, AutoCompleteUnit).
Update occurrences referenced by symbols FieldLabel, TextInput,
emailFieldLabelId and ensure other native inputs (Name, Description) follow the
same pattern so only non-native/custom controls retain aria-labelledby.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/omnichannel/departments/EditDepartment.tsx`: - Line 128: The native TextInput's aria-labelledby (constructed via emailFieldLabelId) is redundant because FieldLabel already provides htmlFor/id association; remove the aria-labelledby attribute from TextInput usage (e.g., where emailFieldLabelId is created and passed) and keep aria-labelledby only on custom components that require it (PaginatedSelectFiltered, AutoCompleteUnit). Update occurrences referenced by symbols FieldLabel, TextInput, emailFieldLabelId and ensure other native inputs (Name, Description) follow the same pattern so only non-native/custom controls retain aria-labelledby.apps/meteor/client/views/omnichannel/departments/EditDepartment.tsx (1)
128-128: Redundant labeling on the nativeTextInputfor Email.The
<FieldLabel htmlFor={emailField}>already creates a native<label>→<input>association via the matchingid={emailField}onTextInput. Addingaria-labelledbyon top is redundant for native inputs — screen readers will announce the label either way. It's not harmful, but it's inconsistent with the other native inputs in this form (e.g., Name, Description) that rely solely onhtmlFor.Consider keeping
aria-labelledbyonly for the custom select components (PaginatedSelectFiltered,AutoCompleteUnit) wherehtmlFormay not reach the actual focusable element, and dropping it from the nativeTextInput.Also applies to: 199-199, 203-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/omnichannel/departments/EditDepartment.tsx` at line 128, The native TextInput's aria-labelledby (constructed via emailFieldLabelId) is redundant because FieldLabel already provides htmlFor/id association; remove the aria-labelledby attribute from TextInput usage (e.g., where emailFieldLabelId is created and passed) and keep aria-labelledby only on custom components that require it (PaginatedSelectFiltered, AutoCompleteUnit). Update occurrences referenced by symbols FieldLabel, TextInput, emailFieldLabelId and ensure other native inputs (Name, Description) follow the same pattern so only non-native/custom controls retain aria-labelledby.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsxapps/meteor/client/views/omnichannel/departments/EditDepartment.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsxapps/meteor/client/views/omnichannel/departments/EditDepartment.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR `#36972` through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
📚 Learning: 2025-09-18T17:32:33.969Z
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR `#36972` through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
Applied to files:
apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsxapps/meteor/client/views/omnichannel/departments/EditDepartment.tsx
⏰ 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: cubic · AI code reviewer
- GitHub Check: Agent
🔇 Additional comments (3)
apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsx (1)
29-29: Clean prop forwarding foraria-labelledby.The destructuring alias and forwarding to
PaginatedSelectFilteredare correct. The prop is already covered by theComponentProps<typeof PaginatedSelectFiltered>base type, so no additional type declaration is needed.Also applies to: 47-47
apps/meteor/client/views/omnichannel/departments/EditDepartment.tsx (2)
229-231: Good use ofaria-labelledbyfor the custom select component.
PaginatedSelectFilteredlikely renders a non-native select wherehtmlForalone won't create a proper label association. The explicitaria-labelledbylinkage is the right approach here.Also applies to: 237-248
352-352: Correctaria-labelledbylinkage forAutoCompleteUnit.The label ID is properly generated, assigned to the
FieldLabel, and passed through to theAutoCompleteUnitwhich forwards it to the underlyingPaginatedSelectFiltered. This completes the accessibility chain for the unit field.Also applies to: 361-367
There was a problem hiding this comment.
Pull request overview
Improves accessibility of Omnichannel department edit fields by explicitly associating composite inputs with their visible labels via aria-labelledby, removing the previous aria-label workaround on the Unit autocomplete.
Changes:
- Add label element IDs and wire Email / Offline Message Channel / Unit inputs with
aria-labelledbyin the Edit Department form - Remove the temporary
aria-labelfromAutoCompleteUnitand support passingaria-labelledbythrough
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/meteor/client/views/omnichannel/departments/EditDepartment.tsx | Adds label IDs and connects related inputs to their labels via aria-labelledby. |
| apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsx | Switches from hardcoded aria-label to using aria-labelledby passed in from the parent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsx
Show resolved
Hide resolved
apps/meteor/client/views/omnichannel/additionalForms/AutoCompleteUnit.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/meteor/client/views/omnichannel/departments/EditDepartment.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/omnichannel/departments/EditDepartment.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Proposed changes (including videos or screenshots)
aria-labelledbyaria-labelworkaround from AutoCompleteUnitIssue(s)
Steps to test or reproduce
aria-labelledbypointing to the label textFurther comments
htmlFor/idassociation, so no changes were needed thereTests
Summary by CodeRabbit