Adds a parent scope TraversalEdgeBehavior and fixes modal route to no… #130841
Conversation
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
github-actions
bot
added
framework
Contributor
There was a problem hiding this comment.
Suggested change
| // No need to mark dirty of this method is called during build phase. | |
| // No need to mark dirty if this method is called during build phase. |
chunhtai
commented
Jul 20, 2023
Contributor
Author
There was a problem hiding this comment.
I am not sure which one is more correct. Previously if every node is not traversable, pressing tab the focus will stay at the current focus.
After the change, it will unfocus the current focus.
I can preserve the previous behavior, but that will add a more complexity (for example i need to cache previous focused node and restore it if it can't find the a traversible node)
but I think this behavior probably doesn't worth restoring.
Contributor
There was a problem hiding this comment.
Yes, I think it's fine to unfocus in that case. The only case I worry about is the trivial one where the tree only contains one focus node and the root scope, because then when it unfocuses, it will focus the root scope, which traps the focus so that even if the widget tree changes later, traversal won't work anymore, since the root scope doesn't have any geometry associated with it, so findFirstFocus can't find anything. But the trivial case is pretty hard to create, since you'd have to have an app that doesn't use WidgetsApp as its base, and doesn't create any scopes of its own.
Contributor
Author
There was a problem hiding this comment.
Do you mean this FocusManager.instance.rootScope? I have a check that in the TraversalEdgeBehavior.parentScope to make sure it won't give up focus if the parent is FocusManager.instance.rootScope
Contributor
There was a problem hiding this comment.
Yes, that's what I mean. Sounds good.
Contributor
Author
There was a problem hiding this comment.
This is probably a bug that page 1 wasn't rebuild before.
gspencergoog
approved these changes
Jul 20, 2023
Contributor
There was a problem hiding this comment.
Is this the right default for web?
Given the code you added in app.dart, I would have expected this to be:
const TraversalEdgeBehavior kDefaultRouteTraversalEdgeBehavior = kIsWeb
? TraversalEdgeBehavior.leaveFlutterView
: TraversalEdgeBehavior.parentScope;
Contributor
Author
There was a problem hiding this comment.
This is to make sure the nested navigator would work in web.
cc @yjbanov correct me if i am wrong.
I think this should only be set to leaveFlutterView for the top-most focus scope, so that it can traverse focus to browser buttons
chunhtai
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
Contributor
|
auto label is removed for flutter/flutter, pr: 130841, due to - The status or check suite Linux firebase_release_smoke_test has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
chunhtai
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
Contributor
|
auto label is removed for flutter/flutter, pr: 130841, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Looks like some tests are failing. Is there any progress on this? |
Contributor
Author
|
I am resolving internal test failures. It may take a week or two. |
Member
|
I am resolving internal test failures. It may take a week or two. (Triage): What's the status of that? |
Contributor
Author
|
Waiting for internal customer's release before merging, they would like to prevent risky migration b/292847728 |
chunhtai
added
the
autosubmit
chunhtai
force-pushed
the
issues/112567
branch
from
1e1040e to
bb74320
Compare
auto-submit
bot
removed
the
autosubmit
Contributor
|
auto label is removed for flutter/flutter/130841, due to - The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
chunhtai
added
the
autosubmit
github-actions
bot
added
the
f: material design
auto-submit
bot
removed
the
autosubmit
Contributor
|
auto label is removed for flutter/flutter/130841, due to - The status or check suite Mac framework_tests_widgets has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 5, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 5, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 6, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 6, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 6, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 6, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 6, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 6, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 6, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 6, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 7, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 7, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 7, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 7, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Sep 7, 2023
chunhtai
added a commit
to chunhtai/flutter
that referenced
this pull request
Sep 12, 2023
…te to no… (flutter#130841)" This reverts commit 0f3bd90.
8 tasks
chunhtai
added a commit
that referenced
this pull request
Sep 12, 2023
#134550) …te to no… (#130841)" This reverts commit 0f3bd90. Internal test needs migration ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
itsjustkevin
pushed a commit
to itsjustkevin/flutter
that referenced
this pull request
Sep 12, 2023
flutter#134550) …te to no… (flutter#130841)" This reverts commit 0f3bd90. Internal test needs migration - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
XilaiZhang
pushed a commit
to XilaiZhang/flutter
that referenced
this pull request
Sep 14, 2023
flutter#134550) …te to no… (flutter#130841)" This reverts commit 0f3bd90. Internal test needs migration ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
XilaiZhang
added a commit
that referenced
this pull request
Sep 14, 2023
…nd fixes modal rou… (#134703) …… (#134550) …te to no… (#130841)" This reverts commit 0f3bd90. Internal test needs migration Co-authored-by: chunhtai <[email protected]>
XilaiZhang
pushed a commit
to XilaiZhang/flutter
that referenced
this pull request
Sep 16, 2023
flutter#134550) …te to no… (flutter#130841)" This reverts commit 0f3bd90. Internal test needs migration ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat
XilaiZhang
added a commit
that referenced
this pull request
Sep 16, 2023
…nd fixes modal rou… (#134848) …… (#134550) …te to no… (#130841)" This reverts commit 0f3bd90. Internal test needs migration Co-authored-by: chunhtai <[email protected]>
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Nov 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
…t trap the focus
fixes #112567
Several things I done:
parentScopeparentScopeleaveFlutterViewif platform is web.Previously 2 and 3 are not needed because once a focusscope trapped the focus, there isn't a chance where the parent focusscope have to deal with next focus.
If we don't do 2 and 3 after the change, it will cause it to stuck in the current scope again. Because once the focus leave the current scope, it needs to also remove the first focus in that scope (so that it can start fresh when focus traversal back to the scope in the future). At this point the current scope will have the primary focus without the first focus. The parent scope will then try to find the next focus, and it will place the focus to the first traversal child in the current scope again.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.