Skip to content

Fix FocusTraversalPolicy makes focus lost#34712

Merged
gspencergoog merged 4 commits intoflutter:masterfrom
boyan01:master
Jul 17, 2019
Merged

Fix FocusTraversalPolicy makes focus lost#34712
gspencergoog merged 4 commits intoflutter:masterfrom
boyan01:master

Conversation

@boyan01
Copy link
Contributor

@boyan01 boyan01 commented Jun 19, 2019

Description

FocusTraversalPolicy keep the previously visited node to avoid hysteresis. But even if the visited focus has been disposed, FocusTraversalPolicy will still use it to requestFocus, which will cause FocusManger to get an abandoned node to get the focus.

Related Issues

Fixes #34153
Fixes #33435

Tests

I added the following tests:

  • A test to see that focus can be correctly found when visited focus has been disposed.

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.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • 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.

@boyan01 boyan01 changed the title Fix FocusTraversalPolicy makes focus loss (#34153) Fix FocusTraversalPolicy makes focus lost (#34153) Jun 19, 2019
@boyan01
Copy link
Contributor Author

boyan01 commented Jun 19, 2019

@darrenaustin @gspencergoog Review Request

@goderbauer goderbauer added a: desktop Running on desktop framework flutter/packages/flutter repository. See also f: labels. labels Jun 19, 2019
@dnfield
Copy link
Contributor

dnfield commented Jun 26, 2019

The release smoke tests failure is expected.

@gspencergoog would really be the best person to review this, but he is OOO right now. Sorry for the delay, but it would be best if we can wait until he's back to review this.

@boyan01
Copy link
Contributor Author

boyan01 commented Jun 27, 2019

@gspencergoog would really be the best person to review this, but he is OOO right now. Sorry for the delay, but it would be best if we can wait until he's back to review this.

thanks for your reply. of course we could wait spencergoog back to review, maybe he has better ideas to fix this issue.

@Piinks
Copy link
Contributor

Piinks commented Jul 17, 2019

Hi @boyan01, Are you ready for this to be merged? It looks like this is ready to go unless you would like to make more changes.

@boyan01
Copy link
Contributor Author

boyan01 commented Jul 17, 2019

Hi @boyan01, Are you ready for this to be merged? It looks like this is ready to go unless you would like to make more changes.

@Piinks Yes, this PR is ready.

@gspencergoog gspencergoog merged commit b2cc970 into flutter:master Jul 17, 2019
@gspencergoog gspencergoog changed the title Fix FocusTraversalPolicy makes focus lost (#34153) Fix FocusTraversalPolicy makes focus lost Jul 17, 2019
@gspencergoog
Copy link
Contributor

@boyan01, thanks so much for the fix!

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
)

FocusTraversalPolicy keep the previously visited node to avoid hysteresis. But even if the visited focus has been disposed, FocusTraversalPolicy will still use it to requestFocus, which will cause FocusManger to get an abandoned node to get the focus.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: desktop Running on desktop framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Focus sometimes lost when focus move in the GridView Anti-hysteresis code causes loss of focus occasionally.

6 participants