Fix Focus.of to not find FocusScope nodes. (#32826)

Until this change, Focus.of would return a FocusScopeNode if it found a FocusScope widget. This isn't really all that useful, and can easily lead to bad situations where many widgets think that the scope they are in (or the root scope!) is their indication of being focused.

This changes Focus.of to throw an exception if it doesn't find a Focus widget before reaching the nearest FocusScope widget, or the root of the widget hierarchy.

It also adds a nullOk optional bool to Focus.of so that if a caller expects to not find a Focus widget, it can deal with that as it sees fit. I modified InkWell to use this new behavior.

This fixes an unreported issue that widgets using an InkWell will be drawn as focused the first time they are visited.

packages/flutter/lib/src/material/ink_well.dart

@@ -475,7 +475,7 @@ class _InkResponseState<T extends InkResponse> extends State<T> with AutomaticKe
   void didChangeDependencies() {
     super.didChangeDependencies();
     _focusNode?.removeListener(_handleFocusUpdate);
-    _focusNode = Focus.of(context);
+    _focusNode = Focus.of(context, nullOk: true);
     _focusNode?.addListener(_handleFocusUpdate);
   }
 

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

@@ -97,7 +97,8 @@ class FocusAttachment {
     assert(_node != null);
     if (isAttached) {
       assert(_node.context != null);
-      parent ??= Focus.of(_node.context);
+      parent ??= Focus.of(_node.context, nullOk: true);
+      parent ??= FocusScope.of(_node.context);
       assert(parent != null);
       parent._reparent(_node);
     }

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

@@ -217,26 +217,61 @@ class Focus extends StatefulWidget {
   /// part of the focus chain, but shouldn't be accessible via focus traversal.
   final bool skipTraversal;
 
-  /// Returns the [focusNode] of the [Focus] that most tightly encloses the given
-  /// [BuildContext].
+  /// Returns the [focusNode] of the [Focus] that most tightly encloses the
+  /// given [BuildContext].
   ///
-  /// If this node doesn't have a [Focus] widget ancestor, then the
-  /// [FocusManager.rootScope] is returned.
+  /// If no [Focus] node is found before reaching the nearest [FocusScope]
+  /// widget, or there is no [Focus] widget in scope, then this method will
+  /// throw an exception. To return null instead of throwing, pass true for
+  /// [nullOk].
   ///
-  /// The [context] argument must not be null.
-  static FocusNode of(BuildContext context) {
+  /// The [context] and [nullOk] arguments must not be null.
+  static FocusNode of(BuildContext context, { bool nullOk = false }) {
     assert(context != null);
+    assert(nullOk != null);
     final _FocusMarker marker = context.inheritFromWidgetOfExactType(_FocusMarker);
-    return marker?.notifier ?? context.owner.focusManager.rootScope;
+    final FocusNode node = marker?.notifier;
+    if (node is FocusScopeNode) {
+      if (!nullOk) {
+        throw FlutterError(
+            'Focus.of() was called with a context that does not contain a Focus between the given '
+            'context and the nearest FocusScope widget.\n'
+            'No Focus ancestor could be found starting from the context that was passed to '
+            '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'
+            '  $context'
+        );
+      }
+      return null;
+    }
+    if (node == null) {
+      if (!nullOk) {
+        throw FlutterError(
+            'Focus.of() was called with a context that does not contain a Focus widget.\n'
+            'No Focus widget 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 '
+            'ancestor, and do not have a Focus widget descendant in the nearest FocusScope.\n'
+            'The context used was:\n'
+            '  $context'
+        );
+      }
+      return null;
+    }
+    return node;
   }
 
   /// Returns true if the nearest enclosing [Focus] widget's node is focused.
   ///
   /// A convenience method to allow build methods to write:
-  /// `Focus.isAt(context)` to get whether or not the nearest [Focus] or
-  /// [FocusScope] above them in the widget hierarchy currently has the keyboard
-  /// focus.
-  static bool isAt(BuildContext context) => Focus.of(context).hasFocus;
+  /// `Focus.isAt(context)` to get whether or not the nearest [Focus] above them
+  /// in the widget hierarchy currently has the input focus.
+  ///
+  /// Returns false if no [Focus] widget is found before reaching the nearest
+  /// [FocusScope], or if the root of the focus tree is reached without finding
+  /// a [Focus] widget.
+  static bool isAt(BuildContext context) => Focus.of(context, nullOk: true)?.hasFocus ?? false;
 
   @override
   void debugFillProperties(DiagnosticPropertiesBuilder properties) {
@@ -252,7 +287,7 @@ class Focus extends StatefulWidget {
 
 class _FocusState extends State<Focus> {
   FocusNode _internalNode;
-  FocusNode get node => widget.focusNode ?? _internalNode;
+  FocusNode get focusNode => widget.focusNode ?? _internalNode;
   bool _hasFocus;
   bool _didAutofocus = false;
   FocusAttachment _focusAttachment;
@@ -266,28 +301,27 @@ class _FocusState extends State<Focus> {
   void _initNode() {
     if (widget.focusNode == null) {
       // Only create a new node if the widget doesn't have one.
+      // This calls a function instead of just allocating in place because
+      // _createNode is overridden in _FocusScopeState.
       _internalNode ??= _createNode();
     }
-    node.skipTraversal = widget.skipTraversal;
-    _focusAttachment = node.attach(context, onKey: widget.onKey);
-    _hasFocus = node.hasFocus;
+    focusNode.skipTraversal = widget.skipTraversal;
+    _focusAttachment = focusNode.attach(context, onKey: widget.onKey);
+    _hasFocus = focusNode.hasFocus;
+
     // Add listener even if the _internalNode existed before, since it should
-    // not be listening now if we're re-using a previous one, because it should
+    // not be listening now if we're re-using a previous one because it should
     // have already removed its listener.
-    node.addListener(_handleFocusChanged);
+    focusNode.addListener(_handleFocusChanged);
   }
 
-  FocusNode _createNode() {
-    return FocusNode(
-      debugLabel: widget.debugLabel,
-    );
-  }
+  FocusNode _createNode() => FocusNode(debugLabel: widget.debugLabel);
 
   @override
   void dispose() {
     // Regardless of the node owner, we need to remove it from the tree and stop
     // listening to it.
-    node.removeListener(_handleFocusChanged);
+    focusNode.removeListener(_handleFocusChanged);
     _focusAttachment.detach();
     // Don't manage the lifetime of external nodes given to the widget, just the
     // internal node.
@@ -300,7 +334,7 @@ class _FocusState extends State<Focus> {
     super.didChangeDependencies();
     _focusAttachment?.reparent();
     if (!_didAutofocus && widget.autofocus) {
-      FocusScope.of(context).autofocus(node);
+      FocusScope.of(context).autofocus(focusNode);
       _didAutofocus = true;
     }
   }
@@ -314,45 +348,33 @@ class _FocusState extends State<Focus> {
   @override
   void didUpdateWidget(Focus oldWidget) {
     super.didUpdateWidget(oldWidget);
+    assert(() {
+      // Only update the debug label in debug builds, and only if we own the
+      // node.
       if (oldWidget.debugLabel != widget.debugLabel && _internalNode != null) {
         _internalNode.debugLabel = widget.debugLabel;
       }
-    if ((oldWidget.focusNode == widget.focusNode && oldWidget.onKey == widget.onKey)
-        || oldWidget.focusNode == null && widget.focusNode == null) {
-      // Either there aren't changes, or the _internalNode is already attached
-      // and being listened to.
+      return true;
+    }());
+
+    if (oldWidget.focusNode == widget.focusNode) {
       return;
     }
+
     _focusAttachment.detach();
-    if (oldWidget.focusNode == null && widget.focusNode != null) {
-      // We're no longer using the node we were managing. We don't stop managing
-      // it until dispose, so just detach it: we might re-use it eventually, and
-      // calling dispose on it here will confuse other widgets that haven't yet
-      // been notified of a widget change and might still be listening.
-      _internalNode?.removeListener(_handleFocusChanged);
-      _focusAttachment = widget.focusNode?.attach(context, onKey: widget.onKey);
-      widget.focusNode?.addListener(_handleFocusChanged);
-    } else if (oldWidget.focusNode != null && widget.focusNode == null) {
-      oldWidget.focusNode?.removeListener(_handleFocusChanged);
-      // We stopped using the external node, and now we need to manage one.
+    focusNode.removeListener(_handleFocusChanged);
     _initNode();
-    } else {
-      // We just switched which node the widget had, so just change what we
-      // listen to/attach.
-      oldWidget.focusNode.removeListener(_handleFocusChanged);
-      widget.focusNode.addListener(_handleFocusChanged);
-      _focusAttachment = widget.focusNode.attach(context, onKey: widget.onKey);
-    }
-    _hasFocus = node.hasFocus;
+
+    _hasFocus = focusNode.hasFocus;
   }
 
   void _handleFocusChanged() {
-    if (_hasFocus != node.hasFocus) {
+    if (_hasFocus != focusNode.hasFocus) {
       setState(() {
-        _hasFocus = node.hasFocus;
+        _hasFocus = focusNode.hasFocus;
       });
       if (widget.onFocusChange != null) {
-        widget.onFocusChange(node.hasFocus);
+        widget.onFocusChange(focusNode.hasFocus);
       }
     }
   }
@@ -361,7 +383,7 @@ class _FocusState extends State<Focus> {
   Widget build(BuildContext context) {
     _focusAttachment.reparent();
     return _FocusMarker(
-      node: node,
+      node: focusNode,
       child: widget.child,
     );
   }
@@ -420,7 +442,7 @@ class FocusScope extends Focus {
   /// The [autofocus], and [showDecorations] arguments must not be null.
   const FocusScope({
     Key key,
-    FocusNode node,
+    FocusScopeNode node,
     @required Widget child,
     bool autofocus = false,
     ValueChanged<bool> onFocusChange,
@@ -469,7 +491,7 @@ class _FocusScopeState extends _FocusState {
     return Semantics(
       explicitChildNodes: true,
       child: _FocusMarker(
-        node: node,
+        node: focusNode,
         child: widget.child,
       ),
     );

packages/flutter/test/widgets/focus_scope_test.dart

@@ -986,18 +986,38 @@ void main() {
       expect(keyB.currentState.focusNode.hasFocus, isFalse);
       expect(find.text('b'), findsOneWidget);
     });
+    testWidgets('Can focus root node.', (WidgetTester tester) async {
+      final GlobalKey key1 = GlobalKey(debugLabel: '1');
+      await tester.pumpWidget(
+        Focus(
+          key: key1,
+          child: Container(),
+        ),
+      );
+
+      final Element firstElement = tester.element(find.byKey(key1));
+      final FocusScopeNode rootNode = FocusScope.of(firstElement);
+      rootNode.requestFocus();
+
+      await tester.pump();
+
+      expect(rootNode.hasFocus, isTrue);
+      expect(rootNode, equals(firstElement.owner.focusManager.rootScope));
+    });
   });
   group(Focus, () {
-    testWidgets('Focus.of stops at the nearest FocusScope.', (WidgetTester tester) async {
+    testWidgets('Focus.of stops at the nearest Focus widget.', (WidgetTester tester) async {
       final GlobalKey key1 = GlobalKey(debugLabel: '1');
       final GlobalKey key2 = GlobalKey(debugLabel: '2');
       final GlobalKey key3 = GlobalKey(debugLabel: '3');
       final GlobalKey key4 = GlobalKey(debugLabel: '4');
       final GlobalKey key5 = GlobalKey(debugLabel: '5');
       final GlobalKey key6 = GlobalKey(debugLabel: '6');
+      final FocusScopeNode scopeNode = FocusScopeNode();
       await tester.pumpWidget(
-        Focus(
+        FocusScope(
           key: key1,
+          node: scopeNode,
           debugLabel: 'Key 1',
           child: Container(
             key: key2,
@@ -1026,9 +1046,9 @@ void main() {
       final Element element6 = tester.element(find.byKey(key6));
       final FocusNode root = element1.owner.focusManager.rootScope;
 
-      expect(Focus.of(element1), equals(root));
-      expect(Focus.of(element2).parent, equals(root));
-      expect(Focus.of(element3).parent, equals(root));
+      expect(Focus.of(element1, nullOk: true), isNull);
+      expect(Focus.of(element2, nullOk: true), isNull);
+      expect(Focus.of(element3, nullOk: true), isNull);
       expect(Focus.of(element4).parent.parent, equals(root));
       expect(Focus.of(element5).parent.parent, equals(root));
       expect(Focus.of(element6).parent.parent.parent, equals(root));
@@ -1129,24 +1149,6 @@ void main() {
       expect(gotFocus, isTrue);
       expect(node.hasFocus, isTrue);
     });
-    testWidgets('Can focus root node.', (WidgetTester tester) async {
-      final GlobalKey key1 = GlobalKey(debugLabel: '1');
-      await tester.pumpWidget(
-        Focus(
-          key: key1,
-          child: Container(),
-        ),
-      );
-
-      final Element firstElement = tester.element(find.byKey(key1));
-      final FocusNode rootNode = Focus.of(firstElement);
-      rootNode.requestFocus();
-
-      await tester.pump();
-
-      expect(rootNode.hasFocus, isTrue);
-      expect(rootNode, equals(firstElement.owner.focusManager.rootScope));
-    });
   });
   testWidgets('Nodes are removed when all Focuses are removed.', (WidgetTester tester) async {
     final GlobalKey key1 = GlobalKey(debugLabel: '1');