Remove use of announce in time_picker and remove double Feedback.forTap calls #167241
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. |
github-actions
bot
added
framework
|
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. |
ash2moon
force-pushed
the
fix/wrap-for-tap
branch
2 times, most recently
from
495ee64 to
bfde3c0
Compare
|
Test failure: @QuncCccccc once the test is fixed, are you the right person to review this Material time picker change? If not can you recommend a reviewer? |
|
I can take a look! But I probably will ask @chunhtai for a second review, as he is the accessibility expert! Please let me know when it's ready😊 |
ash2moon
force-pushed
the
fix/wrap-for-tap
branch
2 times, most recently
from
179de80 to
1769213
Compare
| case Orientation.portrait: | ||
| return Column( | ||
| return Semantics( | ||
| key: const ValueKey<String>('time-picker-header-semantics'), |
There was a problem hiding this comment.
we should avoid adding key for testing purposes like this.
If the purpose is to ensure the semantics label is correct in test, you can use find.bySemanticsLabel('your expected label') and findsOneWidget
| _TimePickerModel.selectedTimeOf(context), | ||
| alwaysUse24HourFormat: MediaQuery.alwaysUse24HourFormatOf(context), | ||
| ), | ||
| child: switch (_TimePickerModel.orientationOf(context)) { |
There was a problem hiding this comment.
Consider store the switch result into a child first for readability
| return Column( | ||
| return Semantics( | ||
| key: const ValueKey<String>('time-picker-header-semantics'), | ||
| label: MaterialLocalizations.of(context).formatTimeOfDay( |
There was a problem hiding this comment.
Can you explain a bit why we need a label here, is the previous behavior broken?
There was a problem hiding this comment.
This is because previously we were using _announceInitialTimeOnce to announce this time, but now with a label, it is announced when it is in focus.
| switch (widget.hourDialType) { | ||
| case _HourDialType.twentyFourHour: | ||
| case _HourDialType.twentyFourHourDoubleRing: | ||
| _announceToAccessibility(context, localizations.formatDecimal(newTime.hour)); |
There was a problem hiding this comment.
Since we remove all the announcement, how do we convey the information to user ? I assume there will be a liveregion somewhere?
Same for other announcements.
There was a problem hiding this comment.
A lot of these announcements were manually invoked because of the problem with Feedback.wrapForTap resulting in double announcements. Since the root cause was due to InkWell already doing a Feedback.wrapForTap, we did not need the extra wrapper within the on* callbacks. I think because of this, the original author used announce to prevent Talkback from speaking the double announcement and rewriting its responses.
There was a problem hiding this comment.
If in the course of your investigation you found prs or comments that helped you come to this conclusion please add them as a comment here. If not then please do not spend time tracking it down.
ash2moon
force-pushed
the
fix/wrap-for-tap
branch
from
6266ef5 to
e630328
Compare
| final RenderObjectWidget orientationSpecificHeader = switch (_TimePickerModel.orientationOf( | ||
| context, | ||
| )) { | ||
| Orientation.portrait => Column( |
There was a problem hiding this comment.
It is out of scope for this pr but I really dont like how we are using the device orientation enum value all over this code for what I think is a proxy for OrientationBuilder.
reidbaker
left a comment
reidbaker
left a comment
There was a problem hiding this comment.
Please hold merge until approval from someone on the framework team.
ash2moon
added
autosubmit
|
@ash2moon Is this PR ready for submission? |
ash2moon
added
the
autosubmit
…ap calls (flutter#167241) <!-- Thanks for filing a pull request! Reviewers are typically assigned within a week of filing a request. To learn more about code review, see our documentation on Tree Hygiene: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md --> This pull request replaces the use of the deprecated announce calls with idiomatic Semantic widgets and also removes the extra `Feedback.forTap` call. The `InkWell` widget already wraps the onTap with the `Feedback.forTap`. I also made sure sure that existing functionality is not impacted. Resolves flutter#166448 Partially addresses flutter#165510 Physical Testing: Android ✅ iOS ✅ Web ✅ ## 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. - [x] 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]. <!-- 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
6 participants
This pull request replaces the use of the deprecated announce calls with idiomatic Semantic widgets and also removes the extra
Feedback.forTapcall. TheInkWellwidget already wraps the onTap with theFeedback.forTap. I also made sure sure that existing functionality is not impacted.Resolves #166448
Partially addresses #165510
Physical Testing:
Android ✅
iOS ✅
Web ✅
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.