[CLX-3537][Horizon] Inbox a11y #3443
Conversation
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Mon, 15 Dec 2025 09:50:08 GMT |
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
|
Student Install Page |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Update Review - Accessibility Improvements
I've reviewed the latest changes to this PR. This update includes comprehensive accessibility improvements across the Horizon UI library.
Summary of Changes
Accessibility Enhancements: ✅
- Added proper semantics for dropdown components (SingleSelect, MultiSelect, SingleSelectImage)
- Improved content descriptions for interactive elements (FileDrop actions, icon buttons)
- Enhanced Tag component with better dismissible action semantics
- Added TalkBack detection for InputDropDownPopup focus handling
UI/Layout Changes:
- Refactored inbox details message list layout approach
- Moved reply content positioning
- Updated message ordering logic in ViewModel
Issues Requiring Attention
I've added inline comments on 4 areas that need careful review:
- Inbox Details Layout Change (HorizonInboxDetailsScreen.kt:236) - Changed from
reverseLayouttoArrangement.Bottom, which significantly alters scroll behavior - Reply Content Positioning (HorizonInboxDetailsScreen.kt:237) - Moved from sticky header to separate component
- Message Ordering (HorizonInboxDetailsViewModel.kt:137) - Added
.reversed()that interacts with layout changes - TalkBack-dependent Focus Logic (InputDropDownPopup.kt:99) - Consider if popup should always be focusable
What's Working Well
- ✅ Excellent use of semantic modifiers throughout
- ✅ Proper role-based accessibility (Role.DropdownList)
- ✅ Good separation of visual and semantic content
- ✅ Consistent approach to preventing duplicate announcements
- ✅ Well-organized string resources for a11y
Testing Recommendations
Critical:
- Test inbox message ordering with initial load, pagination, and new replies
- Verify scroll position behavior with keyboard interactions
- Test reply UI visibility during message scrolling
Important:
- Verify all dropdown components with TalkBack enabled
- Test Tag dismissal actions with screen readers
- Validate InputDropDownPopup with various accessibility services (TalkBack, Switch Access, Voice Access)
Overall Assessment
The accessibility improvements are well-implemented and follow Compose best practices. However, the inbox details refactoring represents a significant behavioral change that warrants thorough testing before merge. Please ensure the inline comments are addressed, particularly around the layout and ordering logic.
| } | ||
| } | ||
| if (state.replyState != null) { | ||
| HorizonInboxReplyContent(state.replyState) |
There was a problem hiding this comment.
This is a significant behavioral change from the previous reverseLayout approach.
Concerns:
reverseLayout = truewithLazyColumnis the standard pattern for bottom-anchored lists (chat-style UIs) because it naturally handles scroll position when items are added/removed- The new approach using
Arrangement.Bottomwith manual scroll state initialization may not handle dynamic content changes as gracefully - When new messages are added via pagination or when the keyboard appears, the scroll position might jump unexpectedly
Recommendation:
If the goal is to fix accessibility traversal order, consider using reverseLayout with Modifier.semantics { isTraversalGroup = true; traversalIndex = ... } instead of completely changing the layout approach. This would maintain the proven scroll behavior while fixing accessibility.
Please ensure this has been thoroughly tested with:
- Initial message load
- Pagination (loading older messages)
- Keyboard appearance/dismissal
- Reply submission with immediate message addition
| } | ||
| if (state.replyState != null) { | ||
| HorizonInboxReplyContent(state.replyState) | ||
| } |
There was a problem hiding this comment.
Moving the reply content from stickyHeader to outside the LazyColumn fundamentally changes the UX.
Previous behavior: Reply UI stayed visible at the top/bottom while scrolling through messages (sticky header behavior)
New behavior: Reply UI is now a separate component after the scrollable list
Questions:
- Was the sticky header causing accessibility issues?
- Is the reply UI supposed to always be visible, or should it scroll with content?
- How does this interact with the keyboard when replying?
If the sticky header was causing problems, consider using a Column with the reply UI and using Modifier.weight(1f) on the LazyColumn to achieve similar results while maintaining the always-visible reply behavior.
| } | ||
| ) | ||
| }, | ||
| }.reversed(), |
There was a problem hiding this comment.
This reversal combined with the layout changes needs careful verification.
Question: With the new Arrangement.Bottom approach and this .reversed(), are messages now displayed in the correct chronological order?
Testing needed:
- Verify message order matches expected chronological ordering
- Test that pagination loads older messages in the correct position
- Ensure new message submission appears in the right place
The interaction between .reversed() here, the changed pagination order at line 395, and the new layout arrangement needs integration testing to ensure correctness.
| verticalOffsetPx + SpaceSize.SPACE_8.value.toPx | ||
| ), | ||
| onDismissRequest = { onMenuOpenChanged(false) }, | ||
| properties = PopupProperties( |
There was a problem hiding this comment.
While detecting TalkBack for focus behavior is functional, consider the architectural implications:
Current approach: Checking accessibility manager state to alter popup behavior
Potential issues:
- Creates dependency on runtime accessibility state
- May cause inconsistent behavior if users enable TalkBack after the popup is created
- The popup should ideally work correctly regardless of accessibility configuration
Suggestion:
Consider making the popup always focusable when isMenuOpen is true, which should work correctly for both regular and TalkBack users. If there's a specific reason why it must be non-focusable for sighted users, document it clearly.
properties = PopupProperties(
focusable = visibleState.targetState && isFocusable
)If the TalkBack check is necessary for a specific behavior, please add a comment explaining why.
andrasmaczak
left a comment
andrasmaczak
left a comment
There was a problem hiding this comment.
Findings:
- Unread items should be announced as Unread
- On Create Message screen the close button's description should be fixed
- On Create Message screen the attach file dialog's close button's description should be fixed
- The 'Send an individual message to each recipient' label and checkbox should be merged
refs: CLX-3537
affects: Student
release note: none