feat(a11y): Message actions announcement and back focus - #7229
Conversation
|
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:
WalkthroughAdds accessibility focus tracking and restoration for message action sheets: shared last-focused message ref, message accessibility label/action utilities and tests, Touch/Handle ref forwarding, action-sheet header focus on present, and focus restoration on dismiss. ChangesAccessibility Focus Restoration Workflow
Sequence DiagramsequenceDiagram
participant User
participant MessageTouchable
participant lastFocusedRef as lastFocusedMessageRef
participant MessageActions
participant ActionSheet
participant Handle
participant AccessibilityInfo
User->>MessageTouchable: Long press / accessibility action
MessageTouchable->>lastFocusedRef: markAsLastFocused()
MessageTouchable->>MessageActions: open message actions
MessageActions->>ActionSheet: showActionSheet(onClose)
ActionSheet->>ActionSheet: onDidPresent
ActionSheet->>Handle: focusHandle -> findNodeHandle(handleRef)
Handle->>AccessibilityInfo: setAccessibilityFocus(header) (defer 300ms on Android)
User->>ActionSheet: Dismiss
MessageActions->>lastFocusedRef: get() -> messageRef
MessageActions->>AccessibilityInfo: setAccessibilityFocus(messageRef) (after animation)
MessageActions->>lastFocusedRef: clear()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
|
Android Build Available Rocket.Chat Experimental 4.72.0.108617 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNR-q4kPySm10tUnMpVhAS_0xW-fB4K8AaSET3MuDoXjn9hXgrmFsio_VEihbYHT191fB6aC8vRNjhGD5Ibp |
|
iOS Build Available Rocket.Chat Experimental 4.72.0.108618 |
…of replied-on-thread indicator
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.maestro/tests/e2ee/flows/verify-message-unread.yaml (1)
16-31: ⚡ Quick winConsider unifying unread verification to use ID-based selector on both platforms.
Android uses the
read-receipt-unreadtestID while iOS uses a text pattern match (.*Message was not read.*). The text pattern is fragile since "Message was not read" is sourced from i18n translations (Message_was_not_readinen.json) and will break if the string is translated or modified. Since theread-receipt-unreadtestID is defined in theReadReceiptcomponent and available for both platforms, verify whether iOS can use it instead of the text pattern for consistency and resilience.🤖 Prompt for 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. In @.maestro/tests/e2ee/flows/verify-message-unread.yaml around lines 16 - 31, Unify the unread verification to use the ID-based selector on both platforms by replacing the iOS text regex check with the same id selector used for Android: update the iOS runFlow block that calls extendedWaitUntil so it looks for id: 'read-receipt-unread' (the testID defined in the ReadReceipt component) with the same timeout; ensure the runFlow entries for platform: iOS and platform: Android both use extendedWaitUntil -> visible -> id: 'read-receipt-unread' to make the check resilient to i18n changes.
🤖 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 @.maestro/tests/e2ee/flows/verify-message-unread.yaml:
- Around line 28-31: The YAML uses a fragile text-regex matcher in the
extendedWaitUntil step (visible: text: '.*Message was not read.*'); replace this
with an ID-based selector or accessibility identifier for the unread receipt
element (e.g., use the iOS accessibilityIdentifier/testID exposed for the unread
receipt) so the step targets the element by stable id rather than localized
text; update the extendedWaitUntil visible selector to use the id/accessibility
selector form your test framework supports (or an iOS predicate using
accessibilityIdentifier) and fall back to a less fragile locator only if the id
is unavailable.
---
Nitpick comments:
In @.maestro/tests/e2ee/flows/verify-message-unread.yaml:
- Around line 16-31: Unify the unread verification to use the ID-based selector
on both platforms by replacing the iOS text regex check with the same id
selector used for Android: update the iOS runFlow block that calls
extendedWaitUntil so it looks for id: 'read-receipt-unread' (the testID defined
in the ReadReceipt component) with the same timeout; ensure the runFlow entries
for platform: iOS and platform: Android both use extendedWaitUntil -> visible ->
id: 'read-receipt-unread' to make the check resilient to i18n changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77aa5ef0-32ce-4187-856e-949df63142f3
⛔ Files ignored due to path filters (1)
app/containers/message/__snapshots__/Message.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
.maestro/tests/e2ee/flows/verify-message-unread.yaml.maestro/tests/room/threads.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .maestro/tests/room/threads.yaml
📜 Review details
⏰ 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: E2E Run iOS (12) / ios-test
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-05T14:28:10.004Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6997
File: .maestro/tests/room/message-markdown-click.yaml:28-39
Timestamp: 2026-03-05T14:28:10.004Z
Learning: In Maestro YAML selector fields (text, id) within the Rocket.Chat React Native repository, use the contains pattern '.*keyword.*' (leading and trailing '.*') for matching text. The pattern '.*keyword*.' is incorrect and will fail to match cases where the keyword appears at the end of the element's text. This guideline applies to all Maestro YAML selector fields across the codebase.
Applied to files:
.maestro/tests/e2ee/flows/verify-message-unread.yaml
🔇 Additional comments (1)
.maestro/tests/e2ee/flows/verify-message-unread.yaml (1)
16-31: Clarify the connection to PR objectives.This change updates the unread message verification flow with platform-specific checks. However, the PR objectives focus on action sheet accessibility (handle accessibility, focus restoration).
Could you clarify how the action sheet accessibility changes necessitated this update to the unread receipt verification? Understanding the connection will help verify this change is complete and correct.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
diegolmello
left a comment
There was a problem hiding this comment.
A few items from review that I'd like resolved before merge:
🔴 Required: i18n keys only translated to English
en.json adds four new keys but every one of the 24 non-English locales adds only two:
| Key | en | other locales |
|---|---|---|
A11y_close_action_sheet |
✅ | ✅ |
A11y_press_to_view_thread |
✅ | ✅ |
A11y_close_action_sheet_hint |
✅ | ❌ |
Show_message_actions |
✅ | ❌ |
The PR body claims "Localized labels and hints for dismissing action sheets and showing message actions across many languages" — but a Spanish/German/Japanese screen-reader user will currently hear "Dismisses the action sheet" and "Show message actions" in English (i18n-js falls back to the default locale). Either translate the missing two keys across all 24 locales, or trim the claim from the PR description.
🟡 Important: Dead IMessageA11y plumbing + duplicate useMessageAccessibilityLabel call
In app/containers/message/Message.tsx:
IMessageA11y(lines 107-111) declaresaccessibilityActions,onAccessibilityAction, andhandleLongPress, but none of the three are read anywhere insideMessage.MessageTouchablepasseshandleLongPress={!isDisabled ? handleLongPress : undefined}at line 223; the child silently discards it. Misleading for future readers, and it also defeatsReact.memo(fresh closure every render).useMessageAccessibilityLabel(props)is called in bothMessage(line 112) andMessageTouchable(line 179). The work involvesi18n.t(...),toLocaleTimeString(), and mention/channel string substitution — running twice per message render. Only theMessageTouchableinstance feeds the outerTouch;Messageonly uses its copy in the early-return path for thread reply/sequential/info/ignored.
Suggest dropping the unused interface fields and computing the label once in the parent.
🟡 Important: Verify duplicate a11y props on Touch outer + inner
Touch.tsx:85-110 now forwards accessible, accessibilityRole, accessibilityLabel, accessibilityHint, accessibilityActions, and onAccessibilityAction to both the outer KeyboardComponent and the inner View. The LoadMore.test.tsx.snap diff confirms the duplication. Two sibling accessible nodes with identical labels typically result in double announcements on iOS VoiceOver and can confuse swipe navigation on TalkBack.
Could you confirm this was tested end-to-end with VoiceOver and TalkBack on a long-pressable message and share what's announced? If the inner View no longer needs accessible, dropping it would also explain (and possibly let you revert) the Maestro iOS testID workaround in verify-message-unread.yaml — that change looks like a side effect of the outer Touch absorbing nested testIDs on iOS.
Happy to pair on the dual-label investigation if useful.
Proposed changes
Improve the actionSheet behavior on a11y.
open message actionsis available on a11y actions;Issue(s)
https://rocketchat.atlassian.net/browse/MA-263
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Accessibility Improvements
New Features
Tests