Skip to content

Implements FocusTraversalPolicy and DefaultFocusTraversal features.#30076

Merged
gspencergoog merged 4 commits intoflutter:masterfrom
gspencergoog:focus_traversal
May 10, 2019
Merged

Implements FocusTraversalPolicy and DefaultFocusTraversal features.#30076
gspencergoog merged 4 commits intoflutter:masterfrom
gspencergoog:focus_traversal

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Mar 28, 2019 •
edited
Loading

Description

This implements a DefaultFocusTraversal widget to describe the focus traversal policy for its children, defined by a FocusTraversalPolicy object from which custom policies may be created. Pre-defined policies include widget-order traversal, "reading order" traversal and directional traversal.

Issues

Addresses #11344, #1608, #13264, and #1678

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?

  • No, this is not a breaking change.

@gspencergoog gspencergoog changed the title Focus traversal Implements FocusTraversalPolicy and DefaultFocusTraversal features. Mar 28, 2019
This was referenced Mar 28, 2019
Closed
Merged
@gspencergoog gspencergoog force-pushed the focus_traversal branch 6 times, most recently from 87debcd to 6a387a9 Compare March 29, 2019 21:31
@goderbauer goderbauer added framework flutter/packages/flutter repository. See also f: labels. a: desktop Running on desktop labels Mar 30, 2019
@gspencergoog gspencergoog force-pushed the focus_traversal branch 2 times, most recently from e80a1a5 to 4a73033 Compare April 1, 2019 16:31
@gspencergoog gspencergoog requested a review from HansMuller April 1, 2019 16:32
@gspencergoog gspencergoog marked this pull request as ready for review April 1, 2019 16:32
@gspencergoog gspencergoog force-pushed the focus_traversal branch 4 times, most recently from 9c971c6 to 47d56a5 Compare April 2, 2019 12:56
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
Hixie
Hixie reviewed Apr 2, 2019
View reviewed changes
@gspencergoog gspencergoog force-pushed the focus_traversal branch 13 times, most recently from fb8f86b to d9d481f Compare April 22, 2019 20:33
@gspencergoog gspencergoog force-pushed the focus_traversal branch 4 times, most recently from 1921c25 to bedde69 Compare April 26, 2019 00:38
HansMuller
HansMuller reviewed Apr 27, 2019
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.

Looks pretty good. The only real issue is what to do with the policyData object.

packages/flutter/lib/src/widgets/focus_scope.dart Outdated
Copy link
Contributor

@HansMuller HansMuller Apr 26, 2019

Choose a reason for hiding this comment

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

descendants => children, since "[FocusScopeNode]s group together their children"

Copy link
Contributor Author

@gspencergoog gspencergoog Apr 29, 2019

Choose a reason for hiding this comment

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

Actually, it's the other way around: they group descendants.

@gspencergoog gspencergoog force-pushed the focus_traversal branch 4 times, most recently from 71d7de8 to ff42af2 Compare May 1, 2019 19:24
HansMuller
HansMuller approved these changes May 9, 2019
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.

LGTM

I wasn't able to study the changes since the last time I went over this PR, but it does look like the one issue that needed to be addressed, policy data's home, has been taken care of.

packages/flutter/lib/src/widgets/focus_traversal.dart Outdated

/// Clears the data associated with the given [FocusScopeNode] for this object.
///
/// This is used to indicate that the focus policy has changed mode, and so
Copy link
Contributor

@HansMuller HansMuller May 9, 2019

Choose a reason for hiding this comment

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

changed => changed its

Copy link
Contributor Author

@gspencergoog gspencergoog May 10, 2019

Choose a reason for hiding this comment

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

Fixed.

@eyun-tv eyun-tv mentioned this pull request May 23, 2019
Open
@eyun-tv
Copy link

eyun-tv commented May 27, 2019 •
edited
Loading

I write a demo use DefaultFocusTraversal with ReadingOrderTraversalPolicy

But , I find when you press up or down , sometimes , the focus will lose .

I record a video https://vimeo.com/338635739 for show this bug

My code is there :

https://github.com/eyun-tv/flutter-tv/blob/master/lib/main.dart

https://github.com/eyun-tv/flutter-tv/blob/master/lib/ui/util/video.dart

How to fix this problem ?

My flutter version is master and my tv box is android 9

@gspencergoog

@gspencergoog
Copy link
Contributor Author

gspencergoog commented May 28, 2019

@eyun-tv, I took a look, and it seems like it has to do with the anti-hysteresis code I put in for when directions are changed. I'll take a closer look and see if I can figure out a fix.

Here's the issue to follow: #33435

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 Awaiting requested review from goderbauer

+1 more reviewer

@HansMuller HansMuller HansMuller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: desktop Running on desktop 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.

6 participants

@gspencergoog @Hixie @eyun-tv @HansMuller @goderbauer @googlebot