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

Fix route focusing and autofocus when reparenting focus nodes. (#42554)

This fixes a problem with reparenting of focus nodes where it would remove the node from the scope's focused children when reparented, even if it was being reparented to another place under the same scope. This only occurred if the scope in question didn't have focus.

This caused nodes to not get autofocused when they were a child of another node within the scope, and were reparented, and then the scope was given focus (as when a route is pushed).

This keeps the node in the scope's child list where it was if the node is reparented under a parent within the same scope.

- Added a test that tries to autofocus a TextField when the route is pushed and there is another 
  FocusNode above the text field. This was how this was first noticed: the autofocus got ignored in 
  this configuration.

- Added a test to focus_manager_test that tests for the specific case of reparenting a node when 
  it's in the focused children of the scope.
parent a4947055
......@@ -698,13 +698,15 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
// Removes the given FocusNode and its children as a child of this node.
@mustCallSuper
void _removeChild(FocusNode node) {
void _removeChild(FocusNode node, {bool removeScopeFocus = true}) {
assert(node != null);
assert(_children.contains(node), "Tried to remove a node that wasn't a child.");
assert(node._parent == this);
assert(node._manager == _manager);
node.enclosingScope?._focusedChildren?.remove(node);
if (removeScopeFocus) {
node.enclosingScope?._focusedChildren?.remove(node);
}
node._parent = null;
_children.remove(node);
......@@ -732,7 +734,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
assert(!ancestors.contains(child), 'The supplied child is already an ancestor of this node. Loops are not allowed.');
final FocusScopeNode oldScope = child.enclosingScope;
final bool hadFocus = child.hasFocus;
child._parent?._removeChild(child);
child._parent?._removeChild(child, removeScopeFocus: oldScope != nearestScope);
_children.add(child);
child._parent = this;
child._updateManager(_manager);
......@@ -813,6 +815,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
// Note that this is overridden in FocusScopeNode.
void _doRequestFocus() {
if (!canRequestFocus) {
assert(_focusDebug('Node NOT requesting focus because canRequestFocus is false: $this'));
return;
}
_setAsFocusedChild();
......@@ -820,6 +823,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
return;
}
_hasKeyboardToken = true;
assert(_focusDebug('Node requesting focus: $this'));
_markAsDirty(newFocus: this);
}
......@@ -836,6 +840,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
FocusNode scopeFocus = this;
for (FocusScopeNode ancestor in ancestors.whereType<FocusScopeNode>()) {
assert(scopeFocus != ancestor, 'Somehow made a loop by setting focusedChild to its scope.');
assert(_focusDebug('Setting $scopeFocus as focused child for scope:', <String>[ancestor.toString()]));
// Remove it anywhere in the focused child history.
ancestor._focusedChildren.remove(scopeFocus);
// Add it to the end of the list, which is also the top of the queue: The
......@@ -879,6 +884,11 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
return child.toDiagnosticsNode(name: 'Child ${count++}');
}).toList();
}
@override
String toStringShort() {
return '${describeIdentity(this)}${debugLabel != null && debugLabel.isNotEmpty ? '($debugLabel)' : ''}';
}
}
/// A subclass of [FocusNode] that acts as a scope for its descendants,
......@@ -950,6 +960,7 @@ class FocusScopeNode extends FocusNode {
void setFirstFocus(FocusScopeNode scope) {
assert(scope != null);
assert(scope != this, 'Unexpected self-reference in setFirstFocus.');
assert(_focusDebug('Setting scope as first focus in $this to node:', <String>[scope.toString()]));
if (scope._parent == null) {
_reparent(scope);
}
......@@ -972,6 +983,7 @@ class FocusScopeNode extends FocusNode {
/// The node is notified that it has received the primary focus in a
/// microtask, so notification may lag the request by up to one frame.
void autofocus(FocusNode node) {
assert(_focusDebug('Node autofocusing: $node'));
if (focusedChild == null) {
if (node._parent == null) {
_reparent(node);
......@@ -1015,7 +1027,7 @@ class FocusScopeNode extends FocusNode {
return;
}
final List<String> childList = _focusedChildren.reversed.map<String>((FocusNode child) {
return '${describeIdentity(child)}${child.debugLabel != null && child.debugLabel.isNotEmpty ? '(${child.debugLabel})' : ''}';
return child.toStringShort();
}).toList();
properties.add(IterableProperty<String>('focusedChildren', childList, defaultValue: <String>[]));
}
......
......@@ -555,23 +555,23 @@ void main() {
description,
equalsIgnoringHashCodes(
'FocusManager#00000\n'
' │ primaryFocus: FocusNode#00000\n'
' │ primaryFocus: FocusNode#00000(Child 4)\n'
' │ primaryFocusCreator: Container-[GlobalKey#00000] ← [root]\n'
' │\n'
' └─rootScope: FocusScopeNode#00000\n'
' └─rootScope: FocusScopeNode#00000(Root Focus Scope)\n'
' │ FOCUSED\n'
' │ debugLabel: "Root Focus Scope"\n'
' │ focusedChildren: FocusScopeNode#00000\n'
' │\n'
' ├─Child 1: FocusScopeNode#00000\n'
' ├─Child 1: FocusScopeNode#00000(Scope 1)\n'
' │ │ context: Container-[GlobalKey#00000]\n'
' │ │ debugLabel: "Scope 1"\n'
' │ │\n'
' │ └─Child 1: FocusNode#00000\n'
' │ └─Child 1: FocusNode#00000(Parent 1)\n'
' │ │ context: Container-[GlobalKey#00000]\n'
' │ │ debugLabel: "Parent 1"\n'
' │ │\n'
' │ ├─Child 1: FocusNode#00000\n'
' │ ├─Child 1: FocusNode#00000(Child 1)\n'
' │ │ context: Container-[GlobalKey#00000]\n'
' │ │ debugLabel: "Child 1"\n'
' │ │\n'
......@@ -583,20 +583,45 @@ void main() {
' │ FOCUSED\n'
' │ focusedChildren: FocusNode#00000(Child 4)\n'
' │\n'
' └─Child 1: FocusNode#00000\n'
' └─Child 1: FocusNode#00000(Parent 2)\n'
' │ context: Container-[GlobalKey#00000]\n'
' │ FOCUSED\n'
' │ debugLabel: "Parent 2"\n'
' │\n'
' ├─Child 1: FocusNode#00000\n'
' ├─Child 1: FocusNode#00000(Child 3)\n'
' │ context: Container-[GlobalKey#00000]\n'
' │ debugLabel: "Child 3"\n'
' │\n'
' └─Child 2: FocusNode#00000\n'
' └─Child 2: FocusNode#00000(Child 4)\n'
' context: Container-[GlobalKey#00000]\n'
' FOCUSED\n'
' debugLabel: "Child 4"\n'
));
});
});
testWidgets("Doesn't lose focused child when reparenting if the nearestScope doesn't change.", (WidgetTester tester) async {
final BuildContext context = await setupWidget(tester);
final FocusScopeNode parent1 = FocusScopeNode(debugLabel: 'parent1');
final FocusScopeNode parent2 = FocusScopeNode(debugLabel: 'parent2');
final FocusAttachment parent1Attachment = parent1.attach(context);
final FocusAttachment parent2Attachment = parent2.attach(context);
final FocusNode child1 = FocusNode(debugLabel: 'child1');
final FocusAttachment child1Attachment = child1.attach(context);
final FocusNode child2 = FocusNode(debugLabel: 'child2');
final FocusAttachment child2Attachment = child2.attach(context);
parent1Attachment.reparent(parent: tester.binding.focusManager.rootScope);
child1Attachment.reparent(parent: parent1);
child2Attachment.reparent(parent: child1);
parent1.autofocus(child2);
await tester.pump();
parent2Attachment.reparent(parent: tester.binding.focusManager.rootScope);
parent2.requestFocus();
await tester.pump();
expect(parent1.focusedChild, equals(child2));
child2Attachment.reparent(parent: parent1);
expect(parent1.focusedChild, equals(child2));
parent1.requestFocus();
await tester.pump();
expect(parent1.focusedChild, equals(child2));
});
}
......@@ -227,13 +227,13 @@ void main() {
expect(parentFocusScope, hasAGoodToStringDeep);
expect(
parentFocusScope.toStringDeep(),
equalsIgnoringHashCodes('FocusScopeNode#00000\n'
equalsIgnoringHashCodes('FocusScopeNode#00000(Parent Scope Node)\n'
' │ context: FocusScope\n'
' │ FOCUSED\n'
' │ debugLabel: "Parent Scope Node"\n'
' │ focusedChildren: FocusNode#00000(Child)\n'
' │\n'
' └─Child 1: FocusNode#00000\n'
' └─Child 1: FocusNode#00000(Child)\n'
' context: Focus\n'
' FOCUSED\n'
' debugLabel: "Child"\n'),
......@@ -242,18 +242,18 @@ void main() {
expect(WidgetsBinding.instance.focusManager.rootScope, hasAGoodToStringDeep);
expect(
WidgetsBinding.instance.focusManager.rootScope.toStringDeep(minLevel: DiagnosticLevel.info),
equalsIgnoringHashCodes('FocusScopeNode#00000\n'
equalsIgnoringHashCodes('FocusScopeNode#00000(Root Focus Scope)\n'
' │ FOCUSED\n'
' │ debugLabel: "Root Focus Scope"\n'
' │ focusedChildren: FocusScopeNode#00000(Parent Scope Node)\n'
' │\n'
' └─Child 1: FocusScopeNode#00000\n'
' └─Child 1: FocusScopeNode#00000(Parent Scope Node)\n'
' │ context: FocusScope\n'
' │ FOCUSED\n'
' │ debugLabel: "Parent Scope Node"\n'
' │ focusedChildren: FocusNode#00000(Child)\n'
' │\n'
' └─Child 1: FocusNode#00000\n'
' └─Child 1: FocusNode#00000(Child)\n'
' context: Focus\n'
' FOCUSED\n'
' debugLabel: "Child"\n'),
......
......@@ -5,6 +5,7 @@
import 'dart:collection';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:mockito/mockito.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:flutter/widgets.dart';
......@@ -503,6 +504,34 @@ void main() {
verifyNoMoreInteractions(pageRouteAware);
});
});
testWidgets('Can autofocus a TextField nested in a Focus in a route.', (WidgetTester tester) async {
final TextEditingController controller = TextEditingController();
final FocusNode focusNode = FocusNode(debugLabel: 'Test Node');
await tester.pumpWidget(
Material(
child: MaterialApp(
onGenerateRoute: (RouteSettings settings) {
return PageRouteBuilder<void>(
settings: settings,
pageBuilder: (BuildContext context, Animation<double> input, Animation<double> out) {
return Focus(
child: TextField(
autofocus: true,
focusNode: focusNode,
controller: controller,
),
);
},
);
},
),
),
);
await tester.pump();
expect(focusNode.hasPrimaryFocus, isTrue);
});
}
class MockPageRoute extends Mock implements PageRoute<dynamic> { }
......
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