Fix time picker period selector a11y touch targets #170060
Conversation
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
| bottom: _TimePickerModel.useMaterial3Of(context) ? 20 : 24, | ||
| bottom: | ||
| (_TimePickerModel.useMaterial3Of(context) ? 20 : 24) - | ||
| minInteractiveVerticalPadding / 2, |
There was a problem hiding this comment.
IIUC, the additional space we save here will be shared by both bottom, why do we divide it by 2?
There was a problem hiding this comment.
Because this space is the one between the header and the period selector. Half of the whole vertical padding applies to it.
| @@ -306,7 +314,7 @@ class _TimePickerHeader extends StatelessWidget { | |||
| : VerticalDirection.down, | |||
| mainAxisAlignment: MainAxisAlignment.center, | |||
| crossAxisAlignment: CrossAxisAlignment.start, | |||
| spacing: 12, | |||
| spacing: math.max(0, 16 - minInteractiveVerticalPadding / 2), | |||
| child: Column( | ||
| children: <Widget>[ | ||
| Expanded(child: amButton), | ||
| if (showSeparator) |
There was a problem hiding this comment.
why we conditionally show separator now?
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.
| @@ -650,50 +646,126 @@ class _DayPeriodControl extends StatelessWidget { | |||
| } | |||
|
|
|||
| final Widget result; | |||
| bool showSeparator = true; | |||
There was a problem hiding this comment.
| bool showSeparator = true; | |
| final bool hasRoundedBorder = resolvedShape is RoundedRectangleBorder && resolvedShape.borderRadius is BorderRadius | |
| bool showSeparator = !hasRoundedBorder; |
There was a problem hiding this comment.
Though I don't quite get why we only show separator when this condition is met
There was a problem hiding this comment.
Though I don't quite get why we only show separator when this condition is met
The goal was to fallback to the previous rendering when it was not possible to cut the shape.
It's true that it might be better to always remove the separator and assume that the bottom of tha AM shape and the top of the PM shape will look as a Divider.
| if (resolvedShape is RoundedRectangleBorder && resolvedShape.borderRadius is BorderRadius) { | ||
| showSeparator = false; |
There was a problem hiding this comment.
| if (resolvedShape is RoundedRectangleBorder && resolvedShape.borderRadius is BorderRadius) { | |
| showSeparator = false; | |
| if (hasRoundedBorder) { |
| if (resolvedShape is RoundedRectangleBorder && resolvedShape.borderRadius is BorderRadius) { | ||
| showSeparator = false; |
There was a problem hiding this comment.
| if (resolvedShape is RoundedRectangleBorder && resolvedShape.borderRadius is BorderRadius) { | |
| showSeparator = false; | |
| if (hasRoundedBorder) { |
| minSemanticHeight: kMinInteractiveDimension, | ||
| checked: selected, | ||
| alignment: semanticAlignment, | ||
| child: Material( |
There was a problem hiding this comment.
Can you explain a bit why this is needed to expand the size of the button? Why would cliping at the parent row/ column prevent the child from expanding the size? Assuming children return big enough space for itself, parent material should not clip the space right?
There was a problem hiding this comment.
Let me try to explain this current solution and why it is not as simple as I would like it to be. I'm very interested in this discussion. It would great to find out a better solution (mainly I would prefer to remove the logic which 'cuts' the shape).
why this is needed to expand the size of the button? Why would cliping at the parent row/ column prevent the child from expanding the size? Assuming children return big enough space for itself, parent material should not clip the space right?
This _AmPmSemantics widget is not used to expand the size of the button, it is used to expand the semantics bounds.
The spec design requires that the visual size for each button is 40 in dial mode and 36 in input mode only.
For instance, see https://m3.material.io/components/time-pickers/specs#1cb56d62-0b4a-4db0-8620-215f2d0cfa69
To respect this visual size, the size of the buttons is restricted (line 758 below):
flutter/packages/flutter/lib/src/material/time_picker.dart
Lines 757 to 770 in 7c5332f
While writing this, I realize that now that this PR contains logic to cut the shape I can probably remove the _AmPmSemantics widget, remove the SizedBox line 757, and add some logic to add padding inside _AmPmButton. That way a usual Semantics widget could be use.
Let me verify this and push a commit. This will get us some reference to discuss further.
There was a problem hiding this comment.
I updated the PR to replace the _AmPmSemantics widget with a normal Semantics and adding some padding to get the inner Material properly sized.
I wonder if it would be possible to use a Stack to get rid of the logic which cuts the shape: one layer would contain the day period container and the InkWells, the second layer the Semantics, I don't know if it is possible. I will experiment and let you know.
| orientation == Orientation.portrait | ||
| ? defaultTheme.dayPeriodPortraitSize.height | ||
| : defaultTheme.dayPeriodLandscapeSize.height; | ||
| final double minInteractiveVerticalPadding = |
There was a problem hiding this comment.
are we sure only veritical space will be a problem?
There was a problem hiding this comment.
With the default day period size (from M3 and M2), the horizontal space is greater than minInteractiveDimension so I focused only on the vertical space.
I'm not sure it would make sense for someone to change this default to something less than minInteractiveDimension and assuming the framework will keep making this accessible.
bleroux
force-pushed
the
fix_time_picker_day_selector_tap_targets
branch
from
7c5332f to
8992206
Compare
|
Convert to draft, as I will file 2 alternatives PRs for discussion. |
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.I'm not aware of a way to specify that the semantics bounds should not be clipped by a parent. If a solution exists for this it would be better to use it instead of splitting the shape.
Related Issue
Fixes touch target size not up to a11y standards for DatePicker day period selector.
Tests
Adds 2 tests.
Updates 5 tests.