Unverified Commit 134ac429 authored by Greg Spencer's avatar Greg Spencer Committed by GitHub

Optimize focus operations by caching descendants and ancestors. (#42683)

This optimizes certain paths in the FocusManager, FocusNode, and FocusScopeNode classes in order to fix a regression in stock_animation_open_first_frame_average when I added more focus nodes to the tree to do focus traversal.

Mainly I removed some remaining sync* iterators, and also started caching the computation of descendants and ancestors, since those are iterated over fairly often.

This improves stock_animation_open_first_frame_average by about 2.8% overall (so about half of a 4.9% regression, both averaged over 10 runs).

Addresses #42564
parent 6b2cc855
...@@ -489,20 +489,20 @@ class _InkResponseState<T extends InkResponse> extends State<T> with AutomaticKe ...@@ -489,20 +489,20 @@ class _InkResponseState<T extends InkResponse> extends State<T> with AutomaticKe
bool get highlightsExist => _highlights.values.where((InkHighlight highlight) => highlight != null).isNotEmpty; bool get highlightsExist => _highlights.values.where((InkHighlight highlight) => highlight != null).isNotEmpty;
Action _createAction() {
return CallbackAction(
ActivateAction.key,
onInvoke: (FocusNode node, Intent intent) {
_startSplash(context: node.context);
_handleTap(node.context);
},
);
}
@override @override
void initState() { void initState() {
super.initState(); super.initState();
_actionMap = <LocalKey, ActionFactory>{ _actionMap = <LocalKey, ActionFactory>{ ActivateAction.key: _createAction };
ActivateAction.key: () {
return CallbackAction(
ActivateAction.key,
onInvoke: (FocusNode node, Intent intent) {
_startSplash(context: node.context);
_handleTap(node.context);
},
);
},
};
WidgetsBinding.instance.focusManager.addHighlightModeListener(_handleFocusHighlightModeChange); WidgetsBinding.instance.focusManager.addHighlightModeListener(_handleFocusHighlightModeChange);
} }
......
...@@ -119,8 +119,8 @@ class FocusAttachment { ...@@ -119,8 +119,8 @@ class FocusAttachment {
assert(_node != null); assert(_node != null);
if (isAttached) { if (isAttached) {
assert(_node.context != null); assert(_node.context != null);
parent ??= Focus.of(_node.context, nullOk: true); parent ??= Focus.of(_node.context, nullOk: true, scopeOk: true);
parent ??= FocusScope.of(_node.context); parent ??= _node.context.owner.focusManager.rootScope;
assert(parent != null); assert(parent != null);
parent._reparent(_node); parent._reparent(_node);
} }
...@@ -421,7 +421,10 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -421,7 +421,10 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
/// its descendants. /// its descendants.
/// - [FocusTraversalPolicy], a class that can be extended to describe a /// - [FocusTraversalPolicy], a class that can be extended to describe a
/// traversal policy. /// traversal policy.
bool get canRequestFocus => _canRequestFocus && (enclosingScope == null || enclosingScope.canRequestFocus); bool get canRequestFocus {
final FocusScopeNode scope = enclosingScope;
return _canRequestFocus && (scope == null || scope.canRequestFocus);
}
bool _canRequestFocus; bool _canRequestFocus;
@mustCallSuper @mustCallSuper
set canRequestFocus(bool value) { set canRequestFocus(bool value) {
...@@ -450,6 +453,8 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -450,6 +453,8 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
FocusOnKeyCallback _onKey; FocusOnKeyCallback _onKey;
FocusManager _manager; FocusManager _manager;
List<FocusNode> _ancestors;
List<FocusNode> _descendants;
bool _hasKeyboardToken = false; bool _hasKeyboardToken = false;
/// Returns the parent node for this object. /// Returns the parent node for this object.
...@@ -471,7 +476,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -471,7 +476,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
return const <FocusNode>[]; return const <FocusNode>[];
} }
return children.where( return children.where(
(FocusNode node) => !node.skipTraversal && node._canRequestFocus, (FocusNode node) => !node.skipTraversal && node.canRequestFocus,
); );
} }
...@@ -492,11 +497,16 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -492,11 +497,16 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
/// An [Iterable] over the hierarchy of children below this one, in /// An [Iterable] over the hierarchy of children below this one, in
/// depth-first order. /// depth-first order.
Iterable<FocusNode> get descendants sync* { Iterable<FocusNode> get descendants {
for (FocusNode child in _children) { if (_descendants == null) {
yield* child.descendants; final List<FocusNode> result = <FocusNode>[];
yield child; for (FocusNode child in _children) {
result.addAll(child.descendants);
result.add(child);
}
_descendants = result;
} }
return _descendants;
} }
/// Returns all descendants which do not have the [skipTraversal] flag set. /// Returns all descendants which do not have the [skipTraversal] flag set.
...@@ -507,12 +517,17 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -507,12 +517,17 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
/// Iterates the ancestors of this node starting at the parent and iterating /// Iterates the ancestors of this node starting at the parent and iterating
/// over successively more remote ancestors of this node, ending at the root /// over successively more remote ancestors of this node, ending at the root
/// [FocusScope] ([FocusManager.rootScope]). /// [FocusScope] ([FocusManager.rootScope]).
Iterable<FocusNode> get ancestors sync* { Iterable<FocusNode> get ancestors {
FocusNode parent = _parent; if (_ancestors == null) {
while (parent != null) { final List<FocusNode> result = <FocusNode>[];
yield parent; FocusNode parent = _parent;
parent = parent._parent; while (parent != null) {
result.add(parent);
parent = parent._parent;
}
_ancestors = result;
} }
return _ancestors;
} }
/// Whether this node has input focus. /// Whether this node has input focus.
...@@ -710,6 +725,10 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -710,6 +725,10 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
node._parent = null; node._parent = null;
_children.remove(node); _children.remove(node);
for (FocusNode ancestor in ancestors) {
ancestor._descendants = null;
}
_descendants = null;
assert(_manager == null || !_manager.rootScope.descendants.contains(node)); assert(_manager == null || !_manager.rootScope.descendants.contains(node));
} }
...@@ -717,6 +736,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -717,6 +736,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
_manager = manager; _manager = manager;
for (FocusNode descendant in descendants) { for (FocusNode descendant in descendants) {
descendant._manager = manager; descendant._manager = manager;
descendant._ancestors = null;
} }
} }
...@@ -737,7 +757,11 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -737,7 +757,11 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
child._parent?._removeChild(child, removeScopeFocus: oldScope != nearestScope); child._parent?._removeChild(child, removeScopeFocus: oldScope != nearestScope);
_children.add(child); _children.add(child);
child._parent = this; child._parent = this;
child._ancestors = null;
child._updateManager(_manager); child._updateManager(_manager);
for (FocusNode ancestor in child.ancestors) {
ancestor._descendants = null;
}
if (hadFocus) { if (hadFocus) {
// Update the focus chain for the current focus without changing it. // Update the focus chain for the current focus without changing it.
_manager?.primaryFocus?._setAsFocusedChild(); _manager?.primaryFocus?._setAsFocusedChild();
...@@ -1266,15 +1290,8 @@ class FocusManager with DiagnosticableTreeMixin { ...@@ -1266,15 +1290,8 @@ class FocusManager with DiagnosticableTreeMixin {
assert(_focusDebug('No primary focus for key event, ignored: $event')); assert(_focusDebug('No primary focus for key event, ignored: $event'));
return; return;
} }
Iterable<FocusNode> allNodes(FocusNode node) sync* {
yield node;
for (FocusNode ancestor in node.ancestors) {
yield ancestor;
}
}
bool handled = false; bool handled = false;
for (FocusNode node in allNodes(_primaryFocus)) { for (FocusNode node in <FocusNode>[_primaryFocus, ..._primaryFocus.ancestors]) {
if (node.onKey != null && node.onKey(node, event)) { if (node.onKey != null && node.onKey(node, event)) {
assert(_focusDebug('Node $node handled key event $event.')); assert(_focusDebug('Node $node handled key event $event.'));
handled = true; handled = true;
...@@ -1338,7 +1355,7 @@ class FocusManager with DiagnosticableTreeMixin { ...@@ -1338,7 +1355,7 @@ class FocusManager with DiagnosticableTreeMixin {
final FocusNode previousFocus = _primaryFocus; final FocusNode previousFocus = _primaryFocus;
if (_primaryFocus == null && _nextFocus == null) { if (_primaryFocus == null && _nextFocus == null) {
// If we don't have any current focus, and nobody has asked to focus yet, // If we don't have any current focus, and nobody has asked to focus yet,
// then pick a first one using widget order as a default. // then revert to the root scope.
_nextFocus = rootScope; _nextFocus = rootScope;
} }
if (_nextFocus != null && _nextFocus != _primaryFocus) { if (_nextFocus != null && _nextFocus != _primaryFocus) {
......
...@@ -262,33 +262,34 @@ class Focus extends StatefulWidget { ...@@ -262,33 +262,34 @@ class Focus extends StatefulWidget {
/// [nullOk]. /// [nullOk].
/// ///
/// The [context] and [nullOk] arguments must not be null. /// The [context] and [nullOk] arguments must not be null.
static FocusNode of(BuildContext context, { bool nullOk = false }) { static FocusNode of(BuildContext context, { bool nullOk = false, bool scopeOk = false }) {
assert(context != null); assert(context != null);
assert(nullOk != null); assert(nullOk != null);
assert(scopeOk != null);
final _FocusMarker marker = context.inheritFromWidgetOfExactType(_FocusMarker); final _FocusMarker marker = context.inheritFromWidgetOfExactType(_FocusMarker);
final FocusNode node = marker?.notifier; final FocusNode node = marker?.notifier;
if (node is FocusScopeNode) { if (node == null) {
if (!nullOk) { if (!nullOk) {
throw FlutterError( throw FlutterError(
'Focus.of() was called with a context that does not contain a Focus between the given ' 'Focus.of() was called with a context that does not contain a Focus widget.\n'
'context and the nearest FocusScope widget.\n' 'No Focus widget ancestor could be found starting from the context that was passed to '
'No Focus ancestor could be found starting from the context that was passed to ' 'Focus.of(). This can happen because you are using a widget that looks for a Focus '
'Focus.of() to the point where it found the nearest FocusScope widget. This can happen ' 'ancestor, and do not have a Focus widget descendant in the nearest FocusScope.\n'
'because you are using a widget that looks for a Focus ancestor, and do not have a ' 'The context used was:\n'
'Focus widget ancestor in the current FocusScope.\n' ' $context'
'The context used was:\n'
' $context'
); );
} }
return null; return null;
} }
if (node == null) { if (!scopeOk && node is FocusScopeNode) {
if (!nullOk) { if (!nullOk) {
throw FlutterError( throw FlutterError(
'Focus.of() was called with a context that does not contain a Focus widget.\n' 'Focus.of() was called with a context that does not contain a Focus between the given '
'No Focus widget ancestor could be found starting from the context that was passed to ' 'context and the nearest FocusScope widget.\n'
'Focus.of(). This can happen because you are using a widget that looks for a Focus ' 'No Focus ancestor could be found starting from the context that was passed to '
'ancestor, and do not have a Focus widget descendant in the nearest FocusScope.\n' 'Focus.of() to the point where it found the nearest FocusScope widget. This can happen '
'because you are using a widget that looks for a Focus ancestor, and do not have a '
'Focus widget ancestor in the current FocusScope.\n'
'The context used was:\n' 'The context used was:\n'
' $context' ' $context'
); );
......
...@@ -300,6 +300,41 @@ void main() { ...@@ -300,6 +300,41 @@ void main() {
expect(scope1.focusedChild, isNull); expect(scope1.focusedChild, isNull);
expect(parent2.children.contains(child1), isTrue); expect(parent2.children.contains(child1), isTrue);
}); });
testWidgets('ancestors and descendants are computed and recomputed properly', (WidgetTester tester) async {
final BuildContext context = await setupWidget(tester);
final FocusScopeNode scope1 = FocusScopeNode(debugLabel: 'scope1');
final FocusAttachment scope1Attachment = scope1.attach(context);
final FocusScopeNode scope2 = FocusScopeNode(debugLabel: 'scope2');
final FocusAttachment scope2Attachment = scope2.attach(context);
final FocusNode parent1 = FocusNode(debugLabel: 'parent1');
final FocusAttachment parent1Attachment = parent1.attach(context);
final FocusNode parent2 = FocusNode(debugLabel: 'parent2');
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);
final FocusNode child3 = FocusNode(debugLabel: 'child3');
final FocusAttachment child3Attachment = child3.attach(context);
final FocusNode child4 = FocusNode(debugLabel: 'child4');
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);
child4.requestFocus();
await tester.pump();
expect(child4.ancestors, equals(<FocusNode>[parent2, scope2, tester.binding.focusManager.rootScope]));
expect(tester.binding.focusManager.rootScope.descendants, equals(<FocusNode>[child1, child2, parent1, scope1, child3, child4, parent2, scope2]));
scope2Attachment.reparent(parent: child2);
await tester.pump();
expect(child4.ancestors, equals(<FocusNode>[parent2, scope2, child2, parent1, scope1, tester.binding.focusManager.rootScope]));
expect(tester.binding.focusManager.rootScope.descendants, equals(<FocusNode>[child1, child3, child4, parent2, scope2, child2, parent1, scope1]));
});
testWidgets('Can move focus between scopes and keep focus', (WidgetTester tester) async { testWidgets('Can move focus between scopes and keep focus', (WidgetTester tester) async {
final BuildContext context = await setupWidget(tester); final BuildContext context = await setupWidget(tester);
final FocusScopeNode scope1 = FocusScopeNode(); final FocusScopeNode scope1 = FocusScopeNode();
......
...@@ -504,6 +504,7 @@ void main() { ...@@ -504,6 +504,7 @@ void main() {
FocusScope.of(keyA.currentContext).requestFocus(keyA.currentState.focusNode); FocusScope.of(keyA.currentContext).requestFocus(keyA.currentState.focusNode);
expect(FocusScope.of(keyA.currentContext), equals(childFocusScope)); expect(FocusScope.of(keyA.currentContext), equals(childFocusScope));
expect(Focus.of(keyA.currentContext, scopeOk: true), equals(childFocusScope));
WidgetsBinding.instance.focusManager.rootScope.setFirstFocus(FocusScope.of(keyA.currentContext)); WidgetsBinding.instance.focusManager.rootScope.setFirstFocus(FocusScope.of(keyA.currentContext));
await tester.pumpAndSettle(); await tester.pumpAndSettle();
......
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