[CLX-3694][Horizon] Accessibility fixes#3454
Conversation
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Thu, 18 Dec 2025 19:51:52 GMT |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Accessibility Improvements Review
This PR makes significant accessibility improvements to the Horizon UI components. The changes primarily focus on adding proper semantic properties, content descriptions, and improved screen reader support. Overall, this is excellent work that will greatly improve the app's accessibility.
Issues Found
- Extensions.kt:57 -
toggleablefunction'sonClickaction always returnsfalseand doesn't perform actual toggle operation - Extensions.kt:44 - Redundant
liveRegionassignment inselectablefunction (set both conditionally and unconditionally) - ProgressScreen.kt:142 - Incorrect use of
rememberwithvar hasPageChanged- should usemutableStateOffor proper state tracking - DashboardWidgetCard.kt:87 & DashboardCourseSection.kt:161 - Empty
contentDescription = ""usage should be verified as intentional
Positive Highlights
- Comprehensive coverage: Accessibility improvements span across dashboard, module sequences, and various UI components
- Proper semantic properties: Good use of
selectable,toggleable,hideFromAccessibility, and other semantic modifiers - Content descriptions: IconButtons and interactive elements now have appropriate content descriptions
- Consistent patterns: The new
selectableandtoggleableextension functions promote code reusability - Live regions: Proper use of
LiveRegionMode.Assertivefor announcing state changes - Focus management: Smart focus handling in ProgressScreen for improved keyboard/screen reader navigation
- String resources: All new accessibility strings are properly externalized
Additional Observations
- Modifier threading: Great job threading the
modifierparameter through component hierarchies (DashboardAnnouncementBannerWidget, DashboardCourseSection, etc.) - Code cleanup: Removed unused
Spacerimport and cleaned up formatting - Separator accessibility: Nice attention to detail hiding the "|" separators from screen readers in ModuleItemSequenceScreen
Recommendations
- Test with TalkBack extensively to ensure all changes work as expected
- Verify the empty content description patterns don't inadvertently hide important information
- Consider adding accessibility testing to prevent regressions
Great work on improving the accessibility of the Canvas Android app!
| stateDescription = if (toggledOn) onStateDesc else offStateDesc | ||
| liveRegion = LiveRegionMode.Assertive | ||
| onClick(toggleActionLabel) { false } | ||
| } |
There was a problem hiding this comment.
The onClick action in the toggleable function always returns false, which means it won't actually handle the click event. This appears to be a placeholder implementation that should either:
- Return
trueto indicate the action was handled - Be removed if the actual click handling is done elsewhere
Additionally, the toggleActionLabel is defined but the action doesn't actually perform any toggle operation. Consider if this semantic action is needed, or if the toggle handling is done at a higher level in the composable hierarchy.
|
|
||
| @Composable | ||
| private fun BoxScope.ProgressScreenContent(uiState: ProgressScreenUiState) { | ||
| val currentPage = uiState.pages[uiState.currentPosition] |
There was a problem hiding this comment.
There's a logic issue with hasPageChanged. It's declared as a var but remember expects an immutable initial value. This should use remember { mutableStateOf(false) } instead:
var hasPageChanged by remember { mutableStateOf(false) }Or if you want to use a simple boolean:
val hasPageChanged = remember { mutableStateOf(false) }The current implementation may not properly track state changes across recompositions.
| } | ||
| }.conditional(!isLoading) { | ||
| semantics { | ||
| contentDescription = "" |
There was a problem hiding this comment.
Setting contentDescription = "" when not loading may interfere with accessibility. An empty content description can sometimes be problematic for screen readers. Consider using clearAndSetSemantics { } instead if you want to clear all semantics, or verify that an empty string is the intended behavior for this use case.
If the goal is to ensure child elements provide their own content descriptions, this approach is correct. Just wanted to flag for verification.
| if (cardHeight > maxCardHeight) { maxCardHeight = cardHeight } | ||
| } | ||
| .semantics { | ||
| contentDescription = "" |
There was a problem hiding this comment.
Same concern as with DashboardWidgetCard - setting contentDescription = "" may not be the best approach. Verify this is intentional and doesn't hide important information from screen readers.
Additionally, this semantic block is only applied in the pager context when page < pagerState.pageCount. Consider whether this semantic clearing should also apply to other cases or if there's a reason for this conditional application.
| val unselectedStateDesc = context.getString(R.string.a11y_unselected) | ||
|
|
||
| stateDescription = if (selected) selectedStateDesc else unselectedStateDesc | ||
| if (selected) { |
There was a problem hiding this comment.
The liveRegion = LiveRegionMode.Assertive appears twice in this function - once on line 44 (outside the conditional) and once on line 46 (inside the if (selected) block). This is redundant.
If the intention is to always set it to Assertive for selectable items, keep only the one on line 44. If it should only be Assertive when selected, remove line 44 and keep only line 46.
|
Student Install Page |
📊 Code Coverage Reportℹ️ Student
ℹ️ Teacher
ℹ️ Pandautils
|
2 participants
refs: CLX-3694
affects: Student
release note: none