Repository navigation
feat(mobile): audit W3-C accessibility semantics - #5076
Merged
Merged
Conversation
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of 2 changed files: the previous focus-restore timer finding is resolved — reopening now cancels the pending restore via Files Reviewed (2 files, incremental since 9e1976a)
Previous Review Summaries (5 snapshots, latest commit 9e1976a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9e1976a)Status: 1 Issue Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Files Reviewed (8 files, incremental since 6a493b5)
Previous review (commit 6a493b5)Status: No Issues Found | Recommendation: Merge Files Reviewed (23 files, incremental since 77193c6)
Previous review (commit 77193c6)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the changes since the previous review ( Files Reviewed (16 files, incremental)
Previous review (commit fe63077)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the 13 PR-authored commits since the previous review ( Files Reviewed (48 files, incremental)
Previously Reviewed (110 files, verdict unchanged)
Previous review (commit 056f042)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the single new PR commit since the previous full review ( Files Reviewed (1 file, incremental)
Previously Reviewed (110 files, verdict unchanged)
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by kimi-k3 · Input: 53.1K · Output: 3.9K · Cached: 160.9K Review guidance: REVIEW.md from base branch |
Keep both sides in every conflicted file: - markdown-renderer.ts: main's shared linkInteractionProps plus the branch's heading header role and image-only pass-through. - markdown-renderer.test.ts: both the heading-semantics suite and main's link-interaction suite. - markdown-table.test.ts: one tree walker (findFirst/findAll) now serves the branch's table-semantics suite and main's trigger and two-axis modal suite. - tool-card-output-cap.test.ts: identical SelectableText assertion, with main's explanatory comment. - app-root-providers.tsx: both OfflineBanner and ReducedMotionSheetHost. - use-theme-colors.ts: the branch's WCAG AA status hues plus main's warnForeground token. - use-theme-colors.contrast.test.ts: both contrast suites; the warn comment now cites the merged hue. Point main's new part-detail-sheet radio assertions at accessibilityState `checked`, because the branch moved radio items from `selected` to `checked`.
The hand-rolled non-animated option list existed because @expo/react-native-action-sheet animates a fixed 225ms presentation. WCAG 2.3.3 is AAA and scopes non-essential animation; a sheet's own slide-in is essential presentation motion, so the target is met without it. Delete the host component, the mapper, the override hook, and their tests. Revert the ten call sites to useActionSheet. Keep useMotionPolicy for the imperative-scroll callers.
Every form in this branch is disabled-until-valid, so no call site can reach a focus-to-first-invalid boundary. Only the helper's own test used it.
One call site, three static props. The module and its test cost more than the object they built.
One ternary, one call site in the root layout. Also drops the redundant reveal-focus guard ref: the effect cleanup already cancels the pending frame before a later render can schedule its own.
announceForA11y already swallows a native accessibility failure, so the three call-site try/catch layers and the delete toast's try/finally could never fire. Drop them and the tests that forced the impossible throw; announce.test.ts owns that guarantee.
Wrapping the primitive in a plain function to add `alt` cost ref forwarding for all eleven callers, and expo-image already treats `alt` as an alias for accessibilityLabel. Exactly one caller needs a label, so it sets accessible/role/label itself.
The session-list screen already owns the search-input ref, so the module level mirror ref was a second source of truth: a header unmount nulls it through the callback ref, making correctness depend on mount ordering. Pass the ref the screen already has. Inline hasHistoryContent to stay inside the file's max-lines cap.
liveIdsRef already invalidates a removed or reset upload, and ids are UUIDs, so the generation check on the two upload completions could never add a rejection. Keep generationRef only where no id exists yet: a reset during candidate measurement.
ui/SessionRow's `onPress` branch has no caller: both session rows wrap the primitive in their own labelled Pressable. Labelling it pulled an agents-layer helper into components/ui, so drop the label and the import.
Dropping the row opacity left disabled preference and notification rows looking enabled. Mute the title instead: blanket opacity pushed every label under the 4.5:1 text minimum, while the token does not. The Switch still renders its own disabled appearance. Extend the muted-foreground contrast test to `secondary`, the surface these rows actually use.
The row label sat on an invisible accessible sibling while the cells stayed in the accessibility tree, so a screen reader read the whole row and then each cell again. A row now announces exactly one way. A plain row keeps the linear label and hides its cells. A row holding a Markdown link or image exposes its cells and drops the label, so those controls keep their own focus and tap target — an accessible container would shadow them on iOS.
Moving retry to a 28pt corner button kept the 44pt effective target but lost the affordance a failed chip has always had: the whole chip retried. Retry now covers the chip as a sibling of the surface and of Remove, so neither is shadowed for assistive technology, and Remove renders last so it still wins the corner it occupies.
The sheet ran its own two-phase presentation: a 200ms constant hand-synced to Reanimated's 180ms SlideOutDown, chained into a second timer, driving two pieces of visibility state. Editing either duration would clip or strand the exit. `animationType="slide"` makes the platform own enter and exit, so `visible` is the single source of truth. Android still waits out the slide-out before restoring focus, because Modal.onDismiss is iOS-only; that is now one timer with one constant and no coupling to a value in this file.
Collaborator
Author
|
(bot) ## Finding 2 — colour tokens The branch values are:
Security Agent dashboard — light theme:
Destructive delete confirmation — light theme:
Security Agent dashboard — dark theme:
Destructive delete confirmation — dark theme:
The human must decide whether to keep or revert the token retune. Finding 6 — chip retry targetNo valid retryable failed attachment chip capture was possible on iOS. The local flow opened both attachment pickers, but the available forced 22 MB PDF failure was rejected before upload as a terminal client-side size error, so no Retry chip rendered. No product code changed.
The Finding 6 screenshot set is missing because the retryable failed-chip state could not be produced. This is an evidence blocker, not a pass. |
…ics-1ab3 # Conflicts: # apps/mobile/src/components/agents/attachment-preview-strip.tsx # apps/mobile/src/components/agents/markdown-renderer.test.ts # apps/mobile/src/components/pr-review/discussion/reaction-picker-sheet.tsx # apps/mobile/src/components/pr-review/discussion/reactions-row.tsx # apps/mobile/src/components/voice-input-control.tsx
eshurakov
approved these changes
Aug 17, 2026
Cut duplicated WCAG helpers, tests that assert test-local arithmetic or JSX pass-through, and props the composed accessibility label overwrites. Behavior is unchanged. - Move the diff-token contrast block next to the palette it paints on, so the WCAG math lives in one file. - Drop the mounting harness around `!useReducedMotion()` and the unread `reduceMotion` field. - Delete duplicate radio, table-row, and reaction-sheet tests. - Collapse the three-branch chip class names to pairs.
jeanduplessis
approved these changes
Aug 17, 2026
The reaction picker held three refs, a callback guard, a platform branch, and an `onDismiss` prop to move focus back to one button. The parent owns the trigger, so it owns the focus restore: one timer, no props. Prune the mounted suites to the accessibility contract. Assertions on Tailwind classes, sibling order, element geometry, and JSX pass-through went out; role, label, state, hidden, and focus assertions stayed.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Reopening the reaction picker inside the restore window let the stale timer move focus to the trigger behind the sheet. Hold the handle and clear it on open. Raise the allowance to 400ms. The slide-out runs ~300ms, so a slow frame could land the focus call while the Modal is still presented, where it silently does nothing.
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
3 participants
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.
The user: Mobile controls, forms, Markdown, uploads, reduced-motion sheets, and deletion flows expose clearer screen-reader semantics and focus behavior.
The product manager: Users receive labeled choices, readable disabled states, upload progress and outcomes, structured Markdown tables, reduced-motion action sheets, and reliable focus after dismissals and deletions.
The maintainer: Shared accessibility contracts cover status announcements, radio groups, form labels, image alt policy, motion policy, hidden routes, Modal focus lifecycle, stable removal anchors, and upload state semantics.
Human steps: No human step is needed before or after merge.
Visual Changes: N/A.
E2E: bot-e2e — runtime verification was blocked by the Android CLI environment after the bundle started; unit and mounted tests are the evidence.
CI note: A main-line regression in the selectable Markdown error assertion was fixed in this branch because the merge ref inherited the failing main test.
Out of scope: KiloChat and KiloClaw files remain unchanged per the approved plan.