Skip to content

Conversation

@JellyO1
Copy link
Contributor

@JellyO1 JellyO1 commented Feb 7, 2023

This PR exposes a requestFocusCallback on FocusTraversalPolicy and it's inheritors.

Fixes #83175.

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.

@flutter-dashboard flutter-dashboard bot added f: focus Focus traversal, gaining or losing focus framework flutter/packages/flutter repository. See also f: labels. labels Feb 7, 2023
@flutter-dashboard
Copy link

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.

Copy link
Contributor

@gspencergoog gspencergoog left a 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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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).

@JellyO1
Copy link
Contributor Author

JellyO1 commented Mar 1, 2023

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.
I'll also write the tests which will probably require further feedback as I'm not sure what exactly how and what I should test.

@flutter-dashboard
Copy link

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The `requestFocusCallback` can be used to overwrite the default behaviour
/// The `requestFocusCallback` can be used to override the default behaviour

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// is null, defaults to [defaultTraversalRequestFocusCallback].
/// is null, it defaults to [defaultTraversalRequestFocusCallback].

Copy link
Contributor

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.

Copy link
Contributor

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).

@goderbauer
Copy link
Member

(triage) @JellyO1 Do you still have plans to follow up on the feedback given above?

@JellyO1
Copy link
Contributor Author

JellyO1 commented Apr 25, 2023

Completely forgot about it, yes I'll finish it this week.

@goderbauer goderbauer requested a review from gspencergoog May 2, 2023 10:16
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@JellyO1 JellyO1 requested a review from gspencergoog May 6, 2023 17:36
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d (again)

Copy link
Contributor

@Piinks Piinks left a 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!

Flutter_LGTM

@Piinks Piinks added autosubmit Merge PR when tree becomes green via auto submit App c: new feature Nothing broken; request for a new capability labels May 8, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 8, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented May 8, 2023

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.

@goderbauer
Copy link
Member

(Triage) Looks like some tests are failing. @JellyO1 can you take a look?

@JellyO1
Copy link
Contributor Author

JellyO1 commented May 17, 2023

Yes, I'll take care of it.

JellyO1 and others added 8 commits May 17, 2023 10:45
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`.
@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label May 17, 2023
@auto-submit auto-submit bot merged commit 561169e into flutter:master May 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 18, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 18, 2023
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
...
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…0235)

This PR exposes a requestFocusCallback on `FocusTraversalPolicy` and it's inheritors.

Fixes flutter#83175.
nploi pushed a commit to nploi/packages that referenced this pull request Jul 16, 2023
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
...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App c: new feature Nothing broken; request for a new capability f: focus Focus traversal, gaining or losing focus framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scrolling with animation to the next focusable node

5 participants