Directional focus edge traversal behavior. #161285
Conversation
github-actions
bot
added
framework
| /// [closedLoop] behavior. | ||
| parentScope, | ||
|
|
||
| /// Stops the focus traversal at the edge of the focus scope. |
There was a problem hiding this comment.
Is it really the edge of the focus scope? Isn't it actually the edge of the window? Directional traversal isn't supposed to be constrained by focus scopes, at least according to the description of DirectionalFocusTraversalPolicyMixin, where it describes an infinite band that it searches for candidates (which is what happens when it's looking for the first focus, at least, although I think we do start with traversalDescendants when we've found the first focus).
It might be good to think about what the actual bounds are on the search, and at least update the documentation to match the real behavior. From what I understand of the existing code and your change, it seems like the behavior is to search globally for the first one, and after that it's bound to the scope that the first focus resides in. Is that correct? And is it desirable? It should mirror what the native platforms do in the case of directional traversal. I think at least Android has a default traversal that I don't think is constrained by the view.
There was a problem hiding this comment.
Should directional traversal be constrained by FocusScope? This is a debatable question. I believe it should be constrained, as it provides more options, and we can achieve an unconstrained effect through TraversalEdgeBehavior. Additionally, directional traversal could not go beyond FocusScope before, so making it unconstrained would be a breaking change. When reaching the first focus, the traversal behavior will adopt the FocusScope where the focus is located. This is how it is currently handled, and from my observation, it seems to be the case in _moveFocus as well.
There was a problem hiding this comment.
Okay, that makes sense, and we don't need to decide the debate here, given the existing behavior seems to be working ok already. However, can you at least update your documentation to explain what "outermost" means in practice (is it the ones closest to the edge of the bounding box containing all the nodes in the scope? Something else?)?
And also please update the documentation for DirectionalFocusTraversalPolicyMixin so that it correctly describes what the algorithm actually does.
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
Thanks for all the work here, I know this is complex code to understand and modify.
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/flutter/161285, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
@gspencergoog could you help with the Google testing failure (in TAP local_focus_test.dart), it actually looks legit. |
|
Yeah, it is legit. It's going to be tough to debug, though. The Google test is testing focus changes initiated with keyboard directional input, but it's using a custom focus scope widget and a custom focus traversal policy, both of which are quite complex, so it's hard to know if that code is suspect. It's clear that this PR is changing the default behavior in some way, though. So maybe that's where to start, @yiiim: check the code and see where it might be changing the default behavior, or the default returned values from the policy functions. |
|
This is really exciting 🙈! Let me give it a try. |
yiiim
force-pushed
the
directional_focus_traversal_edge_behavior
branch
from
5130e4b to
df3f6f6
Compare
|
I did it! @gspencergoog Do you want to take another look?I modified it to not use |
Excellent work! The test indeed appears to pass now.
Could you please add a test that verifies that? If we'd had one before, it would have alerted you while you were working on it, so we're clearly missing that case. |
focus traversal
yiiim
force-pushed
the
directional_focus_traversal_edge_behavior
branch
from
df3f6f6 to
b992143
Compare
|
The new test has been added: testWidgets('When there is no focused node, the focus can be set to the FocusScopeNode.', (
WidgetTester tester,
) async {
final FocusScopeNode scope = FocusScopeNode();
final FocusScopeNode childScope = FocusScopeNode();
final FocusNode nodeA = FocusNode();
await tester.pumpWidget(
Directionality(
textDirection: TextDirection.ltr,
child: FocusScope(
node: scope,
child: FocusScope(
node: childScope,
child: Padding(
padding: const EdgeInsets.only(top: 10),
child: Focus(focusNode: nodeA, child: const Text('A')),
),
),
),
),
);
expect(scope.focusInDirection(TraversalDirection.down), isTrue);
await tester.pump();
expect(childScope.hasFocus, isTrue);
expect(nodeA.hasFocus, isFalse);
});But should we do this? Does it make sense for the focus to be on the |
Yes, I think that makes sense. The scope node should focus the appropriate thing next when it starts with the focus. |
3 participants
Fixes: #160843
This PR adds edge behavior feature for traversing focus using arrow keys. This allows cycling through the focus within the
FocusScopein a closed loop using the arrow keys. Which may be needed by TV application developers.Additionally, as in case #160843,
TraversalEdgeBehavior.parentScopecan be used to allow nested Navigators to traverse focus beyond the Navigator using arrow keys.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.