Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This PR refactors the semantics logic from ExpansionTile into the Expansible widget. The changes look good overall, and centralizing the accessibility logic is a great improvement.
I've found a critical issue in expansible.dart where it uses hardcoded English strings for semantics, which breaks localization. I've also found some incorrect test expectations in the new tests for Expansible.
Please see my detailed comments for suggestions on how to fix these issues.
| assert(!_animationController.isDismissed || !widget.controller.isExpanded); | ||
| final bool closed = !widget.controller.isExpanded && _animationController.isDismissed; | ||
| final bool shouldRemoveBody = closed && !widget.maintainState; | ||
| const DefaultWidgetsLocalizations localizations = DefaultWidgetsLocalizations(); |
There was a problem hiding this comment.
Using const DefaultWidgetsLocalizations() hardcodes the semantics strings to English and prevents them from being localized. You should use WidgetsLocalizations.of(context) to retrieve the correct localizations for the current locale.
final WidgetsLocalizations localizations = WidgetsLocalizations.of(context);| expect(semantics, isNotNull); | ||
| // On iOS/macOS platform, the semantics hint should include expanded/collapsed state guidance | ||
| // even theme platform is set to Android. | ||
| expect(semantics.hint, localizations.collapsedHint); |
There was a problem hiding this comment.
This expectation is incorrect. For non-iOS/macOS platforms, the semanticsHint in expansible.dart is intentionally null. The accessibility state (expanded/collapsed) is conveyed through the Semantics.expanded property, not the hint. The hint should be empty. This is consistent with the changes made to material/expansion_tile_test.dart in this PR.
Also, the comment on lines 519-520 seems to be a copy-paste error and should be corrected.
| expect(semantics.hint, localizations.collapsedHint); | |
| expect(semantics.hint, isEmpty); |
| await tester.pumpAndSettle(); | ||
|
|
||
| expect(semantics, isNotNull); | ||
| expect(semantics.hint, localizations.expandedHint); |
|
Since I introduced new texts in WidgetLocalization, do I need to generate the localization files for other languages to fix the test failures?? |
|
This is the command I ran: and it generated 116 files is this the correct command? |
|
Did all 3 tests fail cause of unnessary imports?? |
| import 'dart:async'; | ||
|
|
||
| import 'package:flutter/foundation.dart'; | ||
| import 'package:flutter/material.dart'; |
There was a problem hiding this comment.
This is a circular import.
| // [intended] https://github.com/flutter/flutter/issues/122101. | ||
| skip: defaultTargetPlatform == TargetPlatform.iOS, | ||
| ); | ||
| // TODO(srivatsvenkataraman): Re-enable these tests if announcements are re-added. |
There was a problem hiding this comment.
Merging commented-out code is disallowed.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Converted to draft a pending PR needs to be merged first |
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Am I good to continue work on this? I see #177785 has been completed for the ExpansionTile |
|
Re-started work on this have a question below if from expansion_tile in the material library: Widget _buildHeader(BuildContext context, Animation<double> animation) {
_iconColor = animation.drive(_iconColorTween.chain(_easeInTween));
_headerColor = animation.drive(_headerColorTween.chain(_easeInTween));
final MaterialLocalizations localizations = MaterialLocalizations.of(context);
final String onTapHint = _tileController.isExpanded
? localizations.expansionTileExpandedTapHint
: localizations.expansionTileCollapsedTapHint;
final String semanticsHint = switch (defaultTargetPlatform) {
TargetPlatform.iOS || TargetPlatform.macOS =>
_tileController.isExpanded
? '${localizations.collapsedHint}\n ${localizations.expansionTileExpandedHint}'
: '${localizations.expandedHint}\n ${localizations.expansionTileCollapsedHint}',
_ => _tileController.isExpanded ? localizations.collapsedHint : localizations.expandedHint,
};
final Widget child = ListTileTheme.merge(
iconColor: _iconColor.value ?? _expansionTileTheme.iconColor,
textColor: _headerColor.value,
child: ListTile(
enabled: widget.enabled,
onTap: _tileController.isExpanded ? _tileController.collapse : _tileController.expand,
dense: widget.dense,
splashColor: widget.splashColor,
visualDensity: widget.visualDensity,
enableFeedback: widget.enableFeedback,
contentPadding: widget.tilePadding ?? _expansionTileTheme.tilePadding,
leading: widget.leading ?? _buildLeadingIcon(context, animation),
title: widget.title,
subtitle: widget.subtitle,
trailing: widget.showTrailingIcon
? widget.trailing ?? _buildTrailingIcon(context, animation)
: null,
minTileHeight: widget.minTileHeight,
internalAddSemanticForOnTap: widget.internalAddSemanticForOnTap,
),
);
if (defaultTargetPlatform == TargetPlatform.android) {
return Semantics(
// Live region used to announce state changes (e.g., "expanded" or "collapsed")
// without taking focus.
// blockNode prevents this node from being part of the focus traversal.
label: semanticsHint,
liveRegion: true,
accessiblityFocusBlockType: AccessiblityFocusBlockType.blockNode,
child: Semantics(hint: semanticsHint, onTapHint: onTapHint, child: child),
);
}
return Semantics(hint: semanticsHint, onTapHint: onTapHint, child: child);
}So it would become: Widget _buildHeader(BuildContext context, Animation<double> animation) {
_iconColor = animation.drive(_iconColorTween.chain(_easeInTween));
_headerColor = animation.drive(_headerColorTween.chain(_easeInTween));
final MaterialLocalizations localizations = MaterialLocalizations.of(context);
final String onTapHint = _tileController.isExpanded
? localizations.expansionTileExpandedTapHint
: localizations.expansionTileCollapsedTapHint;
final String semanticsHint = switch (defaultTargetPlatform) {
TargetPlatform.iOS || TargetPlatform.macOS =>
_tileController.isExpanded
? '${localizations.collapsedHint}\n ${localizations.expansionTileExpandedHint}'
: '${localizations.expandedHint}\n ${localizations.expansionTileCollapsedHint}',
_ => _tileController.isExpanded ? localizations.collapsedHint : localizations.expandedHint,
};
final Widget child = ListTileTheme.merge(
iconColor: _iconColor.value ?? _expansionTileTheme.iconColor,
textColor: _headerColor.value,
child: ListTile(
enabled: widget.enabled,
onTap: _tileController.isExpanded ? _tileController.collapse : _tileController.expand,
dense: widget.dense,
splashColor: widget.splashColor,
visualDensity: widget.visualDensity,
enableFeedback: widget.enableFeedback,
contentPadding: widget.tilePadding ?? _expansionTileTheme.tilePadding,
leading: widget.leading ?? _buildLeadingIcon(context, animation),
title: widget.title,
subtitle: widget.subtitle,
trailing: widget.showTrailingIcon
? widget.trailing ?? _buildTrailingIcon(context, animation)
: null,
minTileHeight: widget.minTileHeight,
internalAddSemanticForOnTap: widget.internalAddSemanticForOnTap,
),
);
return child;
} |
victorsanni
left a comment
There was a problem hiding this comment.
This PR removes semantics logic from _buildHeader in ExpansionTile and CupertinoExpansionTile. However, Expansible.headerBuilder and Expansible.bodyBuilder are required and left entirely to the developer. So by definition there can't be any Semantics logic for the headers in Expansible since the developer has complete freedom on how to implement them.
Has this been considered? Can you explain more about what this PR addresses?
Hi Thank you for the review comment. This PR was created for: #175907
Based on that, I added the new properties to the Expansible widget and removed it from My mistake for not realizing developers could directly call Expansible and completely bypass Maybe I should add the new properties in expansible along with an additional flag if they want to add semantics or not? And when Expansible is called from |
This seems confusing and error prone. There are two main accessibility features of Material's
Feature 1) is universal so should be in Feature 2) is a function of the actual header implemented by |
Got it that makes a lot of sense. Let me copy this comment and add it in the issue to see what solutions we can come up with. I will also give it some thought. Do you recommend I keep this PR open for now? Or should I close it? |
I think this PR can be repurposed to solve feature 1)? |
So add the new attributes in Expansible and ensure there are no double announcements in Expansion tile and cupertino expansion tile? |
I don't have all the answers, but my understanding is we want to move the logic of semantics announcements when an |
This comment was marked as spam.
This comment was marked as spam.
|
Is this PR good for another round of review? (from triage) |
| if (defaultTargetPlatform == TargetPlatform.android) { | ||
| return; | ||
| } | ||
| if (defaultTargetPlatform == TargetPlatform.iOS) { |
There was a problem hiding this comment.
use MediaQuery.maybeSupportsAnnounceOf
There was a problem hiding this comment.
The check if (defaultTargetPlatform == TargetPlatform.iOS) is no longer needed
| ), | ||
| ); | ||
|
|
||
| if (defaultTargetPlatform == TargetPlatform.android) { |
There was a problem hiding this comment.
same, use MediaQuery. maybeSupportsAnnounceOf
There was a problem hiding this comment.
Sure, will do that and push an update.
|
Hi @chunhtai I added the MediaQuery. maybeSupportsAnnounceOf to the places you suggest. Also looks like most recent commit removed cupertino_expansion_tile from the changes. Can i close and open a fresh one? I tried amend commit but its not picking up... |
|
@srivats22 Sure! Please go ahead and reopen the PR if you feel needed, and request @chunhtai's review. Thank you for your contribution! |
|
Changes are actually all good and can be reviewed. |
| if (defaultTargetPlatform == TargetPlatform.android) { | ||
| return; | ||
| } | ||
| if (defaultTargetPlatform == TargetPlatform.iOS) { |
There was a problem hiding this comment.
The check if (defaultTargetPlatform == TargetPlatform.iOS) is no longer needed
| void _toggleExpansion() { | ||
| _announceExpansionChange(); |
There was a problem hiding this comment.
consider move the MediaQuery.maybeSupportsAnnounceOf(context) here.
| expanded: widget.controller.isExpanded, | ||
| onExpand: widget.controller.isExpanded ? null : widget.controller.expand, | ||
| onCollapse: !widget.controller.isExpanded ? null : widget.controller.collapse, | ||
| child: AnimatedBuilder( |
There was a problem hiding this comment.
This code can be deduplicated
Widget child = AnimatedBuilder(...);
if (MediaQuery.maybeSupportsAnnounceOf(context) ?? false) {
return Semantics(... ,child: child);
} else {
return Semantics(... ,child: child);
}
There was a problem hiding this comment.
Hi @chunhtai, I have added all the changes you requested.
|
|
||
| import 'package:flutter/foundation.dart'; | ||
| import 'package:flutter/semantics.dart'; | ||
| import 'package:flutter/widgets.dart'; |
There was a problem hiding this comment.
Importing the widgets library to make code changes to the widgets library is not allowed. Import individual files instead.
| case TargetPlatform.fuchsia: | ||
| case TargetPlatform.linux: | ||
| case TargetPlatform.windows: | ||
| break; |
There was a problem hiding this comment.
Why are we removing this for non-iOS/macOS platforms? Isn't this just the hint for when the header is initially displayed?
| // Match ExpansionTile's delayed VoiceOver announcement behavior. | ||
| _timer?.cancel(); | ||
| _timer = Timer(const Duration(seconds: 1), () { | ||
| SemanticsService.sendAnnouncement(View.of(context), stateHint, textDirection).catchError(( |
There was a problem hiding this comment.
Shouldn't this delay be iOS only?
| _timer = null; | ||
| }); | ||
|
|
||
| SemanticsService.sendAnnouncement(View.of(context), stateHint, textDirection).catchError(( |
There was a problem hiding this comment.
Why are there two announcements?
| child: Semantics(hint: semanticsHint, onTapHint: onTapHint, child: child), | ||
| ); | ||
| } | ||
| return Semantics(hint: semanticsHint, onTapHint: onTapHint, child: child); |
There was a problem hiding this comment.
I'm actually beginning to think maybe all semantics stuff in ExpansionTileState._buildHeader should move down to Expansible. And should just wrap the results of Expansible.headerBuilder.
Else it will be tricky managing different Semantics + announcements across ExpansionTile + Expansible. And I think it is not Material-opinionated (may be Android opinionated, but we can just create multiple strings for Android + iOS) so can be moved down? What do you think @chunhtai
| child: child, | ||
| ); | ||
|
|
||
| if (MediaQuery.maybeSupportsAnnounceOf(context) ?? false) { |
There was a problem hiding this comment.
This will make live region on iOS instead of android, shouldn't this be for android not iOS?
|
@victorsanni thank you for the review. I am thinking maybe it's becoming to confusing to follow along. So maybe after the migration to individual packages is done should I pick this one up again? |
Sure, converting to draft to get this one off our review queue. |
|
I've marked this PR as not ready to port to flutter/packages yet. The changes not in the material or cupertino libraries will remain here. |
|
Advice for PR splitting: Since this touches material/cupertino along with other parts of flutter/flutter, it will need to be split up. If the material/cupertino changes are dependent on the other non-material/cupertino changes in this PR, then the material/cupertino changes will need to wait for them to roll to stable before they can be ported over to material_ui/cupertino_ui in flutter/packages (which uses the stable branch).
|
Hi, |
|
If you decide to return to this, material_ui and cupertino_ui are now accepting contributions. :) The portions of this PR that affect the material and cupertino libraries can be ported over to flutter/packages, although they may have to wait for the changes in the core SDK to reach stable first. Guidance and instructions for easily porting over can be found in #188444 |
Thank you for the update I will resume work on this shortly |

This PR Fixes: #175907
It moves the semantics from
ExpansionTilefor material and cupertino into theExpansiblewidget.expansable_changes.webm
Note: When running the tests a Test related to golden was failling wasnt sure how to fix it... any help/suggestions would be great.
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.