Skip to content

Implement focus traversal for desktop platforms, shoehorn edition. #30040

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 13 commits into from
Apr 22, 2019
Merged

Implement focus traversal for desktop platforms, shoehorn edition. #30040

gspencergoog merged 13 commits into from
Apr 22, 2019

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Mar 27, 2019 •
edited
Loading

Description

Implement focus traversal for desktop platforms, including re-implementing the existing focus manager and focus tree. This shoehorns all of the functionality of my focus traversal changes into the existing API, with minimal breakage.

This implements a Focus widget that can be put into a widget tree to allow input focus to be given to a particular part of a widget tree.

It incorporates with the existing FocusScope and FocusNode infrastructure, and has minimal breakage to the API, although FocusScope.reparentIfNeeded is removed, replaced by a call to FocusAttachment.reparent(), so this is a breaking change.

There are several followup PRs to this one, that will be layered on top: #30076 (see diff here) and #30077 (see diff here)

Related Issues

Addresses #11344, #1608, #13264, and #1678
Fixes #30084
Fixes #26704

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes tests for all changed/updated/fixed behaviors (See [Test Coverage]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change:

    • FocusScopeNodes must now be attached to the focus tree using FocusScopeNode.attach, which takes a context and an optional onKey callback, and returns a FocusAttachment that should be kept by the widget that hosts the FocusScopeNode, because of the need to make sure that the focus tree reflects the widget hierarchy.
    • Callers that used to call FocusScope(context).reparentIfNeeded will call reparent on their FocusAttachment instead.

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Mar 27, 2019 •
edited
Loading

I'd like feedback on other ways to supply the context to the FocusNode object. Currently I've broken the API so that people have to supply a context when they do FocusNode.reparentIfNeeded, but there are other ways I considered. This has been resolved by adding the FocusAttachment, where the context is passed to the attach method.

I could have had less direct breakage by just requiring that it be set before needing it, using the new setter on FocusNode, but that just seemed far too likely to cause people to forget to set it, and cause problems with traversal when someone was allowed to add one to the tree with no context set.

It felt like making them supply it drove home that they needed to set it, but I also dislike having multiple paths to setting it (via the constructor argument to FocusNode, the setter on FocusNode, and this extra argument to FocusNode.reparentIfNeeded). Using only the constructor argument alone seemed like a non-starter, because the context is often not available when constructing it.

@gspencergoog gspencergoog changed the title Focus shoehorn Implement focus traversal for desktop platforms, shoehorn edition. Mar 27, 2019
@gspencergoog gspencergoog force-pushed the focus_shoehorn branch 3 times, most recently from 1abb0dc to b2b947d Compare March 27, 2019 16:19
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
Hixie
Hixie reviewed Mar 27, 2019
View reviewed changes
@goderbauer goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label Mar 27, 2019
@gspencergoog gspencergoog force-pushed the focus_shoehorn branch 4 times, most recently from cf8a4c1 to adb2081 Compare March 28, 2019 00:05
@gspencergoog
Copy link
Contributor Author

gspencergoog commented Mar 28, 2019

I removed some of the "Add on" features for this PR to move them to another PR.

I didn't create the other PR because it still includes this one, but you can see the diffs here.

@justinmc justinmc mentioned this pull request Apr 18, 2019
Closed
@gspencergoog gspencergoog force-pushed the focus_shoehorn branch 2 times, most recently from de85581 to 23bd495 Compare April 18, 2019 16:51
goderbauer
goderbauer approved these changes Apr 18, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer 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 some nits

(Although I still wish we would clean up the API more and remove some of the cruft that's just there for backwards compatibility)

@vanlooverenkoen
Copy link
Contributor

vanlooverenkoen commented Apr 19, 2019

What about changing focus when pressing TAB on an iOS or Android simulator? Will you move to the next input field?

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Apr 19, 2019 •
edited
Loading

@vanlooverenkoen, this PR itself doesn't provide the key bindings for that to happen, but it, along with #30076, will provide the mechanism for changing focus to next and previous (and in an axis direction) widgets.

I have other PRs in the works that will add focus handling to material and cupertino widgets, and we will be producing a keyboard shortcut mechanism that will eventually enable what you're asking for.

In summary, we're working on it, but there are a lot of moving parts to be completed.

@gspencergoog gspencergoog merged commit 4218c0b into flutter:master Apr 22, 2019
gspencergoog added a commit to gspencergoog/flutter that referenced this pull request Apr 23, 2019
@gspencergoog
Revert "Implement focus traversal for desktop platforms, shoehorn edi…
7d1fce9 
…tion. (flutter#30040)"

This reverts commit 4218c0b.
gspencergoog added a commit that referenced this pull request Apr 23, 2019
@gspencergoog
Revert "Implement focus traversal for desktop platforms, shoehorn edi…
590cc27 
…tion. (#30040)" (#31461)

This reverts commit 4218c0b.
@gspencergoog gspencergoog mentioned this pull request Apr 25, 2019
Merged
gspencergoog added a commit that referenced this pull request Apr 25, 2019
@gspencergoog
[Re-Land] Implement focus traversal for desktop platforms. (#31614)
7775c23 
This re-lands the Focus changes in #30040. Correctness changes in routes.dart, and removes the automatic requesting of focus on reparent when there is no current focus, which caused undesirable selections.

Addresses #11344, #1608, #13264, and #1678
Fixes #30084
Fixes #26704
@gspencergoog gspencergoog deleted the focus_shoehorn branch May 15, 2019 16:30
@eyun-tv eyun-tv mentioned this pull request May 23, 2019
Open
@justinmc justinmc mentioned this pull request Oct 18, 2019
Closed
@esDotDev
Copy link

esDotDev commented Apr 16, 2020 •
edited
Loading

@gspencergoog We would like to be able to to traverse from the widget tree into an overlay and back. Is such a thing possible with the current API?

In our case, we're trying to create an auto-complete dropdown menu, we want to traverse into the dropdown contents, and back, as you would on a normal desktop control (https://i.imgur.com/xZCv3xm.gifv).

Happy to create a new issue, just wanted to check in case it is already a supported use case.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Hixie Hixie Hixie left review comments

@goderbauer goderbauer goderbauer approved these changes

+2 more reviewers

@rostopira rostopira rostopira left review comments

@HansMuller HansMuller HansMuller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: desktop Running on desktop c: API break Backwards-incompatible API changes 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.

Focus based navigation needs to have the widget Rect. crash on nested Navigator with GlobalKey

8 participants

@gspencergoog @Hixie @vanlooverenkoen @esDotDev @goderbauer @HansMuller @rostopira @googlebot