From b2a7ff36a9c5a799c7477acacb5a930a282606b8 Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Wed, 12 Feb 2020 16:20:08 -0800 Subject: [PATCH] =?UTF-8?q?Fix=20lack=20of=20ancestor=20notification=20whe?= =?UTF-8?q?n=20a=20focus=20node=20is=20unfocus=E2=80=A6=20(#50319)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a problem when unfocusing focus nodes where the ancestor focus nodes and scopes don't receive notification that a child was unfocused. Fixes #43497 --- .../lib/src/widgets/focus_manager.dart | 179 ++++++++++++------ .../test/widgets/focus_manager_test.dart | 130 +++++++++++++ 2 files changed, 246 insertions(+), 63 deletions(-) diff --git a/packages/flutter/lib/src/widgets/focus_manager.dart b/packages/flutter/lib/src/widgets/focus_manager.dart index b11b130662..3921f36382 100644 --- a/packages/flutter/lib/src/widgets/focus_manager.dart +++ b/packages/flutter/lib/src/widgets/focus_manager.dart @@ -81,14 +81,16 @@ class FocusAttachment { assert(_node != null); assert(_focusDebug('Detaching node:', [_node.toString(), 'With enclosing scope ${_node.enclosingScope}'])); if (isAttached) { - if (_node.hasPrimaryFocus || (_node._manager != null && _node._manager._nextFocus == _node)) { + if (_node.hasPrimaryFocus || (_node._manager != null && _node._manager._markedForFocus == _node)) { _node.unfocus(focusPrevious: true); } - assert(_node._manager?._nextFocus != _node); - assert(!_node.hasPrimaryFocus); - _node._manager?._dirtyNodes?.remove(_node); + // This node is no longer in the tree, so shouldn't send notifications anymore. + _node._manager?._markDetached(_node); _node._parent?._removeChild(_node); _node._attachment = null; + assert(!_node.hasPrimaryFocus); + assert(_node._manager?._markedForFocus != _node); + assert(_node._manager?._markedForUnfocus != _node); } assert(!isAttached); } @@ -398,8 +400,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { set skipTraversal(bool value) { if (value != _skipTraversal) { _skipTraversal = value; - _manager?._dirtyNodes?.add(this); - _manager?._markNeedsUpdate(); + _manager?._markPropertiesChanged(this); } } @@ -442,8 +443,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { unfocus(focusPrevious: true); } _canRequestFocus = value; - _manager?._dirtyNodes?.add(this); - _manager?._markNeedsUpdate(); + _manager?._markPropertiesChanged(this); } } @@ -560,7 +560,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// * [Focus.isAt], which is a static method that will return the focus /// state of the nearest ancestor [Focus] widget's focus node. bool get hasFocus { - if (_manager?.primaryFocus == null) { + if (_manager?.primaryFocus == null || _manager?._markedForUnfocus == this) { return false; } if (hasPrimaryFocus) { @@ -654,8 +654,9 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// This method is safe to call regardless of whether this node has ever /// requested focus. /// - /// Has no effect on nodes that return true from [hasFocus], but false from - /// [hasPrimaryFocus]. + /// For nodes that return true from [hasFocus], but false from + /// [hasPrimaryFocus], this will unfocus the descendant node that has the + /// primary focus instead ([FocusManager.primaryFocus]). /// /// If [focusPrevious] is true, then rather than losing all focus, the focus /// will be moved to the node that the [enclosingScope] thinks should have it, @@ -663,7 +664,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// [FocusScopeNode.setFirstFocus]. void unfocus({ bool focusPrevious = false }) { assert(focusPrevious != null); - if (!hasFocus && (_manager != null && _manager._nextFocus != this)) { + if (!hasFocus && (_manager == null || _manager._markedForFocus != this)) { return; } if (!hasPrimaryFocus) { @@ -671,7 +672,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { // the primary instead. _manager?.primaryFocus?.unfocus(focusPrevious: focusPrevious); } - _manager?._willUnfocusNode(this); + _manager?._markUnfocused(this); final FocusScopeNode scope = enclosingScope; if (scope != null) { scope._focusedChildren.remove(this); @@ -703,20 +704,22 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { return true; } - // Marks the node as dirty, meaning that it needs to notify listeners of a - // focus change the next time focus is resolved by the manager. - void _markAsDirty({FocusNode newFocus}) { + // Marks the node as being the next to be focused, meaning that it will become + // the primary focus and notify listeners of a focus change the next time + // focus is resolved by the manager. If something else calls _markNextFocus + // before then, then that node will become the next focus instead of the + // previous one. + void _markNextFocus(FocusNode newFocus) { if (_manager != null) { // If we have a manager, then let it handle the focus change. - _manager._markNeedsUpdate(newFocus: newFocus); - _manager._dirtyNodes?.add(this); - } else { - // If we don't have a manager, then change the focus locally. - newFocus?._setAsFocusedChild(); - newFocus?._notify(); - if (newFocus != this) { - _notify(); - } + _manager._markNextFocus(this); + return; + } + // If we don't have a manager, then change the focus locally. + newFocus?._setAsFocusedChildForScope(); + newFocus?._notify(); + if (newFocus != this) { + _notify(); } } @@ -773,7 +776,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { } if (hadFocus) { // Update the focus chain for the current focus without changing it. - _manager?.primaryFocus?._setAsFocusedChild(); + _manager?.primaryFocus?._setAsFocusedChildForScope(); } if (oldScope != null && child.context != null && child.enclosingScope != oldScope) { DefaultFocusTraversal.of(child.context, nullOk: true)?.changedScope(node: child, oldScope: oldScope); @@ -816,7 +819,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { return; } if (hasPrimaryFocus) { - _setAsFocusedChild(); + _setAsFocusedChildForScope(); } notifyListeners(); } @@ -865,13 +868,13 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { _requestFocusWhenReparented = true; return; } - _setAsFocusedChild(); - if (hasPrimaryFocus) { + _setAsFocusedChildForScope(); + if (hasPrimaryFocus && (_manager._markedForFocus == null || _manager._markedForFocus == this)) { return; } _hasKeyboardToken = true; assert(_focusDebug('Node requesting focus: $this')); - _markAsDirty(newFocus: this); + _markNextFocus(this); } // If set to true, the node will request focus on this node the next time @@ -897,7 +900,7 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// node would be focused if the enclosing scope receives focus, and keeps /// track of previously focused children in that scope, so that if the focused /// child in that scope is removed, the previous focus returns. - void _setAsFocusedChild() { + void _setAsFocusedChildForScope() { FocusNode scopeFocus = this; for (final FocusScopeNode ancestor in ancestors.whereType()) { assert(scopeFocus != ancestor, 'Somehow made a loop by setting focusedChild to its scope.'); @@ -1043,7 +1046,7 @@ class FocusScopeNode extends FocusNode { if (hasFocus) { scope._doRequestFocus(); } else { - scope._setAsFocusedChild(); + scope._setAsFocusedChildForScope(); } } @@ -1084,8 +1087,8 @@ class FocusScopeNode extends FocusNode { // We didn't find a FocusNode at the leaf, so we're focusing the scope, if // allowed. if (primaryFocus.canRequestFocus) { - _setAsFocusedChild(); - _markAsDirty(newFocus: this); + _setAsFocusedChildForScope(); + _markNextFocus(this); } } else { // We found a FocusScope at the leaf, so ask it to focus itself instead of @@ -1377,29 +1380,70 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier implements Diagn FocusNode get primaryFocus => _primaryFocus; FocusNode _primaryFocus; - // The node that has requested to have the primary focus, but hasn't been - // given it yet. - FocusNode _nextFocus; // The set of nodes that need to notify their listeners of changes at the next // update. final Set _dirtyNodes = {}; - // Called to indicate that the given node is being unfocused, and that any - // pending request to be focused should be canceled. - void _willUnfocusNode(FocusNode node) { - assert(node != null); - assert(_focusDebug('Unfocusing node $node')); - if (_primaryFocus == node || _nextFocus == node) { - if (_primaryFocus == node) { - _primaryFocus = null; + // The node that has requested to have the primary focus, but hasn't been + // given it yet. + FocusNode _markedForFocus; + + // The node that has been marked as needing to be unfocused during the next + // focus update. + FocusNode _markedForUnfocus; + + void _markDetached(FocusNode node) { + // The node has been removed from the tree, so it no longer needs to be + // notified of changes. + assert(_focusDebug('Node was detached: $node')); + if (_markedForUnfocus == node) { + _markedForUnfocus = null; + } + if (_primaryFocus == node) { + _primaryFocus = null; + } + _dirtyNodes?.remove(node); + } + + void _markPropertiesChanged(FocusNode node) { + _markNeedsUpdate(); + assert(_focusDebug('Properties changed for node $node.')); + _dirtyNodes?.add(node); + } + + void _markNextFocus(FocusNode node) { + if (_primaryFocus == node) { + // The caller asked for the current focus to be the next focus, so just + // pretend that didn't happen. + _markedForFocus = null; + // If this node is going to be the next focus, then it's not going to be + // unfocused unless we call _markUnfocused again, so unset _unfocusedNode. + if (_markedForUnfocus == node) { + _markedForUnfocus = null; } - if (_nextFocus == node) { - _nextFocus = null; - } - _dirtyNodes.add(node); + } else { + _markedForFocus = node; _markNeedsUpdate(); } - assert(_focusDebug('Unfocused node $node:', ['primary focus is $_primaryFocus', 'next focus will be $_nextFocus'])); + } + + // Called to indicate that the given node should be marked to be unfocused at + // the next focus update, and that any pending request to focus it should be + // canceled. + void _markUnfocused(FocusNode node) { + assert(node != null); + assert(_focusDebug('Unfocusing node $node')); + if (_primaryFocus == node || _markedForFocus == node) { + if (_markedForFocus == node) { + _markedForFocus = null; + } + if (_primaryFocus == node) { + assert(_markedForUnfocus == null); + _markedForUnfocus = node; + } + _markNeedsUpdate(); + } + assert(_focusDebug('Unfocused node $node:', ['primary focus is $_primaryFocus', 'next focus will be $_markedForFocus'])); } // True indicates that there is an update pending. @@ -1407,11 +1451,8 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier implements Diagn // Request that an update be scheduled, optionally requesting focus for the // given newFocus node. - void _markNeedsUpdate({FocusNode newFocus}) { - // If newFocus isn't specified, then don't mess with _nextFocus, just - // schedule the update. - _nextFocus = newFocus ?? _nextFocus; - assert(_focusDebug('Scheduling update, next focus will be $_nextFocus')); + void _markNeedsUpdate() { + assert(_focusDebug('Scheduling update, current focus is $_primaryFocus, next focus will be $_markedForFocus')); if (_haveScheduledUpdate) { return; } @@ -1421,22 +1462,33 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier implements Diagn void _applyFocusChange() { _haveScheduledUpdate = false; - assert(_focusDebug('Refreshing focus state. Next focus will be $_nextFocus')); + if (_markedForUnfocus == _primaryFocus) { + _primaryFocus = null; + } final FocusNode previousFocus = _primaryFocus; - if (_primaryFocus == null && _nextFocus == null) { + if (_primaryFocus == null && _markedForFocus == null) { // If we don't have any current focus, and nobody has asked to focus yet, // then revert to the root scope. - _nextFocus = rootScope; + _markedForFocus = rootScope; } - if (_nextFocus != null && _nextFocus != _primaryFocus) { - _primaryFocus = _nextFocus; + assert(_focusDebug('Refreshing focus state. Next focus will be $_markedForFocus')); + // A node has requested to be the next focus, and isn't already the primary + // focus. + if (_markedForFocus != null && _markedForFocus != _primaryFocus) { final Set previousPath = previousFocus?.ancestors?.toSet() ?? {}; - final Set nextPath = _nextFocus.ancestors.toSet(); + final Set nextPath = _markedForFocus.ancestors.toSet(); + if (_markedForUnfocus != null) { + final Set unfocusedNodes = {_markedForUnfocus, ..._markedForUnfocus.ancestors}; + unfocusedNodes.removeAll(nextPath); // No need to dirty the ancestors that are in the newly focused set. + _dirtyNodes.addAll(unfocusedNodes); + } // Notify nodes that are newly focused. _dirtyNodes.addAll(nextPath.difference(previousPath)); // Notify nodes that are no longer focused _dirtyNodes.addAll(previousPath.difference(nextPath)); - _nextFocus = null; + + _primaryFocus = _markedForFocus; + _markedForFocus = null; } if (previousFocus != _primaryFocus) { assert(_focusDebug('Updating focus from $previousFocus to $_primaryFocus')); @@ -1452,6 +1504,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier implements Diagn node._notify(); } _dirtyNodes.clear(); + _markedForUnfocus = null; if (previousFocus != _primaryFocus) { notifyListeners(); } @@ -1474,7 +1527,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier implements Diagn void debugFillProperties(DiagnosticPropertiesBuilder properties) { properties.add(FlagProperty('haveScheduledUpdate', value: _haveScheduledUpdate, ifTrue: 'UPDATE SCHEDULED')); properties.add(DiagnosticsProperty('primaryFocus', primaryFocus, defaultValue: null)); - properties.add(DiagnosticsProperty('nextFocus', _nextFocus, defaultValue: null)); + properties.add(DiagnosticsProperty('nextFocus', _markedForFocus, defaultValue: null)); final Element element = primaryFocus?.context as Element; if (element != null) { properties.add(DiagnosticsProperty('primaryFocusCreator', element.debugGetCreatorChain(20))); diff --git a/packages/flutter/test/widgets/focus_manager_test.dart b/packages/flutter/test/widgets/focus_manager_test.dart index 8b559d473e..9a6b3b05d4 100644 --- a/packages/flutter/test/widgets/focus_manager_test.dart +++ b/packages/flutter/test/widgets/focus_manager_test.dart @@ -745,6 +745,136 @@ void main() { await tester.pump(); expect(parent1.focusedChild, equals(child2)); }); + testWidgets('Ancestors get notified exactly as often as needed if focused child changes focus.', (WidgetTester tester) async { + bool topFocus = false; + bool parent1Focus = false; + bool parent2Focus = false; + bool child1Focus = false; + bool child2Focus = false; + int topNotify = 0; + int parent1Notify = 0; + int parent2Notify = 0; + int child1Notify = 0; + int child2Notify = 0; + void clear() { + topFocus = false; + parent1Focus = false; + parent2Focus = false; + child1Focus = false; + child2Focus = false; + topNotify = 0; + parent1Notify = 0; + parent2Notify = 0; + child1Notify = 0; + child2Notify = 0; + } + final BuildContext context = await setupWidget(tester); + final FocusScopeNode top = FocusScopeNode(debugLabel: 'top'); + final FocusAttachment topAttachment = top.attach(context); + final FocusScopeNode parent1 = FocusScopeNode(debugLabel: 'parent1'); + final FocusAttachment parent1Attachment = parent1.attach(context); + final FocusScopeNode parent2 = FocusScopeNode(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); + topAttachment.reparent(parent: tester.binding.focusManager.rootScope); + parent1Attachment.reparent(parent: top); + parent2Attachment.reparent(parent: top); + child1Attachment.reparent(parent: parent1); + child2Attachment.reparent(parent: parent2); + top.addListener(() { + topNotify++; + topFocus = top.hasFocus; + }); + parent1.addListener(() { + parent1Notify++; + parent1Focus = parent1.hasFocus; + }); + parent2.addListener(() { + parent2Notify++; + parent2Focus = parent2.hasFocus; + }); + child1.addListener(() { + child1Notify++; + child1Focus = child1.hasFocus; + }); + child2.addListener(() { + child2Notify++; + child2Focus = child2.hasFocus; + }); + child1.requestFocus(); + await tester.pump(); + expect(topFocus, isTrue); + expect(parent1Focus, isTrue); + expect(child1Focus, isTrue); + expect(parent2Focus, isFalse); + expect(child2Focus, isFalse); + expect(topNotify, equals(1)); + expect(parent1Notify, equals(1)); + expect(child1Notify, equals(1)); + expect(parent2Notify, equals(0)); + expect(child2Notify, equals(0)); + + clear(); + child1.unfocus(); + await tester.pump(); + expect(topFocus, isFalse); + expect(parent1Focus, isFalse); + expect(child1Focus, isFalse); + expect(parent2Focus, isFalse); + expect(child2Focus, isFalse); + expect(topNotify, equals(1)); + expect(parent1Notify, equals(1)); + expect(child1Notify, equals(1)); + expect(parent2Notify, equals(0)); + expect(child2Notify, equals(0)); + + clear(); + child1.requestFocus(); + await tester.pump(); + expect(topFocus, isTrue); + expect(parent1Focus, isTrue); + expect(child1Focus, isTrue); + expect(parent2Focus, isFalse); + expect(child2Focus, isFalse); + expect(topNotify, equals(1)); + expect(parent1Notify, equals(1)); + expect(child1Notify, equals(1)); + expect(parent2Notify, equals(0)); + expect(child2Notify, equals(0)); + + clear(); + child2.requestFocus(); + await tester.pump(); + expect(topFocus, isFalse); + expect(parent1Focus, isFalse); + expect(child1Focus, isFalse); + expect(parent2Focus, isTrue); + expect(child2Focus, isTrue); + expect(topNotify, equals(0)); + expect(parent1Notify, equals(1)); + expect(child1Notify, equals(1)); + expect(parent2Notify, equals(1)); + expect(child2Notify, equals(1)); + + // Changing the focus back before the pump shouldn't cause notifications. + clear(); + child1.requestFocus(); + child2.requestFocus(); + await tester.pump(); + expect(topFocus, isFalse); + expect(parent1Focus, isFalse); + expect(child1Focus, isFalse); + expect(parent2Focus, isFalse); + expect(child2Focus, isFalse); + expect(topNotify, equals(0)); + expect(parent1Notify, equals(0)); + expect(child1Notify, equals(0)); + expect(parent2Notify, equals(0)); + expect(child2Notify, equals(0)); + }); testWidgets('Focus changes notify listeners.', (WidgetTester tester) async { final BuildContext context = await setupWidget(tester); final FocusScopeNode parent1 = FocusScopeNode(debugLabel: 'parent1');