Added proper focus handling when pushing and popping routes by gspencergoog · Pull Request #40166 · flutter/flutter

gspencergoog · 2019-09-10T18:27:42Z

Description

The proposed change will change focus handling when pushing and popping routes so that the FocusScopeNode for the route receives focus when pushed, and that the FocusScopeNode in the navigator receives focus when the route is popped.

This means that the last setFirstFocus call on the scope is used to determine which control actually receives focus. When the focus scope receives focus, it traverses its children, trying to find a non-scope node that is the "first focus" of itself or a child node.

This is a breaking change, because the focus behavior has changed. If you push a route after this change, and had a 'first focus' set on a widget via FocusScopeNode.setFirstFocus, it won't currently receive focus immediately, but after this change it will. Similarly, if you pop a route after this change, the focus will go back to where it was before the route was pushed, which is correct, but different from what happens now.

Related Issues

Addresses #13264

Tests

Added tests for route pushing/popping.
Adjusted other tests to account for new behavior.

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.
I checked all the boxes!

Breaking Change

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

Yes, this is a breaking change. (Breaking change announcement)

codecov · 2019-09-11T17:31:04Z

Codecov Report

Merging #40166 into master will decrease coverage by 1.95%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #40166      +/-   ##
==========================================
- Coverage   60.07%   58.11%   -1.96%     
==========================================
  Files         193      192       -1     
  Lines       18751    18443     -308     
==========================================
- Hits        11265    10719     -546     
- Misses       7486     7724     +238

Flag	Coverage Δ
#flutter_tool	`58.11% <0%> (-1.96%)`	⬇️

Impacted Files	Coverage Δ
packages/flutter_tools/lib/src/devfs.dart	`10.83% <0%> (-57.98%)`	⬇️
...ckages/flutter_tools/lib/src/ios/code_signing.dart	`61.76% <0%> (-36.79%)`	⬇️
...flutter_tools/lib/src/android/android_builder.dart	`12.5% <0%> (-30.36%)`	⬇️
...ackages/flutter_tools/lib/src/commands/attach.dart	`46.8% <0%> (-26.88%)`	⬇️
packages/flutter_tools/lib/src/linux/makefile.dart	`80% <0%> (-20%)`	⬇️
...s/flutter_tools/lib/src/windows/visual_studio.dart	`82.6% <0%> (-14.46%)`	⬇️
packages/flutter_tools/lib/src/base/build.dart	`61.34% <0%> (-13.66%)`	⬇️
...utter_tools/lib/src/reporting/crash_reporting.dart	`80.95% <0%> (-13.5%)`	⬇️
packages/flutter_tools/lib/src/android/gradle.dart	`62.39% <0%> (-13.42%)`	⬇️
packages/flutter_tools/lib/src/base/terminal.dart	`69.89% <0%> (-11.59%)`	⬇️
... and 128 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 406b449...8f8368f. Read the comment docs.

HansMuller

I can't say that I understand all of the implications of this (sensible) change. LGTM though.

…40166) The proposed change will change focus handling when pushing and popping routes so that the FocusScopeNode for the route receives focus when pushed, and that the FocusScopeNode in the navigator receives focus when the route is popped. This means that the last setFirstFocus call on the scope is used to determine which control actually receives focus. When the focus scope receives focus, it traverses its children, trying to find a non-scope node that is the "first focus" of itself or a child node. This is a breaking change, because the focus behavior has changed. If you push a route after this change, and had a 'first focus' set on a widget via FocusScopeNode.setFirstFocus, it won't currently receive focus immediately, but after this change it will. Similarly, if you pop a route after this change, the focus will go back to where it was before the route was pushed, which is correct, but different from what happens now.

ThinkDigitalSoftware · 2019-10-01T23:19:53Z

@gspencergoog This causes an issue when the route that's pushed is not a modal route, like a dropdown button or a popupmenu. It goes against the standard android behavior.

gspencergoog · 2019-10-02T14:44:56Z

@ThinkDigitalSoftware Can you elaborate? This change should only change focus when pushing a route if setFirstFocus has been called on the FocusScope (or autofocus has been set on a Focus widget). It doesn't focus the focus nodes directly, only their scopes. It's necessary to be able to do this in order for keyboard focus traversal to have a starting scope for focus.

ThinkDigitalSoftware · 2019-10-02T16:38:44Z

Well in my case, I haven't set any of those properties. I have a barebones example where when you tap a textfield and then a Dropdown, once the Dropdown is dismissed, the textfield is activated again and the keyboard always comes up, whether it was up previously or not. It makes sense for modal routes, because you're returning to where you are. But if you're filling out a form, this causes the screen to scroll back to the textfield and activate it, regardless of how far it is from the Dropdown /popup

ThinkDigitalSoftware · 2019-10-02T16:41:12Z

This pr was meant to address some of the issues, but not all. Also, I'm not sure if your PR caused all of the issues I'm having. I probably shouldn't insinuate that, but someone linked here and mentioned that this is when the behavior changed. #41574

ThinkDigitalSoftware · 2019-10-02T22:11:20Z

gspencergoog · 2019-10-03T01:19:01Z

@ThinkDigitalSoftware, I see the issue. I've also seen something else that is related just now, so I'm filing a bug and will look at it tomorrow: #41881

fluttergithubbot added p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. labels Sep 10, 2019

googlebot added the cla: yes label Sep 10, 2019

This was referenced Sep 10, 2019

Add shortcuts and actions for default focus traversal #40186

Merged

A11y: Dropdowns have accessibility focus issues #40101

Closed

gspencergoog force-pushed the route_focus2 branch from 7e3d396 to 27988c4 Compare September 12, 2019 23:46

gspencergoog marked this pull request as ready for review September 12, 2019 23:49

gspencergoog requested a review from HansMuller September 12, 2019 23:49

gspencergoog force-pushed the route_focus2 branch 2 times, most recently from 8f65927 to 1f31744 Compare September 19, 2019 08:57

HansMuller approved these changes Sep 23, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/widgets/routes.dart Outdated

Comment thread packages/flutter/lib/src/widgets/focus_manager.dart Outdated

Comment thread packages/flutter/lib/src/widgets/navigator.dart Outdated

gspencergoog force-pushed the route_focus2 branch from 1f31744 to f240aff Compare September 23, 2019 23:00

gspencergoog added the c: API break Backwards-incompatible API changes label Sep 23, 2019

gspencergoog mentioned this pull request Sep 24, 2019

Game controller and DPAD navigation should work #31957

Closed

gspencergoog force-pushed the route_focus2 branch from f240aff to c1d2a20 Compare September 25, 2019 21:43

gspencergoog added 3 commits September 26, 2019 10:55

Added proper focus handling when pushing and popping routes

534dbf2

Review changes

a00c002

Merged HEAD changes.

8f8368f

gspencergoog force-pushed the route_focus2 branch from c1d2a20 to 8f8368f Compare September 26, 2019 17:57

gspencergoog merged commit 1a7bb1f into flutter:master Sep 26, 2019

sarbagyastha mentioned this pull request Sep 29, 2019

Fixed DropdownButton behavior, when keyboard is visible #41574

Closed

8 of 9 tasks

gspencergoog deleted the route_focus2 branch October 9, 2019 18:12

github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Navigation Menu

Repository navigation

Added proper focus handling when pushing and popping routes - #40166

Added proper focus handling when pushing and popping routes#40166
gspencergoog merged 3 commits into
flutter:masterfrom
gspencergoog:route_focus2

gspencergoog commented Sep 10, 2019 •

edited

Loading

Uh oh!

codecov Bot commented Sep 11, 2019 •

edited

Loading

Uh oh!

HansMuller left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ThinkDigitalSoftware commented Oct 1, 2019

Uh oh!

gspencergoog commented Oct 2, 2019

Uh oh!

ThinkDigitalSoftware commented Oct 2, 2019

Uh oh!

ThinkDigitalSoftware commented Oct 2, 2019 •

edited

Loading

Uh oh!

ThinkDigitalSoftware commented Oct 2, 2019

Uh oh!

gspencergoog commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

gspencergoog commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

codecov Bot commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ThinkDigitalSoftware commented Oct 1, 2019

Uh oh!

gspencergoog commented Oct 2, 2019

Uh oh!

ThinkDigitalSoftware commented Oct 2, 2019

Uh oh!

ThinkDigitalSoftware commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThinkDigitalSoftware commented Oct 2, 2019

Uh oh!

gspencergoog commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gspencergoog commented Sep 10, 2019 •

edited

Loading

codecov Bot commented Sep 11, 2019 •

edited

Loading

ThinkDigitalSoftware commented Oct 2, 2019 •

edited

Loading