-
Notifications
You must be signed in to change notification settings - Fork 29.4k
Fix #173302: Announce selected state for AM/PM buttons on iOS #173418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
On iOS, VoiceOver did not announce the selected state for the AM/PM buttons in the TimePickerDialog. This change makes sure the `selected` semantic property is set based on the current platform, allowing VoiceOver to correctly announce the state. Fixes flutter#173302
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses an accessibility issue on iOS where the selected state of AM/PM buttons in the TimePicker was not announced by VoiceOver. The fix correctly uses the selected
semantic property on iOS and checked
on other platforms. The implementation is straightforward and effective. A new widget test is added to verify the fix on iOS. My review includes a suggestion to improve the test's robustness by using addTearDown
for cleanup, ensuring that platform overrides do not leak into other tests.
debugDefaultTargetPlatformOverride = TargetPlatform.iOS; | ||
|
||
await tester.pumpWidget( | ||
MaterialApp( | ||
home: Builder( | ||
builder: (BuildContext context) { | ||
return TextButton( | ||
onPressed: () { | ||
showTimePicker( | ||
context: context, | ||
initialTime: const TimeOfDay(hour: 14, minute: 0), | ||
); | ||
}, | ||
child: const Text('Open Picker'), | ||
); | ||
}, | ||
), | ||
), | ||
); | ||
|
||
await tester.tap(find.text('Open Picker')); | ||
await tester.pumpAndSettle(); | ||
|
||
final Finder pmButtonSemantics = find.descendant( | ||
of: find.widgetWithText(InkWell, 'PM'), | ||
matching: find.byWidgetPredicate( | ||
(Widget widget) => widget is Semantics && widget.properties.selected == true, | ||
), | ||
); | ||
|
||
final Finder amButtonSemantics = find.descendant( | ||
of: find.widgetWithText(InkWell, 'AM'), | ||
matching: find.byWidgetPredicate( | ||
(Widget widget) => widget is Semantics && widget.properties.selected == false, | ||
), | ||
); | ||
|
||
expect(pmButtonSemantics, findsOneWidget, reason: 'The selected PM button should have Semantics with selected: true'); | ||
expect(amButtonSemantics, findsOneWidget, reason: 'The unselected AM button should have Semantics with selected: false'); | ||
|
||
debugDefaultTargetPlatformOverride = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure that debugDefaultTargetPlatformOverride
is always reset to its original value, even if the test fails, it's better to use addTearDown
. This prevents side effects that could cause subsequent tests to fail.1
Using addTearDown
is the idiomatic way to handle cleanup in Flutter tests. It's also good practice to restore the original platform value instead of assuming it was null
.
final TargetPlatform? originalPlatform = debugDefaultTargetPlatformOverride;
debugDefaultTargetPlatformOverride = TargetPlatform.iOS;
addTearDown(() {
debugDefaultTargetPlatformOverride = originalPlatform;
});
await tester.pumpWidget(
MaterialApp(
home: Builder(
builder: (BuildContext context) {
return TextButton(
onPressed: () {
showTimePicker(
context: context,
initialTime: const TimeOfDay(hour: 14, minute: 0),
);
},
child: const Text('Open Picker'),
);
},
),
),
);
await tester.tap(find.text('Open Picker'));
await tester.pumpAndSettle();
final Finder pmButtonSemantics = find.descendant(
of: find.widgetWithText(InkWell, 'PM'),
matching: find.byWidgetPredicate(
(Widget widget) => widget is Semantics && widget.properties.selected == true,
),
);
final Finder amButtonSemantics = find.descendant(
of: find.widgetWithText(InkWell, 'AM'),
matching: find.byWidgetPredicate(
(Widget widget) => widget is Semantics && widget.properties.selected == false,
),
);
expect(pmButtonSemantics, findsOneWidget, reason: 'The selected PM button should have Semantics with selected: true');
expect(amButtonSemantics, findsOneWidget, reason: 'The unselected AM button should have Semantics with selected: false');
Style Guide References
Footnotes
onTap: onPressed, | ||
child: Semantics( | ||
checked: selected, | ||
selected: Theme.of(context).platform == TargetPlatform.iOS ? selected : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should check defaultTargetPlatform instead of from the theme, if for some reason someone wants android feels and look on an iOS, the voice over will still be broken
expect(amButtonSemantics, findsOneWidget, reason: 'The unselected AM button should have Semantics with selected: false'); | ||
|
||
debugDefaultTargetPlatformOverride = null; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do testWidget(variant) to test a certain platform
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. |
Hi @JulioJ11 have you had an opportunity to check out the comments above? (from triage) |
Since we have not heard back in quite a while, I am going to close this PR. I am resolving the merge conflicts and applying the feedback from code review to re-land in another PR as we have users waiting on this fix. Thanks for contributing! |
}); | ||
} | ||
|
||
testWidgets('AM/PM buttons have correct selected semantics on iOS', (WidgetTester tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in the library test files as well, not in the sample code test.
Updated from #173418 Fixes #173302 Applied review feedback and modified for time licker refactor that landed somewhere in between. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Updated from flutter#173418 Fixes flutter#173302 Applied review feedback and modified for time licker refactor that landed somewhere in between. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Updated from flutter#173418 Fixes flutter#173302 Applied review feedback and modified for time licker refactor that landed somewhere in between. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot 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. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
On iOS, VoiceOver did not announce the selected state for the AM/PM buttons in the TimePicker. This change makes sure the
selected
semantic property is set based on the current platform, allowing VoiceOver to correctly announce the state.Fixes #173302
Pre-launch Checklist
///
).