[Re-Land] Implement focus traversal for desktop platforms.#31614
Merged
gspencergoog merged 5 commits intoflutter:masterfrom Apr 25, 2019
Merged
[Re-Land] Implement focus traversal for desktop platforms.#31614gspencergoog merged 5 commits intoflutter:masterfrom
gspencergoog merged 5 commits intoflutter:masterfrom
Conversation
This reverts commit 590cc27 (flutter#31461) to re-land the focus rewrite.
HansMuller
approved these changes
Apr 25, 2019
Contributor
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
LGTM so long as route.navigator is guaranteed to be non-null for some reason or we use route?.navigator
| animations.add(widget.route.secondaryAnimation); | ||
| _listenable = Listenable.merge(animations); | ||
| if (widget.route.isCurrent) { | ||
| widget.route.navigator.focusScopeNode.setFirstFocus(focusScopeNode); |
Contributor
There was a problem hiding this comment.
According to https://docs.flutter.io/flutter/widgets/Route/navigator.html, a route's navigator can be null (here and below).
Contributor
Author
There was a problem hiding this comment.
It can only be null if it hasn't been pushed yet, or if it has been disposed.
In this case isCurrent checks that the navigator isn't null, and in the case of didPush below, by definition it has been pushed, so it's probably OK as-is. I'm happy to still add the ?'s if you think it would be better to do it defensively against a future change, but they're not needed as-is.
This was referenced May 1, 2019
gspencergoog
added a commit
that referenced
this pull request
May 1, 2019
In #31614, I added an unfocus() to FocusNodes to allow giving up of focus, but it only worked on the primary focus. This changes that so that it will unfocus the entire chain, not just the primary focus. Now, if you call unfocus() on a FocusNode or FocusScopeNode, and their hasFocus returns true, then after calling unfocus(), it will return false. Before this change, it would only do that if hasPrimaryFocus was also true. This also fixes a bug in the way setFirstFocus was implemented, making it conform more to the behavior of the previous implementation. It has simplified logic in reparent, and in when it requests focus for scope nodes that have had setFirstFocus called on them.
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 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.
Related Issues
Addresses #11344, #1608, #13264, and #1678
Fixes #30084
Fixes #26704