-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Expose callback that allows focus traversal customization #120235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gspencergoog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice idea, thanks for the PR! When I wrote It, I thought about doing something similar, but decided to wait until there was a clear use case to make it public. Do you have a specific use case in mind?
This PR will need to have tests before we can merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably have a name that reflects that it is part of focus traversal, and not some version of the requestFocus on a focus node. Maybe TraversalRequestFocusCallback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to talk about what the default behavior is, and when it is invoked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be addressed in this doc comment. When the docs are built, this doc comment will be on its own page, and the user won't see the defaultTraversalRequestFocusCallback comment.
You might add a paragraph that talks about what the default value is, and what the default implementation does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to make a public value where this is the default, perhaps it makes sense to make this a static public function on FocusTraversalPolicy, called defaultTraversalRequestFocusCallback? Then people could implement their callback to call the default if all they want to do is log analytics or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, but I'm partial to keeping this function as it is and duplicating it as a static one on FocusTraversalPolicy since the method is also used in RequestFocusAction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not duplicate code. Just call this from defaultTraversalRequestFocusCallback, since it's static, or eliminate this and call the static instead of this (the second one is probably preferable).
|
Thanks for the feedback, I'll make the changes this weekend. My current use case is that I'm developing an Android TV application with stripe navigation and in the design requirements the selected element should always stay at the start so that it's clear what comes ahead. |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The `requestFocusCallback` can be used to overwrite the default behaviour | |
| /// The `requestFocusCallback` can be used to override the default behaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// is null, defaults to [defaultTraversalRequestFocusCallback]. | |
| /// is null, it defaults to [defaultTraversalRequestFocusCallback]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be addressed in this doc comment. When the docs are built, this doc comment will be on its own page, and the user won't see the defaultTraversalRequestFocusCallback comment.
You might add a paragraph that talks about what the default value is, and what the default implementation does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not duplicate code. Just call this from defaultTraversalRequestFocusCallback, since it's static, or eliminate this and call the static instead of this (the second one is probably preferable).
|
(triage) @JellyO1 Do you still have plans to follow up on the feedback given above? |
|
Completely forgot about it, yes I'll finish it this week. |
gspencergoog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RequestFocusIntent could take an optional TraversalRequestFocusCallback in its constructor, which you could call here. That would let the RequestFocusAction be customizable in the same way.
Of course, It would also need a test. Feel free to leave that for another PR if you don't have time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good catch, I'll do it on this PR since it makes the most sense.
gspencergoog
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome @JellyO1, thanks for contributing!
|
auto label is removed for flutter/flutter, pr: 120235, due to - The status or check suite Linux docs_test has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
(Triage) Looks like some tests are failing. @JellyO1 can you take a look? |
|
Yes, I'll take care of it. |
This commit creates an optional TraversalRequestFocusCallback parameter on `RequestFocusIntent` while keeping the original behaviour intact. This allows you to change the behaviour of the focus request in the same way you can change it on the `FocusTraversalPolicy`.
Co-authored-by: Loïc Sharma <[email protected]>
Co-authored-by: Loïc Sharma <[email protected]>
Co-authored-by: Loïc Sharma <[email protected]>
Co-authored-by: Loïc Sharma <[email protected]>
Co-authored-by: Greg Spencer <[email protected]>
Roll Flutter from d0d1feb to 5ae6438 (42 revisions) flutter/flutter@d0d1feb...5ae6438 2023-05-18 [email protected] Revert "Handle null return from WillPopCallback" (flutter/flutter#127112) 2023-05-18 [email protected] Roll Flutter Engine from a22dd6438335 to 843ce0bba356 (6 revisions) (flutter/flutter#127116) 2023-05-18 [email protected] mark windows build tests non-bringup (flutter/flutter#127059) 2023-05-18 [email protected] Roll Flutter Engine from d97037077963 to a22dd6438335 (1 revision) (flutter/flutter#127071) 2023-05-18 [email protected] Roll Packages from 5c69914 to b31a128 (9 revisions) (flutter/flutter#127109) 2023-05-18 [email protected] Revert "Remove obsolete drawShadow bounds workaround" (flutter/flutter#127110) 2023-05-18 [email protected] Clip search view content during animation (flutter/flutter#126975) 2023-05-18 [email protected] Remove obsolete drawShadow bounds workaround (flutter/flutter#127052) 2023-05-17 [email protected] [Windows] Improve version migration message (flutter/flutter#127048) 2023-05-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 9471598e3b7ff22b2fa181bd79addf94cb3e0847 to 6b107e7a7ee5e054e0bcce60de5181d21e2f00fb (flutter/flutter#127056) 2023-05-17 [email protected] Remove deprecated fixTextFieldOutlineLabel (flutter/flutter#125893) 2023-05-17 [email protected] Roll Flutter Engine from 331c5769e291 to d97037077963 (3 revisions) (flutter/flutter#127053) 2023-05-17 [email protected] shard windows build tests 3 -> 4 (flutter/flutter#127057) 2023-05-17 [email protected] Roll Flutter Engine from 0ae3719d7043 to 331c5769e291 (1 revision) (flutter/flutter#127049) 2023-05-17 [email protected] Ignore unused_element_parameter (flutter/flutter#126926) 2023-05-17 [email protected] Roll Flutter Engine from 47fd496c6f8d to 0ae3719d7043 (2 revisions) (flutter/flutter#127043) 2023-05-17 [email protected] Roll Flutter Engine from 400a26ad8343 to 47fd496c6f8d (3 revisions) (flutter/flutter#127041) 2023-05-17 [email protected] [flutter_tools] unpin and roll camera_android (flutter/flutter#126945) 2023-05-17 [email protected] Handle null return from WillPopCallback (flutter/flutter#127039) 2023-05-17 [email protected] Roll Flutter Engine from 6048360c1837 to 400a26ad8343 (1 revision) (flutter/flutter#127036) 2023-05-17 [email protected] Remove unused (and untested) parameters from private _MasterDetailFlow (flutter/flutter#126935) 2023-05-17 [email protected] Autocomplete async examples (flutter/flutter#126283) 2023-05-17 [email protected] Expose callback that allows focus traversal customization (flutter/flutter#120235) 2023-05-17 [email protected] Roll Packages from b971830 to 5c69914 (5 revisions) (flutter/flutter#127034) 2023-05-17 [email protected] Roll Flutter Engine from ea3f27383acc to 6048360c1837 (2 revisions) (flutter/flutter#127032) 2023-05-17 [email protected] Add missing `Switch.onFocusChange` test (flutter/flutter#126685) 2023-05-17 [email protected] Remove 'url_launcher' from pubspec.yaml. (flutter/flutter#126939) 2023-05-17 [email protected] Roll Flutter Engine from 4b7c37532ea4 to ea3f27383acc (1 revision) (flutter/flutter#126994) 2023-05-17 [email protected] Roll Flutter Engine from 170b45bae571 to 4b7c37532ea4 (1 revision) (flutter/flutter#126985) 2023-05-17 [email protected] Roll Flutter Engine from 28b9dc993c30 to 170b45bae571 (1 revision) (flutter/flutter#126981) 2023-05-17 [email protected] Roll Flutter Engine from 87a03e107df7 to 28b9dc993c30 (1 revision) (flutter/flutter#126974) 2023-05-17 [email protected] Roll Flutter Engine from 1c775e34e2d5 to 87a03e107df7 (3 revisions) (flutter/flutter#126968) 2023-05-16 [email protected] Fix style issues (flutter/flutter#122586) 2023-05-16 [email protected] Manual roll Flutter Engine from fe2476743b59 to 1c775e34e2d5 (10 revisions) (flutter/flutter#126961) 2023-05-16 [email protected] Make SlottedMultiChildRenderObjectWidgetMixin a concrete class (flutter/flutter#126108) 2023-05-16 [email protected] Add ScrollMetrics.extentTotal for completeness (flutter/flutter#126607) 2023-05-16 [email protected] Fix drone_dimensions. (flutter/flutter#126953) 2023-05-16 [email protected] Add checkmark style to CupertinoRadio (flutter/flutter#126480) 2023-05-16 [email protected] Move `Mac_build_test flutter_gallery__transition_perf_e2e_ios` to prod (flutter/flutter#126941) 2023-05-16 [email protected] Fix DataTableThemeData.copyWith handling of dataRowHeight (flutter/flutter#126943) 2023-05-16 [email protected] Fix copyWith method of ActionIconThemeData (flutter/flutter#126763) 2023-05-16 [email protected] Revert "Roll Flutter Engine from fe2476743b59 to 5cf141f7c03c (2 revisions)" (flutter/flutter#126954) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages ...
…0235) This PR exposes a requestFocusCallback on `FocusTraversalPolicy` and it's inheritors. Fixes flutter#83175.
Roll Flutter from d0d1feb to 5ae6438 (42 revisions) flutter/flutter@d0d1feb...5ae6438 2023-05-18 [email protected] Revert "Handle null return from WillPopCallback" (flutter/flutter#127112) 2023-05-18 [email protected] Roll Flutter Engine from a22dd6438335 to 843ce0bba356 (6 revisions) (flutter/flutter#127116) 2023-05-18 [email protected] mark windows build tests non-bringup (flutter/flutter#127059) 2023-05-18 [email protected] Roll Flutter Engine from d97037077963 to a22dd6438335 (1 revision) (flutter/flutter#127071) 2023-05-18 [email protected] Roll Packages from 5c69914 to b31a128 (9 revisions) (flutter/flutter#127109) 2023-05-18 [email protected] Revert "Remove obsolete drawShadow bounds workaround" (flutter/flutter#127110) 2023-05-18 [email protected] Clip search view content during animation (flutter/flutter#126975) 2023-05-18 [email protected] Remove obsolete drawShadow bounds workaround (flutter/flutter#127052) 2023-05-17 [email protected] [Windows] Improve version migration message (flutter/flutter#127048) 2023-05-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 9471598e3b7ff22b2fa181bd79addf94cb3e0847 to 6b107e7a7ee5e054e0bcce60de5181d21e2f00fb (flutter/flutter#127056) 2023-05-17 [email protected] Remove deprecated fixTextFieldOutlineLabel (flutter/flutter#125893) 2023-05-17 [email protected] Roll Flutter Engine from 331c5769e291 to d97037077963 (3 revisions) (flutter/flutter#127053) 2023-05-17 [email protected] shard windows build tests 3 -> 4 (flutter/flutter#127057) 2023-05-17 [email protected] Roll Flutter Engine from 0ae3719d7043 to 331c5769e291 (1 revision) (flutter/flutter#127049) 2023-05-17 [email protected] Ignore unused_element_parameter (flutter/flutter#126926) 2023-05-17 [email protected] Roll Flutter Engine from 47fd496c6f8d to 0ae3719d7043 (2 revisions) (flutter/flutter#127043) 2023-05-17 [email protected] Roll Flutter Engine from 400a26ad8343 to 47fd496c6f8d (3 revisions) (flutter/flutter#127041) 2023-05-17 [email protected] [flutter_tools] unpin and roll camera_android (flutter/flutter#126945) 2023-05-17 [email protected] Handle null return from WillPopCallback (flutter/flutter#127039) 2023-05-17 [email protected] Roll Flutter Engine from 6048360c1837 to 400a26ad8343 (1 revision) (flutter/flutter#127036) 2023-05-17 [email protected] Remove unused (and untested) parameters from private _MasterDetailFlow (flutter/flutter#126935) 2023-05-17 [email protected] Autocomplete async examples (flutter/flutter#126283) 2023-05-17 [email protected] Expose callback that allows focus traversal customization (flutter/flutter#120235) 2023-05-17 [email protected] Roll Packages from b971830 to 5c69914 (5 revisions) (flutter/flutter#127034) 2023-05-17 [email protected] Roll Flutter Engine from ea3f27383acc to 6048360c1837 (2 revisions) (flutter/flutter#127032) 2023-05-17 [email protected] Add missing `Switch.onFocusChange` test (flutter/flutter#126685) 2023-05-17 [email protected] Remove 'url_launcher' from pubspec.yaml. (flutter/flutter#126939) 2023-05-17 [email protected] Roll Flutter Engine from 4b7c37532ea4 to ea3f27383acc (1 revision) (flutter/flutter#126994) 2023-05-17 [email protected] Roll Flutter Engine from 170b45bae571 to 4b7c37532ea4 (1 revision) (flutter/flutter#126985) 2023-05-17 [email protected] Roll Flutter Engine from 28b9dc993c30 to 170b45bae571 (1 revision) (flutter/flutter#126981) 2023-05-17 [email protected] Roll Flutter Engine from 87a03e107df7 to 28b9dc993c30 (1 revision) (flutter/flutter#126974) 2023-05-17 [email protected] Roll Flutter Engine from 1c775e34e2d5 to 87a03e107df7 (3 revisions) (flutter/flutter#126968) 2023-05-16 [email protected] Fix style issues (flutter/flutter#122586) 2023-05-16 [email protected] Manual roll Flutter Engine from fe2476743b59 to 1c775e34e2d5 (10 revisions) (flutter/flutter#126961) 2023-05-16 [email protected] Make SlottedMultiChildRenderObjectWidgetMixin a concrete class (flutter/flutter#126108) 2023-05-16 [email protected] Add ScrollMetrics.extentTotal for completeness (flutter/flutter#126607) 2023-05-16 [email protected] Fix drone_dimensions. (flutter/flutter#126953) 2023-05-16 [email protected] Add checkmark style to CupertinoRadio (flutter/flutter#126480) 2023-05-16 [email protected] Move `Mac_build_test flutter_gallery__transition_perf_e2e_ios` to prod (flutter/flutter#126941) 2023-05-16 [email protected] Fix DataTableThemeData.copyWith handling of dataRowHeight (flutter/flutter#126943) 2023-05-16 [email protected] Fix copyWith method of ActionIconThemeData (flutter/flutter#126763) 2023-05-16 [email protected] Revert "Roll Flutter Engine from fe2476743b59 to 5cf141f7c03c (2 revisions)" (flutter/flutter#126954) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages ...


This PR exposes a requestFocusCallback on
FocusTraversalPolicyand it's inheritors.Fixes #83175.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.