Unverified Commit 45050686 authored by auto-submit[bot]'s avatar auto-submit[bot] Committed by GitHub

Reverts "Cache `FocusNode.enclosingScope`, clean up `descendantsAreFocusable` (#144207)" (#144292)

Reverts flutter/flutter#144207

Initiated by: CaseyHillers

Reason for reverting: b/327301206 - Breaking a customer test

Original PR Author: LongCatIsLooong

Reviewed By: {gspencergoog}

This change reverts the following previous change:
Original Description:
`FocusNode.canRequestFocus` was doing a double traversal if no ancestor disallows focus. The last for loop only has to reach as far as the enclosing scope.

Also this caches the `FocusNode.enclosingScope` since the getter access happens much more frequently than node reparenting.
parent 96a390a9
...@@ -517,8 +517,21 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -517,8 +517,21 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
/// focus traversal policy for a widget subtree. /// focus traversal policy for a widget subtree.
/// * [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 && ancestors.every(_allowDescendantsToBeFocused); bool get canRequestFocus {
static bool _allowDescendantsToBeFocused(FocusNode ancestor) => ancestor.descendantsAreFocusable; if (!_canRequestFocus) {
return false;
}
final FocusScopeNode? scope = enclosingScope;
if (scope != null && !scope.canRequestFocus) {
return false;
}
for (final FocusNode ancestor in ancestors) {
if (!ancestor.descendantsAreFocusable) {
return false;
}
}
return true;
}
bool _canRequestFocus; bool _canRequestFocus;
@mustCallSuper @mustCallSuper
...@@ -778,22 +791,6 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -778,22 +791,6 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
/// Use [enclosingScope] to look for scopes above this node. /// Use [enclosingScope] to look for scopes above this node.
FocusScopeNode? get nearestScope => enclosingScope; FocusScopeNode? get nearestScope => enclosingScope;
FocusScopeNode? _enclosingScope;
void _clearEnclosingScopeCache() {
final FocusScopeNode? cachedScope = _enclosingScope;
if (cachedScope == null) {
return;
}
_enclosingScope = null;
if (children.isNotEmpty) {
for (final FocusNode child in children) {
if (identical(cachedScope, child._enclosingScope)) {
child._clearEnclosingScopeCache();
}
}
}
}
/// Returns the nearest enclosing scope node above this node, or null if the /// Returns the nearest enclosing scope node above this node, or null if the
/// node has not yet be added to the focus tree. /// node has not yet be added to the focus tree.
/// ///
...@@ -802,9 +799,12 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -802,9 +799,12 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
/// ///
/// Use [nearestScope] to start at this node instead of above it. /// Use [nearestScope] to start at this node instead of above it.
FocusScopeNode? get enclosingScope { FocusScopeNode? get enclosingScope {
final FocusScopeNode? enclosingScope = _enclosingScope ??= parent?.nearestScope; for (final FocusNode node in ancestors) {
assert(enclosingScope == parent?.nearestScope, '$this has invalid scope cache: $_enclosingScope != ${parent?.nearestScope}'); if (node is FocusScopeNode) {
return enclosingScope; return node;
}
}
return null;
} }
/// Returns the size of the attached widget's [RenderObject], in logical /// Returns the size of the attached widget's [RenderObject], in logical
...@@ -990,7 +990,6 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { ...@@ -990,7 +990,6 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier {
} }
node._parent = null; node._parent = null;
node._clearEnclosingScopeCache();
_children.remove(node); _children.remove(node);
for (final FocusNode ancestor in ancestors) { for (final FocusNode ancestor in ancestors) {
ancestor._descendants = null; ancestor._descendants = null;
...@@ -1269,14 +1268,13 @@ class FocusScopeNode extends FocusNode { ...@@ -1269,14 +1268,13 @@ class FocusScopeNode extends FocusNode {
super.skipTraversal, super.skipTraversal,
super.canRequestFocus, super.canRequestFocus,
this.traversalEdgeBehavior = TraversalEdgeBehavior.closedLoop, this.traversalEdgeBehavior = TraversalEdgeBehavior.closedLoop,
}) : super(descendantsAreFocusable: true); }) : super(
descendantsAreFocusable: true,
);
@override @override
FocusScopeNode get nearestScope => this; FocusScopeNode get nearestScope => this;
@override
bool get descendantsAreFocusable => _canRequestFocus && super.descendantsAreFocusable;
/// Controls the transfer of focus beyond the first and the last items of a /// Controls the transfer of focus beyond the first and the last items of a
/// [FocusScopeNode]. /// [FocusScopeNode].
/// ///
......
...@@ -529,16 +529,16 @@ void main() { ...@@ -529,16 +529,16 @@ void main() {
// child1 // child1
// | // |
// child2 // child2
final FocusScopeNode scope1 = FocusScopeNode(debugLabel: 'scope1'); final FocusScopeNode scope1 = FocusScopeNode(debugLabel: 'scope2');
addTearDown(scope1.dispose); addTearDown(scope1.dispose);
final FocusAttachment scope2Attachment = scope1.attach(context); final FocusAttachment scope2Attachment = scope1.attach(context);
scope2Attachment.reparent(parent: tester.binding.focusManager.rootScope); scope2Attachment.reparent(parent: tester.binding.focusManager.rootScope);
final FocusNode child1 = FocusNode(debugLabel: 'child1'); final FocusNode child1 = FocusNode(debugLabel: 'child2');
addTearDown(child1.dispose); addTearDown(child1.dispose);
final FocusAttachment child2Attachment = child1.attach(context); final FocusAttachment child2Attachment = child1.attach(context);
final FocusNode child2 = FocusNode(debugLabel: 'child2'); final FocusNode child2 = FocusNode(debugLabel: 'child3');
addTearDown(child2.dispose); addTearDown(child2.dispose);
final FocusAttachment child3Attachment = child2.attach(context); final FocusAttachment child3Attachment = child2.attach(context);
...@@ -697,26 +697,6 @@ void main() { ...@@ -697,26 +697,6 @@ void main() {
expect(parent2.children.first, equals(child1)); expect(parent2.children.first, equals(child1));
}); });
test('FocusScopeNode.canRequestFocus affects descendantsAreFocusable', () {
final FocusScopeNode scope = FocusScopeNode(debugLabel: 'Scope');
scope.descendantsAreFocusable = false;
expect(scope.descendantsAreFocusable, isFalse);
expect(scope.canRequestFocus, isTrue);
scope.descendantsAreFocusable = true;
expect(scope.descendantsAreFocusable, isTrue);
expect(scope.canRequestFocus, isTrue);
scope.canRequestFocus = false;
expect(scope.descendantsAreFocusable, isFalse);
expect(scope.canRequestFocus, isFalse);
scope.canRequestFocus = true;
expect(scope.descendantsAreFocusable, isTrue);
expect(scope.canRequestFocus, isTrue);
});
testWidgets('canRequestFocus affects children.', (WidgetTester tester) async { testWidgets('canRequestFocus affects children.', (WidgetTester tester) async {
final BuildContext context = await setupWidget(tester); final BuildContext context = await setupWidget(tester);
final FocusScopeNode scope = FocusScopeNode(debugLabel: 'Scope'); final FocusScopeNode scope = FocusScopeNode(debugLabel: 'Scope');
......
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