From 450506868d6f11875f0aab4b9b4cf59dbb8ff80b Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Wed, 28 Feb 2024 05:06:47 +0000 Subject: [PATCH] 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. --- .../lib/src/widgets/focus_manager.dart | 50 +++++++++---------- .../test/widgets/focus_manager_test.dart | 26 ++-------- 2 files changed, 27 insertions(+), 49 deletions(-) diff --git a/packages/flutter/lib/src/widgets/focus_manager.dart b/packages/flutter/lib/src/widgets/focus_manager.dart index 12202531039..22e53c64619 100644 --- a/packages/flutter/lib/src/widgets/focus_manager.dart +++ b/packages/flutter/lib/src/widgets/focus_manager.dart @@ -517,8 +517,21 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// focus traversal policy for a widget subtree. /// * [FocusTraversalPolicy], a class that can be extended to describe a /// traversal policy. - bool get canRequestFocus => _canRequestFocus && ancestors.every(_allowDescendantsToBeFocused); - static bool _allowDescendantsToBeFocused(FocusNode ancestor) => ancestor.descendantsAreFocusable; + bool get canRequestFocus { + 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; @mustCallSuper @@ -778,22 +791,6 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// Use [enclosingScope] to look for scopes above this node. 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 /// node has not yet be added to the focus tree. /// @@ -802,9 +799,12 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// /// Use [nearestScope] to start at this node instead of above it. FocusScopeNode? get enclosingScope { - final FocusScopeNode? enclosingScope = _enclosingScope ??= parent?.nearestScope; - assert(enclosingScope == parent?.nearestScope, '$this has invalid scope cache: $_enclosingScope != ${parent?.nearestScope}'); - return enclosingScope; + for (final FocusNode node in ancestors) { + if (node is FocusScopeNode) { + return node; + } + } + return null; } /// Returns the size of the attached widget's [RenderObject], in logical @@ -990,7 +990,6 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { } node._parent = null; - node._clearEnclosingScopeCache(); _children.remove(node); for (final FocusNode ancestor in ancestors) { ancestor._descendants = null; @@ -1269,14 +1268,13 @@ class FocusScopeNode extends FocusNode { super.skipTraversal, super.canRequestFocus, this.traversalEdgeBehavior = TraversalEdgeBehavior.closedLoop, - }) : super(descendantsAreFocusable: true); + }) : super( + descendantsAreFocusable: true, + ); @override 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 /// [FocusScopeNode]. /// diff --git a/packages/flutter/test/widgets/focus_manager_test.dart b/packages/flutter/test/widgets/focus_manager_test.dart index 8c87bcb0609..ecedf3c987b 100644 --- a/packages/flutter/test/widgets/focus_manager_test.dart +++ b/packages/flutter/test/widgets/focus_manager_test.dart @@ -529,16 +529,16 @@ void main() { // child1 // | // child2 - final FocusScopeNode scope1 = FocusScopeNode(debugLabel: 'scope1'); + final FocusScopeNode scope1 = FocusScopeNode(debugLabel: 'scope2'); addTearDown(scope1.dispose); final FocusAttachment scope2Attachment = scope1.attach(context); scope2Attachment.reparent(parent: tester.binding.focusManager.rootScope); - final FocusNode child1 = FocusNode(debugLabel: 'child1'); + final FocusNode child1 = FocusNode(debugLabel: 'child2'); addTearDown(child1.dispose); final FocusAttachment child2Attachment = child1.attach(context); - final FocusNode child2 = FocusNode(debugLabel: 'child2'); + final FocusNode child2 = FocusNode(debugLabel: 'child3'); addTearDown(child2.dispose); final FocusAttachment child3Attachment = child2.attach(context); @@ -697,26 +697,6 @@ void main() { 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 { final BuildContext context = await setupWidget(tester); final FocusScopeNode scope = FocusScopeNode(debugLabel: 'Scope');