Improve Giphy preview TalkBack accessibility#6483
Open
andremion wants to merge 5 commits into
Open
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Contributor
SDK Size Comparison 📏
|
WalkthroughThis PR enhances Giphy attachments with an interactive control flag and improves accessibility for the message preview flow. The ChangesGiphy interactive parameter and accessibility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 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
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/GiphyMessageContent.kt`:
- Around line 84-100: The current LaunchedEffect(message.id) always delays and
calls previewFocusRequester.requestFocus() and the preview node is always
.focusable(), which steals focus even when TalkBack is off; modify the logic to
check accessibility touch exploration before applying focus behavior: obtain
AccessibilityManager and check isTouchExplorationEnabled (or equivalent), wrap
the .focusRequester(previewFocusRequester) and .focusable() modifiers behind
that condition on the preview composable, and only call
previewFocusRequester.requestFocus() within LaunchedEffect when
isTouchExplorationEnabled is true (still keep the existing delay using
PreviewFocusRequestDelayMs); ensure you reference previewFocusRequester,
LaunchedEffect(message.id), PreviewFocusRequestDelayMs, focusRequester,
focusable(), and requestFocus() when making the change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0271b33b-153c-45a7-a0bf-89040710fd4a
📒 Files selected for processing (13)
stream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/GiphyAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/GiphyMessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactory.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactoryParams.ktstream-chat-android-compose/src/main/res/values-es/strings.xmlstream-chat-android-compose/src/main/res/values-fr/strings.xmlstream-chat-android-compose/src/main/res/values-hi/strings.xmlstream-chat-android-compose/src/main/res/values-in/strings.xmlstream-chat-android-compose/src/main/res/values-it/strings.xmlstream-chat-android-compose/src/main/res/values-ja/strings.xmlstream-chat-android-compose/src/main/res/values-ko/strings.xmlstream-chat-android-compose/src/main/res/values/strings.xml
andremion
force-pushed
the
fix/compose-giphy-preview-a11y
branch
2 times, most recently
from
e696962 to
c5f6111
Compare
andremion
force-pushed
the
fix/compose-giphy-preview-a11y
branch
from
365016a to
9f6169e
Compare
The Giphy image had `contentDescription = null`, so TalkBack only announced the "Giphy" overlay badge on focus. Set the image's `contentDescription` to the attachment title (when non-blank), so screen-reader users hear the search query together with the badge, e.g. "Hello, giphy". When the title is blank or absent, falls back to the existing "Giphy" announcement. Improves both the ephemeral preview path and sent Giphy messages.
After a `/giphy` command, the ephemeral preview rendered in the
message list but TalkBack focus stayed on the composer. Screen-
reader users had no signal the preview arrived.
Wrap the visible-to-you row + the Giphy image in a focusable
`Column` with `Modifier.semantics(mergeDescendants = true) {}`,
and request focus on it 100 ms after the message id is first
composed. Mirrors the precedent in
`MessageComposerAudioRecordingLockedContent`.
Adds `interactive: Boolean = true` to `GiphyAttachmentContent`
and `GiphyAttachmentContentParams`. The ephemeral preview passes
`false`, which skips the inner `combinedClickable` — both its
handlers are no-ops in the ephemeral case, and the inner
clickable was a nested accessibility merge boundary that kept
the image alt and "Giphy" badge from merging into the bubble's
focus.
TalkBack now announces the bubble as one focus, e.g. "Only
visible to you, Hello, Giphy".
After tapping Send / Shuffle / Cancel on the ephemeral Giphy preview, screen-reader users got no confirmation that their action took effect — focus drifted depending on TalkBack's fallback-recovery race after the bubble unmounted. Wrap each action button's onClick to fire a polite live region announce before forwarding to `onGiphyActionClick`: - `SendGiphy` → "Giphy sent" - `ShuffleGiphy` → "Giphy shuffled" - `CancelGiphy` → "Giphy preview cancelled" The announce queues before the state change, so users hear the confirmation regardless of where TalkBack focus lands afterward. New strings translated across the 7 supported locales.
The new `GiphyMessageContent`, the `interactive = false` branch in `GiphyAttachmentContent`, and the `attachment.title.takeIf(String::isNotBlank)` null / blank paths had no test exercising them. Parameterize the existing `internal fun GiphyAttachmentContent()` preview helper to accept `interactive` and `title` with backward-compatible defaults, and add a sibling `internal fun GiphyMessageContent()` helper following the same fixture-data pattern. Add three Paparazzi snapshot tests in `AttachmentsContentTest`: - `giphy attachment content non-interactive` covers the `applyIf(interactive)` false branch. - `giphy attachment content blank title` covers the `takeIf(String::isNotBlank)` null path. - `giphy message content` covers `GiphyMessageContent`'s static rendering plus the new factory forwarding in `ChatComponentFactory.GiphyAttachmentContent` and the new `interactive` field in `GiphyAttachmentContentParams`. Runtime-only paths (`LaunchedEffect`, `requestFocus()`, accessibility listener callbacks, button `onClick` lambdas) remain uncovered — they require UI tests beyond snapshot scope.
andremion
force-pushed
the
fix/compose-giphy-preview-a11y
branch
from
23d1e65 to
03801fc
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AND-1180
Goal
The ephemeral Giphy preview (rendered after a
/giphycommand) had three accessibility gaps:contentDescription = null, the innercombinedClickablecreated a nested accessibility merge boundary, and the visible-to-you row was a sibling focus.Implementation
Three feature-scoped commits.
1. Name the Giphy image for TalkBack.
Set
contentDescriptionon the innerStreamAsyncImageinGiphyAttachmentContenttoattachment.title?.takeIf(String::isNotBlank). When the attachment has a non-blank title (typically the search query supplied by the Giphy API), TalkBack reads it together with the existing "Giphy" badge — e.g. "Hello, giphy". Improves both the ephemeral preview path and sent Giphy messages.2. Focus the preview bubble on appearance.
Wrap the visible-to-you row + Giphy image in
GiphyMessageContentin a focusableColumnwithModifier.semantics(mergeDescendants = true) {}, andrequestFocus()on it 100 ms after the message id is composed. Mirrors the precedent inMessageComposerAudioRecordingLockedContent.Adds
interactive: Boolean = truetoGiphyAttachmentContentandGiphyAttachmentContentParams. The ephemeral preview passesfalse, which skips the innercombinedClickable— its handlers are no-ops in the ephemeral case, and the inner clickable was a nested accessibility merge boundary that prevented the image alt and "Giphy" badge from merging into the bubble's focus.TalkBack now announces the bubble as one focus, e.g. "Only visible to you, Hello, Giphy".
Note
The
interactiveworkaround. The cleaner design is to makeonItemClick(andAttachmentState.onLongItemClick) nullable and gatecombinedClickableonhandler != null, which removes the need for a separate flag. That's a binary-breaking type change on a published API, so this PR takes the additive route. An inline comment inGiphyAttachmentContent.ktmarks the spot; v8 should refactor to nullable handlers and dropinteractive.3. Announce action state changes.
Wrap each Send / Shuffle / Cancel button's
onClickto fireview.announceForAccessibilitybefore forwarding toonGiphyActionClick:SendGiphy→ "Giphy sent"ShuffleGiphy→ "Giphy shuffled"CancelGiphy→ "Giphy preview cancelled"The announce queues before the state change, so users hear the confirmation regardless of where TalkBack focus lands afterward (the focus drift itself is a TalkBack fallback-recovery race after the bubble unmounts, not deterministically fixable from this composable).
New strings translated across the 7 supported locales. No public API breaking changes;
interactiveis an additive defaulted param.Testing
Enable TalkBack on a physical device. Run the Compose sample.
/giphy hello, then submit the command./giphy welcome. On the preview, swipe to Shuffle and double-tap./giphy something. On the preview, swipe to Cancel and double-tap.Summary by CodeRabbit
New Features
Localization