Skip to content

Conversation

@flutter-zl
Copy link
Contributor

@flutter-zl flutter-zl commented Apr 24, 2025

This PR addresses the issue in time_picker.dart where pressing the TAB key to navigate between the hour and minute fields in input mode doesn't properly select the text in the newly focused field.

What Changed

  • Added focus traversal logic in the _HourMinuteTextFieldState class's initState method that:
  1. Properly handles TAB key presses to move focus to the next field
  2. Automatically selects all text when a field gains focus
  • Added tests in time_picker_test.dart to verify that:
  1. The hour field's text is selected when it gains focus
  2. Pressing TAB moves focus to the minute field
  3. The minute field's text is selected when it receives focus via TAB key

Why This Matters
This change improves accessibility and usability of the time picker in input mode. When users navigate using the keyboard (pressing TAB), they expect the text in the newly focused field to be fully selected, allowing them to immediately type a new value without having to manually delete the existing text. This is standard behavior in form fields and now the TimePicker component follows this expected pattern.

Before the change:
web:
https://time-picker-0423-before-change.web.app/
mobile:
https://github.com/user-attachments/assets/532c8aa2-4f8d-4118-9eaa-2fc6c2825486

After the change:
web:
https://time-picker-04232025.web.app/
mobile:
https://github.com/user-attachments/assets/9271b3b0-9ece-479e-a01e-a62e31b1d6c7

Issue to fix:
#165830

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine related. See also e: labels. f: material design flutter/packages/flutter/material repository. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) platform-web Web applications specifically labels Apr 24, 2025
@flutter-zl flutter-zl changed the title Time picker 0423 Highlighting hour and minute input fields Apr 25, 2025
@github-actions github-actions bot removed engine flutter/engine related. See also e: labels. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) platform-web Web applications specifically labels Apr 25, 2025
@flutter-zl flutter-zl requested review from mdebbar and yjbanov April 25, 2025 17:43
@flutter-zl flutter-zl marked this pull request as ready for review April 25, 2025 17:43
@flutter-zl flutter-zl requested a review from justinmc April 25, 2025 21:15
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me, but I would like someone more familiar with this code to take a look. Maybe @justinmc or @Renzo-Olivares ?

});

focusNode.onKey = (FocusNode node, RawKeyEvent event) {
if (event is RawKeyDownEvent && event.logicalKey == LogicalKeyboardKey.tab) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Before web app it looks like the focus was already moving to the next field as expected when pressing the Tab key. Is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expectation is the Text should be completely highlighted in both web and mobile.
Before the change
web:
before

After the change
web:
AFTER

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the before change video it looks like focus traversal is working as expected, the focus goes to the next widget on pressing the tab key. The difference I see in the after video is that the text is completely highlighted. If focus traversal is already working do we need to add the logic in the onKey method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I've pushed changes to address your comments — please take another look when you have time.

super.initState();
focusNode =
FocusNode()..addListener(() {
setState(() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use Actions here instead.

setState(() {
  // Rebuild when focus changes.
  if (kIsWeb && focusNode.hasFocus && primaryFocus?.context != null) {
    Actions.maybeInvoke(primaryFocus!.context!, SelectAllTextIntent(SelectionChangedCause.keyboard));
  }
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added kIsWeb because I couldn't reproduce the select all behavior on the native google calendar app timepicker on my Pixel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For google calendar, there are two options for time input. One is working properly. We are fixing the one that is not working properly. Please check the video. The expectation is the Text should be completely highlighted in both web and mobile.

screen-20250430-014509.mp4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the video above it looks like google calendar's time picker does not select all text on pressing the tab key, I think we should probably try to match that behavior to be consistent with native Android/mobile unless their is some spec or guideline that says otherwise. I agree that on all web platforms we should add the select all behavior since that is the default behavior or single line inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Yes. I agree. I will make the implementation web only.


// Verify that the hour field is focused and its text is selected.
final TextField hourTextField = tester.widget(hourField);
if (kIsWeb) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having the if branches for kIsWeb we can use skip: !kIsWeb to skip the test entirely if the platform is not web, it is a parameter on testWidgets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Thanks for this suggestions. I have made the changes. Please review.

if (kIsWeb && focusNode.hasFocus && primaryFocus?.context != null) {
Actions.maybeInvoke(
primaryFocus!.context!,
SelectAllTextIntent(SelectionChangedCause.keyboard),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The analyzer is complaining that we need to add a const here.

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the analyzer issue.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 6, 2025
mboetger pushed a commit to mboetger/flutter that referenced this pull request May 6, 2025
<!--
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 PR addresses the issue in time_picker.dart where pressing the TAB
key to navigate between the hour and minute fields in input mode doesn't
properly select the text in the newly focused field.

**What Changed**

- Added focus traversal logic in the _HourMinuteTextFieldState class's
initState method that:

1. Properly handles TAB key presses to move focus to the next field
2. Automatically selects all text when a field gains focus

- Added tests in time_picker_test.dart to verify that:

1. The hour field's text is selected when it gains focus
2. Pressing TAB moves focus to the minute field
3. The minute field's text is selected when it receives focus via TAB
key

**Why This Matters**
This change improves accessibility and usability of the time picker in
input mode. When users navigate using the keyboard (pressing TAB), they
expect the text in the newly focused field to be fully selected,
allowing them to immediately type a new value without having to manually
delete the existing text. This is standard behavior in form fields and
now the TimePicker component follows this expected pattern.

**Before the change:**
web:
https://time-picker-0423-before-change.web.app/
mobile:

https://github.com/user-attachments/assets/532c8aa2-4f8d-4118-9eaa-2fc6c2825486


**After the change:**
web:
https://time-picker-04232025.web.app/
mobile:

https://github.com/user-attachments/assets/9271b3b0-9ece-479e-a01e-a62e31b1d6c7



**Issue to fix:** 
flutter#165830 

## 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].
- [ ] 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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants