Skip to content

Hook up soft keyboard "next" and "previous" buttons so that they move the focus by default #63592

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.

Sign up for GitHub

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

Jump to bottom
Merged
gspencergoog merged 3 commits into from
Aug 18, 2020
Merged

Hook up soft keyboard "next" and "previous" buttons so that they move the focus by default #63592

gspencergoog merged 3 commits into from
Aug 18, 2020

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Aug 12, 2020

Description

Focus will be moved automatically if onEditingComplete is not specified, but must
by moved manually if onEditingComplete is specified.

Related Issues

Tests

  • Added tests to make sure that the focus moves, and removed a test that verified that the focus was not lost.

Breaking Change

  • No, no existing tests failed, so this is not a breaking change.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 12, 2020
@gspencergoog gspencergoog force-pushed the editable_next_previous branch from 7ca9730 to ec2fc89 Compare August 12, 2020 18:06
@gspencergoog gspencergoog requested a review from justinmc August 12, 2020 18:06
HansMuller
HansMuller approved these changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

There are many more TextInputActions that have focus implications. Shouldn't we be checking all of them?

packages/flutter/test/widgets/editable_text_test.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Aug 12, 2020

Choose a reason for hiding this comment

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

Previous node has ... 'previous' button

Copy link
Contributor Author

@gspencergoog gspencergoog Aug 12, 2020

Choose a reason for hiding this comment

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

Thanks, fixed.

packages/flutter/test/widgets/editable_text_test.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Aug 12, 2020

Choose a reason for hiding this comment

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

In theory, you could do all of this checking in just one slightly more complicated version of this test.

Copy link
Contributor Author

@gspencergoog gspencergoog Aug 12, 2020

Choose a reason for hiding this comment

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

Well, sure, but doesn't that violate the principle of "one behavior => one test"?

Copy link
Contributor Author

@gspencergoog gspencergoog Aug 12, 2020

Choose a reason for hiding this comment

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

OK, I modified the tests to test all of the behaviors in separate tests (so they fail separately), but using a new TestVariant subclass so that I don't duplicate code when doing so. Let me know what you think.

@gspencergoog gspencergoog force-pushed the editable_next_previous branch 2 times, most recently from 9d5c366 to 48d33fa Compare August 12, 2020 20:05
HansMuller
HansMuller approved these changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

The test updates LGTM

packages/flutter/test/widgets/editable_text_test.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Aug 12, 2020

Choose a reason for hiding this comment

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

NICE

justinmc
justinmc approved these changes Aug 12, 2020
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/test/widgets/editable_text_test.dart Outdated
Copy link
Contributor

@justinmc justinmc Aug 12, 2020

Choose a reason for hiding this comment

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

I feel like this is an analyzer error if it's not indented, but if I'm wrong then no worries.

Copy link
Contributor Author

@gspencergoog gspencergoog Aug 13, 2020

Choose a reason for hiding this comment

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

No, it won't fail the analyzer, but it does need to be indented for readabililty. Done.

@gspencergoog gspencergoog force-pushed the editable_next_previous branch from 48d33fa to 91f614e Compare August 13, 2020 00:20
@gspencergoog
Hooks up soft keyboard "next" and "previous" buttons so that they mov…
c54b612 
…e the focus by default

Focus will be moved auotmatically if onEditingComplete is not specified, but must
by moved manually if onEditingComplete is specified.
@gspencergoog
Test focus behavior for all input actions.
6fc0b40
@gspencergoog
Review Changes
58f9c07
@gspencergoog gspencergoog force-pushed the editable_next_previous branch from 91f614e to 58f9c07 Compare August 18, 2020 23:04
@gspencergoog gspencergoog merged commit d1eff0b into flutter:master Aug 18, 2020
@gspencergoog gspencergoog deleted the editable_next_previous branch August 18, 2020 23:47
smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020
@gspencergoog @smadey
Hook up soft keyboard "next" and "previous" buttons so that they move…
49fadab 
… the focus by default (flutter#63592)

Focus will be moved automatically if onEditingComplete is not specified, but must
by moved manually if onEditingComplete is specified.
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@gspencergoog @mingwandroid
Hook up soft keyboard "next" and "previous" buttons so that they move…
b2844da 
… the focus by default (flutter#63592)

Focus will be moved automatically if onEditingComplete is not specified, but must
by moved manually if onEditingComplete is specified.
@LongCatIsLooong LongCatIsLooong mentioned this pull request Oct 26, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@justinmc justinmc justinmc approved these changes

+1 more reviewer

@HansMuller HansMuller HansMuller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TextInputAction.next does not move to the next input field, when pressing next(right-facing arrow button) on Android keyboard

4 participants

@gspencergoog @justinmc @HansMuller @googlebot