From 0028887a692977b30b3f95d1748618ef99a27d15 Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Thu, 31 Oct 2019 12:58:14 -0700 Subject: [PATCH] Don't allow Disabled InkWells to be focusable (#43848) Makes sure that disabled InkWell/InkResponse and widgets that use them don't allow themselves to be focused. ListTile, PopupMenu, and Stepper were not setting canRequestFocus properly on the InkWell, and InkWell was allowing focus even if it was disabled (it was basically just relying on the containing widget to set canRequestFocus properly). Now InkWell must both be enabled (have an onTap or similar) and have canRequestFocus set to true. --- .../flutter/lib/src/material/ink_well.dart | 3 +- .../flutter/lib/src/material/list_tile.dart | 1 + .../flutter/lib/src/material/popup_menu.dart | 2 + .../flutter/lib/src/material/stepper.dart | 2 + packages/flutter/test/material/chip_test.dart | 1 - .../flutter/test/material/ink_well_test.dart | 88 +++++++++++++ .../flutter/test/material/list_tile_test.dart | 77 ++++++++++-- .../test/material/popup_menu_test.dart | 117 ++++++++++++++++++ .../flutter/test/material/stepper_test.dart | 52 ++++++++ 9 files changed, 331 insertions(+), 12 deletions(-) diff --git a/packages/flutter/lib/src/material/ink_well.dart b/packages/flutter/lib/src/material/ink_well.dart index 3acf8be51d..f478dfeeac 100644 --- a/packages/flutter/lib/src/material/ink_well.dart +++ b/packages/flutter/lib/src/material/ink_well.dart @@ -773,11 +773,12 @@ class _InkResponseState extends State with AutomaticKe _highlights[type]?.color = getHighlightColorForType(type); } _currentSplash?.color = widget.splashColor ?? Theme.of(context).splashColor; + final bool canRequestFocus = enabled && widget.canRequestFocus; return Actions( actions: _actionMap, child: Focus( focusNode: widget.focusNode, - canRequestFocus: widget.canRequestFocus, + canRequestFocus: canRequestFocus, onFocusChange: _handleFocusUpdate, autofocus: widget.autofocus, child: MouseRegion( diff --git a/packages/flutter/lib/src/material/list_tile.dart b/packages/flutter/lib/src/material/list_tile.dart index 4f45c932de..2d2e639120 100644 --- a/packages/flutter/lib/src/material/list_tile.dart +++ b/packages/flutter/lib/src/material/list_tile.dart @@ -880,6 +880,7 @@ class ListTile extends StatelessWidget { return InkWell( onTap: enabled ? onTap : null, onLongPress: enabled ? onLongPress : null, + canRequestFocus: enabled, child: Semantics( selected: selected, enabled: enabled, diff --git a/packages/flutter/lib/src/material/popup_menu.dart b/packages/flutter/lib/src/material/popup_menu.dart index b1deb595c0..d54195bb3a 100644 --- a/packages/flutter/lib/src/material/popup_menu.dart +++ b/packages/flutter/lib/src/material/popup_menu.dart @@ -323,6 +323,7 @@ class PopupMenuItemState> extends State { return InkWell( onTap: widget.enabled ? handleTap : null, + canRequestFocus: widget.enabled, child: item, ); } @@ -1090,6 +1091,7 @@ class _PopupMenuButtonState extends State> { message: widget.tooltip ?? MaterialLocalizations.of(context).showMenuTooltip, child: InkWell( onTap: widget.enabled ? showButtonMenu : null, + canRequestFocus: widget.enabled, child: widget.child, ), ); diff --git a/packages/flutter/lib/src/material/stepper.dart b/packages/flutter/lib/src/material/stepper.dart index 653de0914e..f0f4571a77 100644 --- a/packages/flutter/lib/src/material/stepper.dart +++ b/packages/flutter/lib/src/material/stepper.dart @@ -593,6 +593,7 @@ class _StepperState extends State with TickerProviderStateMixin { if (widget.onStepTapped != null) widget.onStepTapped(i); } : null, + canRequestFocus: widget.steps[i].state != StepState.disabled, child: _buildVerticalHeader(i), ), _buildVerticalBody(i), @@ -610,6 +611,7 @@ class _StepperState extends State with TickerProviderStateMixin { if (widget.onStepTapped != null) widget.onStepTapped(i); } : null, + canRequestFocus: widget.steps[i].state != StepState.disabled, child: Row( children: [ Container( diff --git a/packages/flutter/test/material/chip_test.dart b/packages/flutter/test/material/chip_test.dart index 0a9d79f2c0..3802883e24 100644 --- a/packages/flutter/test/material/chip_test.dart +++ b/packages/flutter/test/material/chip_test.dart @@ -1620,7 +1620,6 @@ void main() { TestSemantics( label: 'test', textDirection: TextDirection.ltr, - flags: [SemanticsFlag.isFocusable], ), ], ), diff --git a/packages/flutter/test/material/ink_well_test.dart b/packages/flutter/test/material/ink_well_test.dart index dfad562e23..45312abbbd 100644 --- a/packages/flutter/test/material/ink_well_test.dart +++ b/packages/flutter/test/material/ink_well_test.dart @@ -322,4 +322,92 @@ void main() { semantics.dispose(); }); + testWidgets("ink response doesn't focus when disabled", (WidgetTester tester) async { + WidgetsBinding.instance.focusManager.highlightStrategy = FocusHighlightStrategy.alwaysTouch; + final FocusNode focusNode = FocusNode(debugLabel: 'Ink Focus'); + final GlobalKey childKey = GlobalKey(); + await tester.pumpWidget( + Material( + child: Directionality( + textDirection: TextDirection.ltr, + child: InkWell( + autofocus: true, + onTap: () {}, + onLongPress: () {}, + onHover: (bool hover) {}, + focusNode: focusNode, + child: Container(key: childKey), + ), + ), + ), + ); + await tester.pumpAndSettle(); + expect(focusNode.hasPrimaryFocus, isTrue); + await tester.pumpWidget( + Material( + child: Directionality( + textDirection: TextDirection.ltr, + child: InkWell( + focusNode: focusNode, + child: Container(key: childKey), + ), + ), + ), + ); + await tester.pumpAndSettle(); + expect(focusNode.hasPrimaryFocus, isFalse); + }); + + testWidgets("ink response doesn't hover when disabled", (WidgetTester tester) async { + WidgetsBinding.instance.focusManager.highlightStrategy = FocusHighlightStrategy.alwaysTouch; + final FocusNode focusNode = FocusNode(debugLabel: 'Ink Focus'); + final GlobalKey childKey = GlobalKey(); + bool hovering = false; + await tester.pumpWidget( + Material( + child: Directionality( + textDirection: TextDirection.ltr, + child: Container( + width: 100, + height: 100, + child: InkWell( + autofocus: true, + onTap: () {}, + onLongPress: () {}, + onHover: (bool value) { hovering = value; }, + focusNode: focusNode, + child: Container(key: childKey), + ), + ), + ), + ), + ); + await tester.pumpAndSettle(); + expect(focusNode.hasPrimaryFocus, isTrue); + final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse); + addTearDown(gesture.removePointer); + await gesture.moveTo(tester.getCenter(find.byKey(childKey))); + await tester.pumpAndSettle(); + expect(hovering, isTrue); + + await tester.pumpWidget( + Material( + child: Directionality( + textDirection: TextDirection.ltr, + child: Container( + width: 100, + height: 100, + child: InkWell( + focusNode: focusNode, + onHover: (bool value) { hovering = value; }, + child: Container(key: childKey), + ), + ), + ), + ), + ); + + await tester.pumpAndSettle(); + expect(focusNode.hasPrimaryFocus, isFalse); + }); } diff --git a/packages/flutter/test/material/list_tile_test.dart b/packages/flutter/test/material/list_tile_test.dart index ddf3e68e15..3ead6fa35e 100644 --- a/packages/flutter/test/material/list_tile_test.dart +++ b/packages/flutter/test/material/list_tile_test.dart @@ -349,16 +349,20 @@ void main() { child: MediaQuery( data: const MediaQueryData(), child: Column( - children: const [ - ListTile( + children: [ + const ListTile( title: Text('one'), ), ListTile( - title: Text('two'), + title: const Text('two'), + onTap: () {}, + ), + const ListTile( + title: Text('three'), selected: true, ), - ListTile( - title: Text('three'), + const ListTile( + title: Text('four'), enabled: false, ), ], @@ -377,25 +381,31 @@ void main() { flags: [ SemanticsFlag.hasEnabledState, SemanticsFlag.isEnabled, - SemanticsFlag.isFocusable, ], label: 'one', ), + TestSemantics.rootChild( + flags: [ + SemanticsFlag.hasEnabledState, + SemanticsFlag.isEnabled, + SemanticsFlag.isFocusable, + ], + actions: [SemanticsAction.tap], + label: 'two', + ), TestSemantics.rootChild( flags: [ SemanticsFlag.isSelected, SemanticsFlag.hasEnabledState, SemanticsFlag.isEnabled, - SemanticsFlag.isFocusable, ], - label: 'two', + label: 'three', ), TestSemantics.rootChild( flags: [ SemanticsFlag.hasEnabledState, - SemanticsFlag.isFocusable, ], - label: 'three', + label: 'four', ), ], ), @@ -1127,4 +1137,51 @@ void main() { expect(tester.getRect(find.byType(Placeholder).at(0)), const Rect.fromLTWH(800.0 - 16.0 - 24.0, 16.0, 24.0, 56.0)); expect(tester.getRect(find.byType(Placeholder).at(1)), const Rect.fromLTWH(800.0 - 16.0 - 24.0, 88.0 + 16.0, 24.0, 56.0)); }); + testWidgets('ListTile only accepts focus when enabled', (WidgetTester tester) async { + final GlobalKey childKey = GlobalKey(); + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: ListView( + children: [ + ListTile( + title: Text('A', key: childKey), + dense: true, + enabled: true, + onTap: () {}, + ), + ], + ), + ), + ), + ); + await tester.pump(); // Let the focus take effect. + + final FocusNode tileNode = Focus.of(childKey.currentContext); + tileNode.requestFocus(); + await tester.pump(); // Let the focus take effect. + expect(Focus.of(childKey.currentContext, nullOk: true).hasPrimaryFocus, isTrue); + + expect(tileNode.hasPrimaryFocus, isTrue); + await tester.pumpWidget( + MaterialApp( + home: Material( + child: ListView( + children: [ + ListTile( + title: Text('A', key: childKey), + dense: true, + enabled: false, + onTap: () {}, + ), + ], + ), + ), + ), + ); + + expect(tester.binding.focusManager.primaryFocus, isNot(equals(tileNode))); + expect(Focus.of(childKey.currentContext, nullOk: true).hasPrimaryFocus, isFalse); + }); } diff --git a/packages/flutter/test/material/popup_menu_test.dart b/packages/flutter/test/material/popup_menu_test.dart index d05abe72bd..791cb52f3c 100644 --- a/packages/flutter/test/material/popup_menu_test.dart +++ b/packages/flutter/test/material/popup_menu_test.dart @@ -175,6 +175,123 @@ void main() { expect(onCanceledCalled, isFalse); }); + testWidgets('disabled PopupMenuButton is not focusable', (WidgetTester tester) async { + final Key popupButtonKey = UniqueKey(); + final GlobalKey childKey = GlobalKey(); + bool itemBuilderCalled = false; + bool onSelectedCalled = false; + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Column( + children: [ + PopupMenuButton( + key: popupButtonKey, + child: Container(key: childKey), + enabled: false, + itemBuilder: (BuildContext context) { + itemBuilderCalled = true; + return >[ + const PopupMenuItem( + value: 1, + child: Text('Tap me please!'), + ), + ]; + }, + onSelected: (int selected) => onSelectedCalled = true, + ), + ], + ), + ), + ), + ); + Focus.of(childKey.currentContext, nullOk: true).requestFocus(); + await tester.pump(); + + expect(Focus.of(childKey.currentContext, nullOk: true).hasPrimaryFocus, isFalse); + expect(itemBuilderCalled, isFalse); + expect(onSelectedCalled, isFalse); + }); + + testWidgets('PopupMenuItem is only focusable when enabled', (WidgetTester tester) async { + final Key popupButtonKey = UniqueKey(); + final GlobalKey childKey = GlobalKey(); + bool itemBuilderCalled = false; + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Column( + children: [ + PopupMenuButton( + key: popupButtonKey, + itemBuilder: (BuildContext context) { + itemBuilderCalled = true; + return >[ + PopupMenuItem( + enabled: true, + value: 1, + child: Text('Tap me please!', key: childKey), + ), + ]; + }, + ), + ], + ), + ), + ), + ); + + // Open the popup to build and show the menu contents. + await tester.tap(find.byKey(popupButtonKey)); + await tester.pumpAndSettle(); + final FocusNode childNode = Focus.of(childKey.currentContext, nullOk: true); + // Now that the contents are shown, request focus on the child text. + childNode.requestFocus(); + await tester.pumpAndSettle(); + expect(itemBuilderCalled, isTrue); + + // Make sure that the focus went where we expected it to. + expect(childNode.hasPrimaryFocus, isTrue); + itemBuilderCalled = false; + + // Close the popup. + await tester.tap(find.byKey(popupButtonKey)); + await tester.pumpAndSettle(); + + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Column( + children: [ + PopupMenuButton( + key: popupButtonKey, + itemBuilder: (BuildContext context) { + itemBuilderCalled = true; + return >[ + PopupMenuItem( + enabled: false, + value: 1, + child: Text('Tap me please!', key: childKey), + ), + ]; + }, + ), + ], + ), + ), + ), + ); + await tester.pumpAndSettle(); + // Open the popup again to rebuild the contents with enabled == false. + await tester.tap(find.byKey(popupButtonKey)); + await tester.pumpAndSettle(); + + expect(itemBuilderCalled, isTrue); + expect(Focus.of(childKey.currentContext, nullOk: true).hasPrimaryFocus, isFalse); + }); + testWidgets('PopupMenuButton is horizontal on iOS', (WidgetTester tester) async { Widget build(TargetPlatform platform) { return MaterialApp( diff --git a/packages/flutter/test/material/stepper_test.dart b/packages/flutter/test/material/stepper_test.dart index 611b6f392a..79a93e8d2f 100644 --- a/packages/flutter/test/material/stepper_test.dart +++ b/packages/flutter/test/material/stepper_test.dart @@ -605,4 +605,56 @@ void main() { expect(find.text('Text After Stepper'), findsNothing); }); + + testWidgets("Vertical Stepper can't be focused when disabled.", (WidgetTester tester) async { + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Stepper( + currentStep: 0, + type: StepperType.vertical, + steps: const [ + Step( + title: Text('Step 0'), + state: StepState.disabled, + content: Text('Text 0'), + ), + ], + ), + ), + ), + ); + await tester.pump(); + + final FocusNode disabledNode = Focus.of(tester.element(find.text('Step 0')), nullOk: true, scopeOk: true); + disabledNode.requestFocus(); + await tester.pump(); + expect(disabledNode.hasPrimaryFocus, isFalse); + }); + + testWidgets("Horizontal Stepper can't be focused when disabled.", (WidgetTester tester) async { + await tester.pumpWidget( + MaterialApp( + home: Material( + child: Stepper( + currentStep: 0, + type: StepperType.horizontal, + steps: const [ + Step( + title: Text('Step 0'), + state: StepState.disabled, + content: Text('Text 0'), + ), + ], + ), + ), + ), + ); + await tester.pump(); + + final FocusNode disabledNode = Focus.of(tester.element(find.text('Step 0')), nullOk: true, scopeOk: true); + disabledNode.requestFocus(); + await tester.pump(); + expect(disabledNode.hasPrimaryFocus, isFalse); + }); }