Unverified Commit be75fb36 authored by Greg Spencer's avatar Greg Spencer Committed by GitHub

Change unfocus to unfocus the entire chain, Fix setFirstFocus (#31909)

In #31614, I added an unfocus() to FocusNodes to allow giving up of focus, but it only worked on the primary focus. This changes that so that it will unfocus the entire chain, not just the primary focus. Now, if you call unfocus() on a FocusNode or FocusScopeNode, and their hasFocus returns true, then after calling unfocus(), it will return false. Before this change, it would only do that if hasPrimaryFocus was also true.

This also fixes a bug in the way setFirstFocus was implemented, making it conform more to the behavior of the previous implementation. It has simplified logic in reparent, and in when it requests focus for scope nodes that have had setFirstFocus called on them.
parent 63aa5b36
...@@ -61,6 +61,9 @@ class FocusAttachment { ...@@ -61,6 +61,9 @@ class FocusAttachment {
void detach() { void detach() {
assert(_node != null); assert(_node != null);
if (isAttached) { if (isAttached) {
if (_node.hasPrimaryFocus) {
_node.unfocus();
}
_node._parent?._removeChild(_node); _node._parent?._removeChild(_node);
_node._attachment = null; _node._attachment = null;
} }
...@@ -494,13 +497,18 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -494,13 +497,18 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
/// Has no effect on nodes that return true from [hasFocus], but false from /// Has no effect on nodes that return true from [hasFocus], but false from
/// [hasPrimaryFocus]. /// [hasPrimaryFocus].
void unfocus() { void unfocus() {
final FocusScopeNode scope = enclosingScope; if (hasPrimaryFocus) {
if (scope == null) { final FocusScopeNode scope = enclosingScope;
// This node isn't part of a tree. assert(scope != null, 'Node has primary focus, but no enclosingScope.');
scope._focusedChildren.remove(this);
_manager?._willUnfocusNode(this);
return; return;
} }
scope._focusedChildren.remove(this); if (hasFocus) {
_manager?._willUnfocusNode(this); // If we are in the focus chain, but not the primary focus, then unfocus
// the primary instead.
_manager._currentFocus.unfocus();
}
} }
/// Removes the keyboard token from this focus node if it has one. /// Removes the keyboard token from this focus node if it has one.
...@@ -545,13 +553,12 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -545,13 +553,12 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
// Removes the given FocusNode and its children as a child of this node. // Removes the given FocusNode and its children as a child of this node.
@mustCallSuper @mustCallSuper
void _removeChild(FocusNode node) { void _removeChild(FocusNode node) {
assert(node != null);
assert(_children.contains(node), "Tried to remove a node that wasn't a child."); assert(_children.contains(node), "Tried to remove a node that wasn't a child.");
assert(node._parent == this); assert(node._parent == this);
assert(node._manager == _manager); assert(node._manager == _manager);
// If the child was (or requested to be) the primary focus, then unfocus it node.enclosingScope?._focusedChildren?.remove(node);
// and cancel any outstanding request to be focused.
node.unfocus();
node._parent = null; node._parent = null;
_children.remove(node); _children.remove(node);
...@@ -577,32 +584,14 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -577,32 +584,14 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
} }
assert(_manager == null || child != _manager.rootScope, "Reparenting the root node isn't allowed."); assert(_manager == null || child != _manager.rootScope, "Reparenting the root node isn't allowed.");
assert(!ancestors.contains(child), 'The supplied child is already an ancestor of this node. Loops are not allowed.'); assert(!ancestors.contains(child), 'The supplied child is already an ancestor of this node. Loops are not allowed.');
FocusNode oldPrimaryFocus; final bool hadFocus = child.hasFocus;
if (child._manager != null) {
// We want to find out what the primary focus is, since the new child
// might be an ancestor of the primary focus, and the primary focus should
// move with the child.
oldPrimaryFocus = child.hasFocus ? child._manager._currentFocus : null;
assert(oldPrimaryFocus == null || oldPrimaryFocus == child || oldPrimaryFocus.ancestors.contains(child),
"child has focus, but primary focus isn't a descendant of it.");
}
// If the child currently has focus, we have to do some extra work to keep
// that focus, and to notify any scopes that used to be ancestors, and no
// longer have focus after we move it.
final Set<FocusNode> oldFocusPath = oldPrimaryFocus?.ancestors?.toSet() ?? <FocusNode>{};
child._parent?._removeChild(child); child._parent?._removeChild(child);
_children.add(child); _children.add(child);
child._parent = this; child._parent = this;
child._updateManager(_manager); child._updateManager(_manager);
if (oldPrimaryFocus != null) { if (hadFocus) {
final Set<FocusNode> newFocusPath = _manager?._currentFocus?.ancestors?.toSet() ?? <FocusNode>{}; // Update the focus chain for the current focus without changing it.
// Nodes that will no longer be focused need to be marked dirty. _manager?._currentFocus?._setAsFocusedChild();
for (FocusNode node in oldFocusPath.difference(newFocusPath)) {
node._markAsDirty();
}
// If the node used to have focus, make sure it keeps it's old primary
// focus when it moves.
oldPrimaryFocus.requestFocus();
} }
} }
...@@ -792,18 +781,10 @@ class FocusScopeNode extends FocusNode { ...@@ -792,18 +781,10 @@ class FocusScopeNode extends FocusNode {
_reparent(scope); _reparent(scope);
} }
assert(scope.ancestors.contains(this), '$FocusScopeNode $scope must be a child of $this to set it as first focus.'); assert(scope.ancestors.contains(this), '$FocusScopeNode $scope must be a child of $this to set it as first focus.');
// Move down the tree, checking each focusedChild until we get to a node if (hasFocus) {
// that either isn't a scope node, or has no focused child, and then request
// focus on that node.
FocusNode descendantFocus = scope.focusedChild;
while (descendantFocus is FocusScopeNode && descendantFocus != null) {
final FocusScopeNode descendantScope = descendantFocus;
descendantFocus = descendantScope.focusedChild;
}
if (descendantFocus != null) {
descendantFocus?._doRequestFocus(isFromPolicy: false);
} else {
scope._doRequestFocus(isFromPolicy: false); scope._doRequestFocus(isFromPolicy: false);
} else {
scope._setAsFocusedChild();
} }
} }
...@@ -843,6 +824,7 @@ class FocusScopeNode extends FocusNode { ...@@ -843,6 +824,7 @@ class FocusScopeNode extends FocusNode {
} }
if (primaryFocus is FocusScopeNode) { if (primaryFocus is FocusScopeNode) {
// We didn't find a FocusNode at the leaf, so we're focusing the scope. // We didn't find a FocusNode at the leaf, so we're focusing the scope.
_setAsFocusedChild();
_markAsDirty(newFocus: primaryFocus); _markAsDirty(newFocus: primaryFocus);
} else { } else {
primaryFocus.requestFocus(); primaryFocus.requestFocus();
......
...@@ -273,21 +273,21 @@ void main() { ...@@ -273,21 +273,21 @@ void main() {
}); });
testWidgets('Can move node between scopes and lose scope focus', (WidgetTester tester) async { testWidgets('Can move node between scopes and lose scope focus', (WidgetTester tester) async {
final BuildContext context = await setupWidget(tester); final BuildContext context = await setupWidget(tester);
final FocusScopeNode scope1 = FocusScopeNode()..attach(context); final FocusScopeNode scope1 = FocusScopeNode(debugLabel: 'scope1')..attach(context);
final FocusAttachment scope1Attachment = scope1.attach(context); final FocusAttachment scope1Attachment = scope1.attach(context);
final FocusScopeNode scope2 = FocusScopeNode(); final FocusScopeNode scope2 = FocusScopeNode(debugLabel: 'scope2');
final FocusAttachment scope2Attachment = scope2.attach(context); final FocusAttachment scope2Attachment = scope2.attach(context);
final FocusNode parent1 = FocusNode(); final FocusNode parent1 = FocusNode(debugLabel: 'parent1');
final FocusAttachment parent1Attachment = parent1.attach(context); final FocusAttachment parent1Attachment = parent1.attach(context);
final FocusNode parent2 = FocusNode(); final FocusNode parent2 = FocusNode(debugLabel: 'parent2');
final FocusAttachment parent2Attachment = parent2.attach(context); final FocusAttachment parent2Attachment = parent2.attach(context);
final FocusNode child1 = FocusNode(); final FocusNode child1 = FocusNode(debugLabel: 'child1');
final FocusAttachment child1Attachment = child1.attach(context); final FocusAttachment child1Attachment = child1.attach(context);
final FocusNode child2 = FocusNode(); final FocusNode child2 = FocusNode(debugLabel: 'child2');
final FocusAttachment child2Attachment = child2.attach(context); final FocusAttachment child2Attachment = child2.attach(context);
final FocusNode child3 = FocusNode(); final FocusNode child3 = FocusNode(debugLabel: 'child3');
final FocusAttachment child3Attachment = child3.attach(context); final FocusAttachment child3Attachment = child3.attach(context);
final FocusNode child4 = FocusNode(); final FocusNode child4 = FocusNode(debugLabel: 'child4');
final FocusAttachment child4Attachment = child4.attach(context); final FocusAttachment child4Attachment = child4.attach(context);
scope1Attachment.reparent(parent: tester.binding.focusManager.rootScope); scope1Attachment.reparent(parent: tester.binding.focusManager.rootScope);
scope2Attachment.reparent(parent: tester.binding.focusManager.rootScope); scope2Attachment.reparent(parent: tester.binding.focusManager.rootScope);
...@@ -365,6 +365,55 @@ void main() { ...@@ -365,6 +365,55 @@ void main() {
expect(scope1.focusedChild, equals(child1)); expect(scope1.focusedChild, equals(child1));
expect(scope2.focusedChild, equals(child4)); expect(scope2.focusedChild, equals(child4));
}); });
testWidgets('Unfocus works properly', (WidgetTester tester) async {
final BuildContext context = await setupWidget(tester);
final FocusScopeNode scope1 = FocusScopeNode()..attach(context);
final FocusAttachment scope1Attachment = scope1.attach(context);
final FocusScopeNode scope2 = FocusScopeNode();
final FocusAttachment scope2Attachment = scope2.attach(context);
final FocusNode parent1 = FocusNode();
final FocusAttachment parent1Attachment = parent1.attach(context);
final FocusNode parent2 = FocusNode();
final FocusAttachment parent2Attachment = parent2.attach(context);
final FocusNode child1 = FocusNode();
final FocusAttachment child1Attachment = child1.attach(context);
final FocusNode child2 = FocusNode();
final FocusAttachment child2Attachment = child2.attach(context);
final FocusNode child3 = FocusNode();
final FocusAttachment child3Attachment = child3.attach(context);
final FocusNode child4 = FocusNode();
final FocusAttachment child4Attachment = child4.attach(context);
scope1Attachment.reparent(parent: tester.binding.focusManager.rootScope);
scope2Attachment.reparent(parent: tester.binding.focusManager.rootScope);
parent1Attachment.reparent(parent: scope1);
parent2Attachment.reparent(parent: scope2);
child1Attachment.reparent(parent: parent1);
child2Attachment.reparent(parent: parent1);
child3Attachment.reparent(parent: parent2);
child4Attachment.reparent(parent: parent2);
child1.requestFocus();
await tester.pump();
expect(scope1.focusedChild, equals(child1));
expect(parent2.children.contains(child1), isFalse);
child1.unfocus();
await tester.pump();
expect(scope1.focusedChild, isNull);
expect(child1.hasPrimaryFocus, isFalse);
expect(scope1.hasFocus, isFalse);
child1.requestFocus();
await tester.pump();
expect(scope1.focusedChild, equals(child1));
expect(parent2.children.contains(child1), isFalse);
scope1.unfocus();
await tester.pump();
expect(scope1.focusedChild, isNull);
expect(child1.hasPrimaryFocus, isFalse);
expect(scope1.hasFocus, isFalse);
});
testWidgets('Key handling bubbles up and terminates when handled.', (WidgetTester tester) async { testWidgets('Key handling bubbles up and terminates when handled.', (WidgetTester tester) async {
final Set<FocusNode> receivedAnEvent = <FocusNode>{}; final Set<FocusNode> receivedAnEvent = <FocusNode>{};
final Set<FocusNode> shouldHandle = <FocusNode>{}; final Set<FocusNode> shouldHandle = <FocusNode>{};
......
...@@ -22,42 +22,46 @@ class TestFocus extends StatefulWidget { ...@@ -22,42 +22,46 @@ class TestFocus extends StatefulWidget {
} }
class TestFocusState extends State<TestFocus> { class TestFocusState extends State<TestFocus> {
FocusNode focusNode = FocusNode(); FocusNode focusNode;
FocusAttachment focusAttachment; String _label;
bool _didAutofocus = false;
@override @override
void dispose() { void dispose() {
focusNode.dispose(); focusNode.removeListener(_updateLabel);
focusNode?.dispose();
super.dispose(); super.dispose();
} }
String get label => focusNode.hasFocus ? '${widget.name.toUpperCase()} FOCUSED' : widget.name.toLowerCase();
@override @override
void initState() { void initState() {
super.initState(); super.initState();
focusNode = FocusNode(debugLabel: widget.debugLabel); focusNode = FocusNode(debugLabel: widget.debugLabel);
focusAttachment = focusNode.attach(context); _label = label;
focusNode.addListener(_updateLabel);
}
void _updateLabel() {
setState(() {
_label = label;
});
} }
@override @override
Widget build(BuildContext context) { Widget build(BuildContext context) {
focusAttachment.reparent();
if (!_didAutofocus && widget.autofocus) {
_didAutofocus = true;
FocusScope.of(context).autofocus(focusNode);
}
return GestureDetector( return GestureDetector(
onTap: () { onTap: () {
FocusScope.of(context).requestFocus(focusNode); FocusScope.of(context).requestFocus(focusNode);
}, },
child: AnimatedBuilder( child: Focus(
animation: focusNode, autofocus: widget.autofocus,
builder: (BuildContext context, Widget child) { focusNode: focusNode,
return Text( debugLabel: widget.debugLabel,
focusNode.hasFocus ? '${widget.name.toUpperCase()} FOCUSED' : widget.name.toLowerCase(), child: Text(
textDirection: TextDirection.ltr, _label,
); textDirection: TextDirection.ltr,
}, ),
), ),
); );
} }
...@@ -116,6 +120,26 @@ void main() { ...@@ -116,6 +120,26 @@ void main() {
expect(find.text('B FOCUSED'), findsOneWidget); expect(find.text('B FOCUSED'), findsOneWidget);
}); });
testWidgets('Autofocus works', (WidgetTester tester) async {
final GlobalKey<TestFocusState> keyA = GlobalKey();
final GlobalKey<TestFocusState> keyB = GlobalKey();
await tester.pumpWidget(
Column(
children: <Widget>[
TestFocus(key: keyA, name: 'a'),
TestFocus(key: keyB, name: 'b', autofocus: true),
],
),
);
await tester.pump();
expect(keyA.currentState.focusNode.hasFocus, isFalse);
expect(find.text('a'), findsOneWidget);
expect(keyB.currentState.focusNode.hasFocus, isTrue);
expect(find.text('B FOCUSED'), findsOneWidget);
});
testWidgets('Can have multiple focused children and they update accordingly', (WidgetTester tester) async { testWidgets('Can have multiple focused children and they update accordingly', (WidgetTester tester) async {
final GlobalKey<TestFocusState> keyA = GlobalKey(); final GlobalKey<TestFocusState> keyA = GlobalKey();
final GlobalKey<TestFocusState> keyB = GlobalKey(); final GlobalKey<TestFocusState> keyB = GlobalKey();
...@@ -205,7 +229,7 @@ void main() { ...@@ -205,7 +229,7 @@ void main() {
' │ focusedChild: FocusNode#00000\n' ' │ focusedChild: FocusNode#00000\n'
' │\n' ' │\n'
' └─Child 1: FocusNode#00000\n' ' └─Child 1: FocusNode#00000\n'
' context: TestFocus-[LabeledGlobalKey<TestFocusState>#00000]\n' ' context: Focus\n'
' FOCUSED\n' ' FOCUSED\n'
' debugLabel: "Child"\n'), ' debugLabel: "Child"\n'),
); );
...@@ -225,13 +249,13 @@ void main() { ...@@ -225,13 +249,13 @@ void main() {
' │ focusedChild: FocusNode#00000\n' ' │ focusedChild: FocusNode#00000\n'
' │\n' ' │\n'
' └─Child 1: FocusNode#00000\n' ' └─Child 1: FocusNode#00000\n'
' context: TestFocus-[LabeledGlobalKey<TestFocusState>#00000]\n' ' context: Focus\n'
' FOCUSED\n' ' FOCUSED\n'
' debugLabel: "Child"\n'), ' debugLabel: "Child"\n'),
); );
// Add the child focus scope to the focus tree. // Add the child focus scope to the focus tree.
final FocusAttachment childAttachment = childFocusScope.attach(key.currentContext); final FocusAttachment childAttachment = childFocusScope.attach(key.currentContext);
parentFocusScope.setFirstFocus(childFocusScope); parentFocusScope.setFirstFocus(childFocusScope);
await tester.pumpAndSettle(); await tester.pumpAndSettle();
expect(childFocusScope.isFirstFocus, isTrue); expect(childFocusScope.isFirstFocus, isTrue);
...@@ -310,8 +334,110 @@ void main() { ...@@ -310,8 +334,110 @@ void main() {
childAttachment.detach(); childAttachment.detach();
}); });
// Arguably, this isn't correct behavior, but it is what happens now. testWidgets('Setting first focus requests focus for the scope properly.', (WidgetTester tester) async {
testWidgets("Removing focused widget doesn't move focus to next widget", (WidgetTester tester) async { final FocusScopeNode parentFocusScope = FocusScopeNode(debugLabel: 'Parent Scope Node');
final FocusScopeNode childFocusScope1 = FocusScopeNode(debugLabel: 'Child Scope Node 1');
final FocusScopeNode childFocusScope2 = FocusScopeNode(debugLabel: 'Child Scope Node 2');
final GlobalKey<TestFocusState> keyA = GlobalKey(debugLabel: 'Key A');
final GlobalKey<TestFocusState> keyB = GlobalKey(debugLabel: 'Key B');
final GlobalKey<TestFocusState> keyC = GlobalKey(debugLabel: 'Key C');
await tester.pumpWidget(
FocusScope(
debugLabel: 'Parent Scope',
node: parentFocusScope,
child: Column(
children: <Widget>[
FocusScope(
debugLabel: 'Child Scope 1',
node: childFocusScope1,
child: Column(
children: <Widget>[
TestFocus(
key: keyA,
name: 'a',
autofocus: true,
debugLabel: 'Child A',
),
TestFocus(
key: keyB,
name: 'b',
debugLabel: 'Child B',
),
],
),
),
FocusScope(
debugLabel: 'Child Scope 2',
node: childFocusScope2,
child: TestFocus(
key: keyC,
name: 'c',
debugLabel: 'Child C',
),
),
],
),
),
);
await tester.pumpAndSettle();
expect(keyA.currentState.focusNode.hasFocus, isTrue);
expect(find.text('A FOCUSED'), findsOneWidget);
parentFocusScope.setFirstFocus(childFocusScope2);
await tester.pumpAndSettle();
expect(keyA.currentState.focusNode.hasFocus, isFalse);
expect(find.text('a'), findsOneWidget);
parentFocusScope.setFirstFocus(childFocusScope1);
await tester.pumpAndSettle();
expect(keyA.currentState.focusNode.hasFocus, isTrue);
expect(find.text('A FOCUSED'), findsOneWidget);
keyB.currentState.focusNode.requestFocus();
await tester.pumpAndSettle();
expect(keyB.currentState.focusNode.hasFocus, isTrue);
expect(find.text('B FOCUSED'), findsOneWidget);
expect(parentFocusScope.isFirstFocus, isTrue);
expect(childFocusScope1.isFirstFocus, isTrue);
parentFocusScope.setFirstFocus(childFocusScope2);
await tester.pumpAndSettle();
expect(keyB.currentState.focusNode.hasFocus, isFalse);
expect(find.text('b'), findsOneWidget);
expect(parentFocusScope.isFirstFocus, isTrue);
expect(childFocusScope1.isFirstFocus, isFalse);
expect(childFocusScope2.isFirstFocus, isTrue);
keyC.currentState.focusNode.requestFocus();
await tester.pumpAndSettle();
expect(keyB.currentState.focusNode.hasFocus, isFalse);
expect(find.text('b'), findsOneWidget);
expect(keyC.currentState.focusNode.hasFocus, isTrue);
expect(find.text('C FOCUSED'), findsOneWidget);
expect(parentFocusScope.isFirstFocus, isTrue);
expect(childFocusScope1.isFirstFocus, isFalse);
expect(childFocusScope2.isFirstFocus, isTrue);
childFocusScope1.requestFocus();
await tester.pumpAndSettle();
expect(keyB.currentState.focusNode.hasFocus, isTrue);
expect(find.text('B FOCUSED'), findsOneWidget);
expect(keyC.currentState.focusNode.hasFocus, isFalse);
expect(find.text('c'), findsOneWidget);
expect(parentFocusScope.isFirstFocus, isTrue);
expect(childFocusScope1.isFirstFocus, isTrue);
expect(childFocusScope2.isFirstFocus, isFalse);
});
testWidgets('Removing focused widget moves focus to next widget', (WidgetTester tester) async {
final GlobalKey<TestFocusState> keyA = GlobalKey(); final GlobalKey<TestFocusState> keyA = GlobalKey();
final GlobalKey<TestFocusState> keyB = GlobalKey(); final GlobalKey<TestFocusState> keyB = GlobalKey();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment