Fix lack of ancestor notification when a focus node is unfocus… (#50319)

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

packages/flutter/lib/src/widgets/focus_manager.dart

@@ -81,14 +81,16 @@ class FocusAttachment {
     assert(_node != null);
     assert(_focusDebug('Detaching node:', <String>[_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<FocusScopeNode>()) {
       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<FocusNode> _dirtyNodes = <FocusNode>{};
 
-  // 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) {
+  // 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;
+      }
+    } else {
+      _markedForFocus = node;
+      _markNeedsUpdate();
+    }
+  }
+
+  // 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 || _nextFocus == node) {
-      if (_primaryFocus == node) {
-        _primaryFocus = null;
+    if (_primaryFocus == node || _markedForFocus == node) {
+      if (_markedForFocus == node) {
+        _markedForFocus = null;
       }
-      if (_nextFocus == node) {
-        _nextFocus = null;
+      if (_primaryFocus == node) {
+        assert(_markedForUnfocus == null);
+        _markedForUnfocus = node;
       }
-      _dirtyNodes.add(node);
       _markNeedsUpdate();
     }
-    assert(_focusDebug('Unfocused node $node:', <String>['primary focus is $_primaryFocus', 'next focus will be $_nextFocus']));
+    assert(_focusDebug('Unfocused node $node:', <String>['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<FocusNode> previousPath = previousFocus?.ancestors?.toSet() ?? <FocusNode>{};
-      final Set<FocusNode> nextPath = _nextFocus.ancestors.toSet();
+      final Set<FocusNode> nextPath = _markedForFocus.ancestors.toSet();
+      if (_markedForUnfocus != null) {
+        final Set<FocusNode> unfocusedNodes = <FocusNode>{_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<FocusNode>('primaryFocus', primaryFocus, defaultValue: null));
-    properties.add(DiagnosticsProperty<FocusNode>('nextFocus', _nextFocus, defaultValue: null));
+    properties.add(DiagnosticsProperty<FocusNode>('nextFocus', _markedForFocus, defaultValue: null));
     final Element element = primaryFocus?.context as Element;
     if (element != null) {
       properties.add(DiagnosticsProperty<String>('primaryFocusCreator', element.debugGetCreatorChain(20)));

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');