Fix time picker period selector a11y touch targets #170060
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
framework
bleroux
force-pushed
the
fix_time_picker_day_selector_tap_targets
branch
2 times, most recently
from
1f7e605 to
7c5332f
Compare
chunhtai
reviewed
Jun 5, 2025
| child: Column( | ||
| children: <Widget>[ | ||
| Expanded(child: amButton), | ||
| if (showSeparator) |
Contributor
There was a problem hiding this comment.
why we conditionally show separator now?
Contributor
Author
There was a problem hiding this comment.
Because with this fix two shapes are drawn. The bottom border of the AM button and the top border of the PM button replace the divider.
bleroux
force-pushed
the
fix_time_picker_day_selector_tap_targets
branch
from
7c5332f to
8992206
Compare
Contributor
Author
|
Convert to draft, as I will file 2 alternatives PRs for discussion. |
chunhtai
reviewed
Aug 13, 2025
Contributor
chunhtai
left a comment
chunhtai
left a comment
There was a problem hiding this comment.
I think this PR overall LGTM, just some minor comment
| inMutuallyExclusiveGroup: true, | ||
| button: true, | ||
| child: Padding( | ||
| padding: padding, |
Contributor
There was a problem hiding this comment.
Should we add a tap handler for the padded area?
Contributor
Author
There was a problem hiding this comment.
Managing tap events into the padded areas is already done by _DayPeriodInputPadding
| shape: pmShape, | ||
| ); | ||
|
|
||
| result = _DayPeriodInputPadding( |
Contributor
There was a problem hiding this comment.
Cans just return here directly
| shape: pmShape, | ||
| ); | ||
|
|
||
| result = _DayPeriodInputPadding( |
bleroux
force-pushed
the
fix_time_picker_day_selector_tap_targets
branch
from
8992206 to
7c8c2fd
Compare
Contributor
Author
|
Resurrecting this PR, see #171437 (comment). I still need some time to verify if more changes are needed. |
bleroux
force-pushed
the
fix_time_picker_day_selector_tap_targets
branch
from
7c8c2fd to
c541296
Compare
Contributor
Author
|
@chunhtai The PR is ready for review. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. 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. |
Contributor
|
Looks like there is a slight padding that shifted the entire clock widget by several pixel, which looks fines to me. cc @QuncCccccc Do you think that is ok? |
Contributor
Author
|
Thanks for the review 🙏
I will have a look tomorrow at this slight unwanted padding, I think I fixed a very similar golden failure on #171437. |
bleroux
force-pushed
the
fix_time_picker_day_selector_tap_targets
branch
from
c541296 to
a4ab736
Compare
github-actions
bot
added
the
tool
bleroux
force-pushed
the
fix_time_picker_day_selector_tap_targets
branch
from
7ccb148 to
29f3437
Compare
github-actions
bot
removed
the
tool
bleroux
force-pushed
the
fix_time_picker_day_selector_tap_targets
branch
from
29f3437 to
2986599
Compare
|
Golden file changes are available for triage from new commit, Click here to view. 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. |
Contributor
|
The golden diffs look very small, so I just triaged them. Waiting to check google testing result. |
bleroux
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 20, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 20, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 20, 2025
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Aug 20, 2025
…9862) Manual roll Flutter from e65380a22076 to 960d1078f876 (36 revisions) Manual roll requested by [email protected] flutter/flutter@e65380a...960d107 2025-08-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reapply "Add set semantics enabled API and wire iOS a11y bridge (#161… (#171198)" (flutter/flutter#174153) 2025-08-20 [email protected] Make sure that a Badge doesn't crash in 0x0 environment (flutter/flutter#172065) 2025-08-20 [email protected] Make sure that CalendarDatePicker & YearPicker don't crash in 0x0 environment (flutter/flutter#173408) 2025-08-20 [email protected] Roll Packages from 953cae0 to 58c02e0 (2 revisions) (flutter/flutter#174142) 2025-08-20 [email protected] Make sure that a CircleAvatar doesn't crash in 0x0 environment (flutter/flutter#173498) 2025-08-20 [email protected] Roll Dart SDK from 0d674ff61e2e to 0d0a0c394381 (1 revision) (flutter/flutter#174126) 2025-08-20 [email protected] [Android] Fix version code override calculation in FlutterPlugin (flutter/flutter#174081) 2025-08-20 [email protected] Make sure that a BackButton doesn't crash in 0x0 environment (flutter/flutter#172817) 2025-08-20 [email protected] Roll Dart SDK from c5f5a32df36c to 0d674ff61e2e (1 revision) (flutter/flutter#174099) 2025-08-20 [email protected] feat: Added FocusNode prop for DropdownMenu Trailing Icon Button (flutter/flutter#172753) 2025-08-20 [email protected] Make component theme data defaults use `WidgetStateProperty` (flutter/flutter#173893) 2025-08-20 [email protected] Fix Menu anchor reduce padding on web and desktop (flutter/flutter#172691) 2025-08-20 [email protected] Roll Skia from 4b788d0e5e63 to 721e68fe652a (2 revisions) (flutter/flutter#174095) 2025-08-20 [email protected] Fix time picker period selector a11y touch targets (flutter/flutter#170060) 2025-08-20 [email protected] Fix SegmentedButton focus issue (flutter/flutter#173953) 2025-08-20 [email protected] Roll Dart SDK from e936404543f1 to c5f5a32df36c (1 revision) (flutter/flutter#174089) 2025-08-20 [email protected] Roll Skia from 953bfc0e2f2a to 4b788d0e5e63 (1 revision) (flutter/flutter#174086) 2025-08-19 [email protected] Roll Skia from 07d71ea4d056 to 953bfc0e2f2a (18 revisions) (flutter/flutter#174072) 2025-08-19 [email protected] Roll Dart SDK from 9105d946af95 to e936404543f1 (5 revisions) (flutter/flutter#174074) 2025-08-19 [email protected] NavigationRail correct traversal order (flutter/flutter#173891) 2025-08-19 [email protected] Update CupertinoSliverNavigationBar.middle (flutter/flutter#173868) 2025-08-19 [email protected] Update the AccessibilityPlugin::Announce method to account for the view (flutter/flutter#172669) 2025-08-19 [email protected] [ Widget Preview ] Report an error if a web device is unavailable (flutter/flutter#174036) 2025-08-19 [email protected] [web] Fix error in ClickDebouncer when using VoiceOver (flutter/flutter#174046) 2025-08-19 [email protected] [ Tool ] Add logging to test_adapter_test.dart (flutter/flutter#174073) 2025-08-19 [email protected] Roll Fuchsia Linux SDK from n0EnLlotF2wczlOq_... to V1A1J6uXZ62Q10i9u... (flutter/flutter#174059) 2025-08-19 [email protected] Cleanup legacy `bringup: true` tasks, either removing or enabling (flutter/flutter#173815) 2025-08-19 [email protected] Add Shift+Enter shortcut example for TextField. (flutter/flutter#167952) 2025-08-19 [email protected] Check that the windows architecture is 64-bit and not the process architecture (flutter/flutter#174019) 2025-08-19 [email protected] Improve Stack widget error message for bounded constraints (flutter/flutter#173352) 2025-08-19 [email protected] [VPAT][A11y] AutoComplete dropdown option is missing button role (flutter/flutter#173297) 2025-08-19 [email protected] fix: Android build fails when minSdk is set below 24 in build.gradle.kts (#173823) (flutter/flutter#173825) 2025-08-19 [email protected] Reapply "Add set semantics enabled API and wire iOS a11y bridge (#161… (flutter/flutter#171198) 2025-08-19 [email protected] fix: only use library props for libraries (flutter/flutter#172704) 2025-08-19 [email protected] Roll Packages from 5c52c55 to 953cae0 (22 revisions) (flutter/flutter#174040) 2025-08-19 [email protected] Add `open_jdk` to `Linux linux_android_emulator.debug_x64` (flutter/flutter#173989) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose ...
SydneyBao
pushed a commit
to SydneyBao/flutter
that referenced
this pull request
Aug 22, 2025
## Description This PR fixes the TimePicker day period selector touch targets. ### Before | Flutter | Google Agenda | Compose | |--------|--------|--------| |  |  |   | ### After | Flutter | Google Agenda | Compose | |--------|--------|--------| | |  |   | ## Implementation choice This PR implements two main changes: - it expands the `_DayPeriodControl` bounds by reducing some existing spacing. Doing so the touch area of the AM/PM buttons can grow outside the day period container and respect the minimum interactive height. - it changes the tree order to correctly sized the AM/PM buttons semantics. The solution here is somewhat tricky/hacky: Because the semantics bounds are clipped by their ancestor, the PR changes the widget order: Before, the order was `_DayPeriodControl` > `_DayPeriodInputPadding` > `Material` (with shape and clip) > `Row` (or Column depending on the orientation) > children [ `Semantics` > AM `InkWell`, `Semantics` > PM `InkWell` ] (Which leads to the Semantics being clipped by the Material shape). After, the order is `_DayPeriodControl` > `_DayPeriodInputPadding` > `Row` (or Column depending on the orientation) > children [ `Semantics` > `Material` (with shape and clip) > AM `InkWell`, `Semantics` > `Material` (with shape and clip) > PM `InkWell` ] The difficulty here is that the `TimePickerThemeData.dayPeriodShape` is meant to be the shape for the whole day period container. In order to change the order, this PR has to set separetly the shapes of the AM and PM buttons. To do so it adds some logic to 'split' the shape in two parts. This is ok for the default shape but not possible for any shape, in that case the original shape will be use for both buttons which gives a different result than before this PR. This annoyance seems less a problem than having the default rendering not respecting a11y touch targets. ## Related Issue Fixes [touch target size not up to a11y standards for DatePicker day period selector.](flutter#168245) ## Tests Adds 2 tests. Updates 5 tests.
SydneyBao
pushed a commit
to SydneyBao/flutter
that referenced
this pull request
Aug 22, 2025
## Description This PR fixes the TimePicker day period selector touch targets. ### Before | Flutter | Google Agenda | Compose | |--------|--------|--------| |  |  |   | ### After | Flutter | Google Agenda | Compose | |--------|--------|--------| | |  |   | ## Implementation choice This PR implements two main changes: - it expands the `_DayPeriodControl` bounds by reducing some existing spacing. Doing so the touch area of the AM/PM buttons can grow outside the day period container and respect the minimum interactive height. - it changes the tree order to correctly sized the AM/PM buttons semantics. The solution here is somewhat tricky/hacky: Because the semantics bounds are clipped by their ancestor, the PR changes the widget order: Before, the order was `_DayPeriodControl` > `_DayPeriodInputPadding` > `Material` (with shape and clip) > `Row` (or Column depending on the orientation) > children [ `Semantics` > AM `InkWell`, `Semantics` > PM `InkWell` ] (Which leads to the Semantics being clipped by the Material shape). After, the order is `_DayPeriodControl` > `_DayPeriodInputPadding` > `Row` (or Column depending on the orientation) > children [ `Semantics` > `Material` (with shape and clip) > AM `InkWell`, `Semantics` > `Material` (with shape and clip) > PM `InkWell` ] The difficulty here is that the `TimePickerThemeData.dayPeriodShape` is meant to be the shape for the whole day period container. In order to change the order, this PR has to set separetly the shapes of the AM and PM buttons. To do so it adds some logic to 'split' the shape in two parts. This is ok for the default shape but not possible for any shape, in that case the original shape will be use for both buttons which gives a different result than before this PR. This annoyance seems less a problem than having the default rendering not respecting a11y touch targets. ## Related Issue Fixes [touch target size not up to a11y standards for DatePicker day period selector.](flutter#168245) ## Tests Adds 2 tests. Updates 5 tests.
mboetger
pushed a commit
to mboetger/flutter
that referenced
this pull request
Sep 18, 2025
## Description This PR fixes the TimePicker day period selector touch targets. ### Before | Flutter | Google Agenda | Compose | |--------|--------|--------| |  |  |   | ### After | Flutter | Google Agenda | Compose | |--------|--------|--------| | |  |   | ## Implementation choice This PR implements two main changes: - it expands the `_DayPeriodControl` bounds by reducing some existing spacing. Doing so the touch area of the AM/PM buttons can grow outside the day period container and respect the minimum interactive height. - it changes the tree order to correctly sized the AM/PM buttons semantics. The solution here is somewhat tricky/hacky: Because the semantics bounds are clipped by their ancestor, the PR changes the widget order: Before, the order was `_DayPeriodControl` > `_DayPeriodInputPadding` > `Material` (with shape and clip) > `Row` (or Column depending on the orientation) > children [ `Semantics` > AM `InkWell`, `Semantics` > PM `InkWell` ] (Which leads to the Semantics being clipped by the Material shape). After, the order is `_DayPeriodControl` > `_DayPeriodInputPadding` > `Row` (or Column depending on the orientation) > children [ `Semantics` > `Material` (with shape and clip) > AM `InkWell`, `Semantics` > `Material` (with shape and clip) > PM `InkWell` ] The difficulty here is that the `TimePickerThemeData.dayPeriodShape` is meant to be the shape for the whole day period container. In order to change the order, this PR has to set separetly the shapes of the AM and PM buttons. To do so it adds some logic to 'split' the shape in two parts. This is ok for the default shape but not possible for any shape, in that case the original shape will be use for both buttons which gives a different result than before this PR. This annoyance seems less a problem than having the default rendering not respecting a11y touch targets. ## Related Issue Fixes [touch target size not up to a11y standards for DatePicker day period selector.](flutter#168245) ## Tests Adds 2 tests. Updates 5 tests.
korca0220
pushed a commit
to korca0220/flutter
that referenced
this pull request
Sep 22, 2025
## Description This PR fixes the TimePicker day period selector touch targets. ### Before | Flutter | Google Agenda | Compose | |--------|--------|--------| |  |  |   | ### After | Flutter | Google Agenda | Compose | |--------|--------|--------| | |  |   | ## Implementation choice This PR implements two main changes: - it expands the `_DayPeriodControl` bounds by reducing some existing spacing. Doing so the touch area of the AM/PM buttons can grow outside the day period container and respect the minimum interactive height. - it changes the tree order to correctly sized the AM/PM buttons semantics. The solution here is somewhat tricky/hacky: Because the semantics bounds are clipped by their ancestor, the PR changes the widget order: Before, the order was `_DayPeriodControl` > `_DayPeriodInputPadding` > `Material` (with shape and clip) > `Row` (or Column depending on the orientation) > children [ `Semantics` > AM `InkWell`, `Semantics` > PM `InkWell` ] (Which leads to the Semantics being clipped by the Material shape). After, the order is `_DayPeriodControl` > `_DayPeriodInputPadding` > `Row` (or Column depending on the orientation) > children [ `Semantics` > `Material` (with shape and clip) > AM `InkWell`, `Semantics` > `Material` (with shape and clip) > PM `InkWell` ] The difficulty here is that the `TimePickerThemeData.dayPeriodShape` is meant to be the shape for the whole day period container. In order to change the order, this PR has to set separetly the shapes of the AM and PM buttons. To do so it adds some logic to 'split' the shape in two parts. This is ok for the default shape but not possible for any shape, in that case the original shape will be use for both buttons which gives a different result than before this PR. This annoyance seems less a problem than having the default rendering not respecting a11y touch targets. ## Related Issue Fixes [touch target size not up to a11y standards for DatePicker day period selector.](flutter#168245) ## Tests Adds 2 tests. Updates 5 tests.
Jaineel-Mamtora
pushed a commit
to Jaineel-Mamtora/flutter_forked
that referenced
this pull request
Sep 24, 2025
This PR fixes the TimePicker day period selector touch targets. | Flutter | Google Agenda | Compose | |--------|--------|--------| |  |  |   | | Flutter | Google Agenda | Compose | |--------|--------|--------| | |  |   | This PR implements two main changes: - it expands the `_DayPeriodControl` bounds by reducing some existing spacing. Doing so the touch area of the AM/PM buttons can grow outside the day period container and respect the minimum interactive height. - it changes the tree order to correctly sized the AM/PM buttons semantics. The solution here is somewhat tricky/hacky: Because the semantics bounds are clipped by their ancestor, the PR changes the widget order: Before, the order was `_DayPeriodControl` > `_DayPeriodInputPadding` > `Material` (with shape and clip) > `Row` (or Column depending on the orientation) > children [ `Semantics` > AM `InkWell`, `Semantics` > PM `InkWell` ] (Which leads to the Semantics being clipped by the Material shape). After, the order is `_DayPeriodControl` > `_DayPeriodInputPadding` > `Row` (or Column depending on the orientation) > children [ `Semantics` > `Material` (with shape and clip) > AM `InkWell`, `Semantics` > `Material` (with shape and clip) > PM `InkWell` ] The difficulty here is that the `TimePickerThemeData.dayPeriodShape` is meant to be the shape for the whole day period container. In order to change the order, this PR has to set separetly the shapes of the AM and PM buttons. To do so it adds some logic to 'split' the shape in two parts. This is ok for the default shape but not possible for any shape, in that case the original shape will be use for both buttons which gives a different result than before this PR. This annoyance seems less a problem than having the default rendering not respecting a11y touch targets. Fixes [touch target size not up to a11y standards for DatePicker day period selector.](flutter#168245) Adds 2 tests. Updates 5 tests.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Nov 12, 2025
lucaantonelli
pushed a commit
to lucaantonelli/flutter
that referenced
this pull request
Nov 21, 2025
## Description This PR fixes the TimePicker day period selector touch targets. ### Before | Flutter | Google Agenda | Compose | |--------|--------|--------| |  |  |   | ### After | Flutter | Google Agenda | Compose | |--------|--------|--------| | |  |   | ## Implementation choice This PR implements two main changes: - it expands the `_DayPeriodControl` bounds by reducing some existing spacing. Doing so the touch area of the AM/PM buttons can grow outside the day period container and respect the minimum interactive height. - it changes the tree order to correctly sized the AM/PM buttons semantics. The solution here is somewhat tricky/hacky: Because the semantics bounds are clipped by their ancestor, the PR changes the widget order: Before, the order was `_DayPeriodControl` > `_DayPeriodInputPadding` > `Material` (with shape and clip) > `Row` (or Column depending on the orientation) > children [ `Semantics` > AM `InkWell`, `Semantics` > PM `InkWell` ] (Which leads to the Semantics being clipped by the Material shape). After, the order is `_DayPeriodControl` > `_DayPeriodInputPadding` > `Row` (or Column depending on the orientation) > children [ `Semantics` > `Material` (with shape and clip) > AM `InkWell`, `Semantics` > `Material` (with shape and clip) > PM `InkWell` ] The difficulty here is that the `TimePickerThemeData.dayPeriodShape` is meant to be the shape for the whole day period container. In order to change the order, this PR has to set separetly the shapes of the AM and PM buttons. To do so it adds some logic to 'split' the shape in two parts. This is ok for the default shape but not possible for any shape, in that case the original shape will be use for both buttons which gives a different result than before this PR. This annoyance seems less a problem than having the default rendering not respecting a11y touch targets. ## Related Issue Fixes [touch target size not up to a11y standards for DatePicker day period selector.](flutter#168245) ## Tests Adds 2 tests. Updates 5 tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the TimePicker day period selector touch targets.
Before
After
Implementation choice
This PR implements two main changes:
_DayPeriodControlbounds by reducing some existing spacing. Doing so the touch area of the AM/PM buttons can grow outside the day period container and respect the minimum interactive height.Because the semantics bounds are clipped by their ancestor, the PR changes the widget order:
Before, the order was
_DayPeriodControl>_DayPeriodInputPadding>Material(with shape and clip) >Row(or Column depending on the orientation) > children [Semantics> AMInkWell,Semantics> PMInkWell](Which leads to the Semantics being clipped by the Material shape).
After, the order is
_DayPeriodControl>_DayPeriodInputPadding>Row(or Column depending on the orientation) > children [Semantics>Material(with shape and clip) > AMInkWell,Semantics>Material(with shape and clip) > PMInkWell]The difficulty here is that the
TimePickerThemeData.dayPeriodShapeis meant to be the shape for the whole day period container. In order to change the order, this PR has to set separetly the shapes of the AM and PM buttons. To do so it adds some logic to 'split' the shape in two parts. This is ok for the default shape but not possible for any shape, in that case the original shape will be use for both buttons which gives a different result than before this PR. This annoyance seems less a problem than having the default rendering not respecting a11y touch targets.Related Issue
Fixes touch target size not up to a11y standards for DatePicker day period selector.
Tests
Adds 2 tests.
Updates 5 tests.