draft: Migrate expansion tile to live region #174598
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request migrates ExpansionTile to use a Semantics live region for accessibility announcements instead of SemanticsService.announce. This is achieved by wrapping the tile in a Semantics widget with liveRegion: true and commenting out the explicit announcement calls.
My review focuses on the correctness and maintainability of these changes. I've identified a potentially brittle magic string check in the Android accessibility bridge that should be clarified or removed. I also have a question about a change to the semantics hint on iOS/macOS, which might affect how the tile's state is announced on focus.
| if (label != null && label.startsWith("T")) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This condition seems brittle as it relies on a magic string "T". Could you add a comment explaining why this check is necessary and what kind of labels it's intended to match? If this is for temporary testing, it should be removed before merging. Relying on string prefixes can lead to maintainability issues and unintended side effects if labels change in the future.
| semanticsHint = _tileController.isExpanded | ||
| ? '${localizations.collapsedHint}\n ${localizations.expansionTileExpandedHint}' | ||
| : '${localizations.expandedHint}\n ${localizations.expansionTileCollapsedHint}'; | ||
| ? localizations.expansionTileExpandedHint | ||
| : localizations.expansionTileCollapsedHint; |
There was a problem hiding this comment.
By removing the state (collapsedHint/expandedHint) from semanticsHint, the state of the ExpansionTile (expanded or collapsed) might not be announced when the user focuses on it. The new liveRegion will announce the state when it changes, but not on initial focus. Is it intended that the state is not announced on focus? Usually, for controls like this, the state is part of the accessibility label or hint.
Co-authored-by: chunhtai <47866232+chunhtai@users.noreply.github.com>
…o feat/expansion-tile-live-region
|
This PR is closed because a new and improved method is now available that avoids using a live region. Recent developments support the expanded flag for ExpansionTile. It is preferable to use the expanded flag, rather than a live region, to announce whether a tile is expanded. |
Why this PR is closed:
This PR is closed because a new and improved method is now available that avoids using a live region. Recent developments support the
expandedflag forExpansionTile. It is preferable to use theexpandedflag, rather than a live region, to announce whether a tile is expanded.Migrates expansion tile to prefer using idiomatic a11y semantics over announcements.
Resolves #122101
Partially completes #165510
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on [Discord].
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.