From e3005e6962cfefbc12e7aac56576597177cc966f Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Thu, 5 Dec 2019 15:33:05 -0800 Subject: [PATCH] Fixes Focus and FocusScope's assignment of canRequestFocus. (#46168) This fixes an issue where lines like this: focusNode.canRequestFocus = widget.canRequestFocus ?? focusNode.canRequestFocus; Were causing the canRequestFocus bit to copy the status of the enclosing scope, since canRequestFocus also looks to the enclosing scope to decide if it can focus. --- packages/flutter/lib/src/widgets/basic.dart | 9 + .../lib/src/widgets/focus_manager.dart | 13 +- .../flutter/lib/src/widgets/focus_scope.dart | 16 +- .../test/widgets/focus_scope_test.dart | 163 ++++++++++++++++++ 4 files changed, 194 insertions(+), 7 deletions(-) diff --git a/packages/flutter/lib/src/widgets/basic.dart b/packages/flutter/lib/src/widgets/basic.dart index c54c89e0e6..a1143b9307 100644 --- a/packages/flutter/lib/src/widgets/basic.dart +++ b/packages/flutter/lib/src/widgets/basic.dart @@ -2608,6 +2608,9 @@ class SizedOverflowBox extends SingleChildRenderObjectWidget { /// painting anything, without making the child available for hit testing, and /// without taking any room in the parent. /// +/// Offstage children are still active: they can receive focus and have keyboard +/// input directed to them. +/// /// Animations continue to run in offstage children, and therefore use battery /// and CPU time, regardless of whether the animations end up being visible. /// @@ -2634,6 +2637,12 @@ class Offstage extends SingleChildRenderObjectWidget { /// painting anything, without making the child available for hit testing, and /// without taking any room in the parent. /// + /// Offstage children are still active: they can receive focus and have keyboard + /// input directed to them. + /// + /// Animations continue to run in offstage children, and therefore use battery + /// and CPU time, regardless of whether the animations end up being visible. + /// /// If false, the child is included in the tree as normal. final bool offstage; diff --git a/packages/flutter/lib/src/widgets/focus_manager.dart b/packages/flutter/lib/src/widgets/focus_manager.dart index 4d8ec5356d..e5d9a00084 100644 --- a/packages/flutter/lib/src/widgets/focus_manager.dart +++ b/packages/flutter/lib/src/widgets/focus_manager.dart @@ -404,9 +404,16 @@ class FocusNode with DiagnosticableTreeMixin, ChangeNotifier { /// If true, this focus node may request the primary focus. /// /// Defaults to true. Set to false if you want this node to do nothing when - /// [requestFocus] is called on it. Does not affect the children of this node, - /// and [hasFocus] can still return true if this node is the ancestor of a - /// node with primary focus. + /// [requestFocus] is called on it. + /// + /// If set to false on a [FocusScopeNode], will cause all of the children of + /// the scope node to not be focusable. + /// + /// If set to false on a [FocusNode], it will not affect the children of the + /// node. + /// + /// The [hasFocus] member can still return true if this node is the ancestor + /// of a node with primary focus. /// /// This is different than [skipTraversal] because [skipTraversal] still /// allows the node to be focused, just not traversed to via the diff --git a/packages/flutter/lib/src/widgets/focus_scope.dart b/packages/flutter/lib/src/widgets/focus_scope.dart index 60d3ff62b3..03a79372f6 100644 --- a/packages/flutter/lib/src/widgets/focus_scope.dart +++ b/packages/flutter/lib/src/widgets/focus_scope.dart @@ -345,8 +345,12 @@ class _FocusState extends State { _internalNode ??= _createNode(); } _focusAttachment = focusNode.attach(context, onKey: widget.onKey); - focusNode.skipTraversal = widget.skipTraversal ?? focusNode.skipTraversal; - focusNode.canRequestFocus = widget.canRequestFocus ?? focusNode.canRequestFocus; + if (widget.skipTraversal != null) { + focusNode.skipTraversal = widget.skipTraversal; + } + if (widget.canRequestFocus != null) { + focusNode.canRequestFocus = widget.canRequestFocus; + } _canRequestFocus = focusNode.canRequestFocus; _hasPrimaryFocus = focusNode.hasPrimaryFocus; @@ -410,8 +414,12 @@ class _FocusState extends State { }()); if (oldWidget.focusNode == widget.focusNode) { - focusNode.skipTraversal = widget.skipTraversal ?? focusNode.skipTraversal; - focusNode.canRequestFocus = widget.canRequestFocus ?? focusNode.canRequestFocus; + if (widget.skipTraversal != null) { + focusNode.skipTraversal = widget.skipTraversal; + } + if (widget.canRequestFocus != null) { + focusNode.canRequestFocus = widget.canRequestFocus; + } } else { _focusAttachment.detach(); focusNode.removeListener(_handleFocusChanged); diff --git a/packages/flutter/test/widgets/focus_scope_test.dart b/packages/flutter/test/widgets/focus_scope_test.dart index 043da1c3c9..afa8f9b16f 100644 --- a/packages/flutter/test/widgets/focus_scope_test.dart +++ b/packages/flutter/test/widgets/focus_scope_test.dart @@ -1336,4 +1336,167 @@ void main() { expect(key.currentState.focusNode.canRequestFocus, isFalse); }); + + testWidgets('canRequestFocus causes descendants of scope to be skipped.', (WidgetTester tester) async { + final GlobalKey scope1 = GlobalKey(debugLabel: 'scope1'); + final GlobalKey scope2 = GlobalKey(debugLabel: 'scope2'); + final GlobalKey focus1 = GlobalKey(debugLabel: 'focus1'); + final GlobalKey focus2 = GlobalKey(debugLabel: 'focus2'); + final GlobalKey container1 = GlobalKey(debugLabel: 'container'); + Future pumpTest({ + bool allowScope1 = true, + bool allowScope2 = true, + bool allowFocus1 = true, + bool allowFocus2 = true, + }) async { + await tester.pumpWidget( + FocusScope( + key: scope1, + canRequestFocus: allowScope1, + child: FocusScope( + key: scope2, + canRequestFocus: allowScope2, + child: Focus( + key: focus1, + canRequestFocus: allowFocus1, + child: Focus( + key: focus2, + canRequestFocus: allowFocus2, + child: Container( + key: container1, + ), + ), + ), + ), + ), + ); + await tester.pump(); + } + + // Check childless node (focus2). + await pumpTest(); + Focus.of(container1.currentContext).requestFocus(); + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isTrue); + await pumpTest(allowFocus2: false); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + Focus.of(container1.currentContext).requestFocus(); + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + await pumpTest(); + Focus.of(container1.currentContext).requestFocus(); + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isTrue); + + // Check FocusNode with child (focus1). Shouldn't affect children. + await pumpTest(allowFocus1: false); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + Focus.of(focus2.currentContext).requestFocus(); // Try to focus focus1 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + Focus.of(container1.currentContext).requestFocus(); // Now try to focus focus2 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isTrue); + await pumpTest(); + // Try again, now that we've set focus1's canRequestFocus to true again. + Focus.of(container1.currentContext).unfocus(); + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + Focus.of(container1.currentContext).requestFocus(); + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isTrue); + + // Check FocusScopeNode with only FocusNode children (scope2). Should affect children. + await pumpTest(allowScope2: false); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + FocusScope.of(focus1.currentContext).requestFocus(); // Try to focus scope2 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + Focus.of(focus2.currentContext).requestFocus(); // Try to focus focus1 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + Focus.of(container1.currentContext).requestFocus(); // Try to focus focus2 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + await pumpTest(); + // Try again, now that we've set scope2's canRequestFocus to true again. + Focus.of(container1.currentContext).requestFocus(); + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isTrue); + + // Check FocusScopeNode with both FocusNode children and FocusScope children (scope1). Should affect children. + await pumpTest(allowScope1: false); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + FocusScope.of(scope2.currentContext).requestFocus(); // Try to focus scope1 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + FocusScope.of(focus1.currentContext).requestFocus(); // Try to focus scope2 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + Focus.of(focus2.currentContext).requestFocus(); // Try to focus focus1 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + Focus.of(container1.currentContext).requestFocus(); // Try to focus focus2 + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isFalse); + await pumpTest(); + // Try again, now that we've set scope1's canRequestFocus to true again. + Focus.of(container1.currentContext).requestFocus(); + await tester.pump(); + expect(Focus.of(container1.currentContext).hasFocus, isTrue); + }); + + testWidgets('skipTraversal works as expected.', (WidgetTester tester) async { + final FocusScopeNode scope1 = FocusScopeNode(debugLabel: 'scope1'); + final FocusScopeNode scope2 = FocusScopeNode(debugLabel: 'scope2'); + final FocusNode focus1 = FocusNode(debugLabel: 'focus1'); + final FocusNode focus2 = FocusNode(debugLabel: 'focus2'); + + Future pumpTest({ + bool traverseScope1 = false, + bool traverseScope2 = false, + bool traverseFocus1 = false, + bool traverseFocus2 = false, + }) async { + await tester.pumpWidget( + FocusScope( + node: scope1, + skipTraversal: traverseScope1, + child: FocusScope( + node: scope2, + skipTraversal: traverseScope2, + child: Focus( + focusNode: focus1, + skipTraversal: traverseFocus1, + child: Focus( + focusNode: focus2, + skipTraversal: traverseFocus2, + child: Container(), + ), + ), + ), + ), + ); + await tester.pump(); + } + + await pumpTest(); + expect(scope1.traversalDescendants, equals([focus2, focus1, scope2])); + + // Check childless node (focus2). + await pumpTest(traverseFocus2: true); + expect(scope1.traversalDescendants, equals([focus1, scope2])); + + // Check FocusNode with child (focus1). Shouldn't affect children. + await pumpTest(traverseFocus1: true); + expect(scope1.traversalDescendants, equals([focus2, scope2])); + + // Check FocusScopeNode with only FocusNode children (scope2). Should affect children. + await pumpTest(traverseScope2: true); + expect(scope1.traversalDescendants, equals([focus2, focus1])); + + // Check FocusScopeNode with both FocusNode children and FocusScope children (scope1). Should affect children. + await pumpTest(traverseScope1: true); + expect(scope1.traversalDescendants, equals([focus2, focus1, scope2])); + }); }