Fix initial value for highlight mode on desktop platforms. (flutter#54306)

gspencergoog · web-flow · commit c7b10df76558 · 2020-04-10T13:23:55.000-07:00
This fixes the initial value of FocusManager.highlightMode so that it gets initialized correctly on desktop platforms. My recent update of this code (flutter#52990) broke things so that the highlight mode never changed from the initial default of touch, which meant that focus highlights didn't show unless you set FocusManager.highlightStrategy to something (even automatic, the default: setting it caused the mode to be updated).
diff --git a/packages/flutter/lib/src/widgets/focus_manager.dart b/packages/flutter/lib/src/widgets/focus_manager.dart
@@ -1367,35 +1367,6 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
   /// the [WidgetsBinding] instance.
   static FocusManager get instance => WidgetsBinding.instance.focusManager;
 
-  bool get _lastInteractionWasTouch {
-    // Assume that if we're on one of these mobile platforms, or if there's no
-    // mouse connected, that the initial interaction will be touch-based, and
-    // that it's traditional mouse and keyboard on all others.
-    //
-    // This only affects the initial value: the ongoing value is updated to a
-    // known correct value as soon as any pointer events are received.
-    if (_lastInteractionWasTouchValue == null) {
-      switch (defaultTargetPlatform) {
-        case TargetPlatform.android:
-        case TargetPlatform.fuchsia:
-        case TargetPlatform.iOS:
-          _lastInteractionWasTouchValue = !WidgetsBinding.instance.mouseTracker.mouseIsConnected;
-          break;
-        case TargetPlatform.linux:
-        case TargetPlatform.macOS:
-        case TargetPlatform.windows:
-          _lastInteractionWasTouchValue = false;
-          break;
-      }
-    }
-    return _lastInteractionWasTouchValue;
-  }
-  bool _lastInteractionWasTouchValue;
-  set _lastInteractionWasTouch(bool value) {
-    _lastInteractionWasTouchValue = value;
-  }
-
-
   /// Sets the strategy by which [highlightMode] is determined.
   ///
   /// If set to [FocusHighlightStrategy.automatic], then the highlight mode will
@@ -1427,6 +1398,30 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
     _updateHighlightMode();
   }
 
+  static FocusHighlightMode get _defaultModeForPlatform {
+    // Assume that if we're on one of the mobile platforms, and there's no mouse
+    // connected, that the initial interaction will be touch-based, and that
+    // it's traditional mouse and keyboard on all other platforms.
+    //
+    // This only affects the initial value: the ongoing value is updated to a
+    // known correct value as soon as any pointer/keyboard events are received.
+    switch (defaultTargetPlatform) {
+      case TargetPlatform.android:
+      case TargetPlatform.fuchsia:
+      case TargetPlatform.iOS:
+        if (WidgetsBinding.instance.mouseTracker.mouseIsConnected) {
+          return FocusHighlightMode.traditional;
+        }
+        return FocusHighlightMode.touch;
+      case TargetPlatform.linux:
+      case TargetPlatform.macOS:
+      case TargetPlatform.windows:
+        return FocusHighlightMode.traditional;
+    }
+    assert(false, 'Unhandled target platform $defaultTargetPlatform');
+    return null;
+  }
+
   /// Indicates the current interaction mode for focus highlights.
   ///
   /// The value returned depends upon the [highlightStrategy] used, and possibly
@@ -1438,15 +1433,28 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
   ///
   /// If [highlightMode] returns [FocusHighlightMode.traditional], then widgets should
   /// draw their focus highlight whenever they are focused.
-  FocusHighlightMode get highlightMode => _highlightMode;
-  FocusHighlightMode _highlightMode = FocusHighlightMode.touch;
+  // Don't want to set _highlightMode here, since it's possible for the target
+  // platform to change (especially in tests).
+  FocusHighlightMode get highlightMode => _highlightMode ?? _defaultModeForPlatform;
+  FocusHighlightMode _highlightMode;
+
+  // If set, indicates if the last interaction detected was touch or not.
+  // If null, no interactions have occurred yet.
+  bool _lastInteractionWasTouch;
 
   // Update function to be called whenever the state relating to highlightMode
   // changes.
   void _updateHighlightMode() {
     FocusHighlightMode newMode;
     switch (highlightStrategy) {
       case FocusHighlightStrategy.automatic:
+        if (_lastInteractionWasTouch == null) {
+          // If we don't have any information about the last interaction yet,
+          // then just rely on the default value for the platform, which will be
+          // determined based on the target platform if _highlightMode is not
+          // set.
+          return;
+        }
         if (_lastInteractionWasTouch) {
           newMode = FocusHighlightMode.touch;
         } else {
@@ -1460,8 +1468,12 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
         newMode = FocusHighlightMode.traditional;
         break;
     }
-    if (newMode != _highlightMode) {
-      _highlightMode = newMode;
+    // We can't just compare newMode with _highlightMode here, since
+    // _highlightMode could be null, so we want to compare with the return value
+    // for the getter, since that's what clients will be looking at.
+    final FocusHighlightMode oldMode = highlightMode;
+    _highlightMode = newMode;
+    if (highlightMode != oldMode) {
       _notifyHighlightModeListeners();
     }
   }
@@ -1485,7 +1497,7 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
     for (final ValueChanged<FocusHighlightMode> listener in localListeners) {
       try {
         if (_listeners.contains(listener)) {
-          listener(_highlightMode);
+          listener(highlightMode);
         }
       } catch (exception, stack) {
         InformationCollector collector;
@@ -1517,31 +1529,30 @@ class FocusManager with DiagnosticableTreeMixin, ChangeNotifier {
   final FocusScopeNode rootScope = FocusScopeNode(debugLabel: 'Root Focus Scope');
 
   void _handlePointerEvent(PointerEvent event) {
-    bool currentInteractionIsTouch;
+    FocusHighlightMode expectedMode;
     switch (event.kind) {
       case PointerDeviceKind.touch:
       case PointerDeviceKind.stylus:
       case PointerDeviceKind.invertedStylus:
-        currentInteractionIsTouch = true;
+        _lastInteractionWasTouch = true;
+        expectedMode = FocusHighlightMode.touch;
         break;
       case PointerDeviceKind.mouse:
       case PointerDeviceKind.unknown:
-        currentInteractionIsTouch = false;
+        _lastInteractionWasTouch = false;
+        expectedMode = FocusHighlightMode.traditional;
         break;
     }
-    if (_lastInteractionWasTouch != currentInteractionIsTouch) {
-      _lastInteractionWasTouch = currentInteractionIsTouch;
+    if (expectedMode != highlightMode) {
       _updateHighlightMode();
     }
   }
 
   void _handleRawKeyEvent(RawKeyEvent event) {
     // Update highlightMode first, since things responding to the keys might
     // look at the highlight mode, and it should be accurate.
-    if (_lastInteractionWasTouch) {
-      _lastInteractionWasTouch = false;
-      _updateHighlightMode();
-    }
+    _lastInteractionWasTouch = false;
+    _updateHighlightMode();
 
     assert(_focusDebug('Received key event ${event.logicalKey}'));
     // Walk the current focus from the leaf to the root, calling each one's
diff --git a/packages/flutter/test/widgets/focus_manager_test.dart b/packages/flutter/test/widgets/focus_manager_test.dart
@@ -5,6 +5,7 @@
 import 'package:flutter/foundation.dart';
 import 'package:flutter/gestures.dart';
 import 'package:flutter/material.dart';
+import 'package:flutter/rendering.dart';
 import 'package:flutter/services.dart';
 import 'package:flutter/widgets.dart';
 import 'package:flutter_test/flutter_test.dart';
@@ -843,6 +844,26 @@ void main() {
           break;
       }
     }, variant: TargetPlatformVariant.all());
+    testWidgets('Mouse events change initial focus highlight mode on mobile.', (WidgetTester tester) async {
+      expect(FocusManager.instance.highlightMode, equals(FocusHighlightMode.touch));
+      RendererBinding.instance.initMouseTracker(); // Clear out the mouse state.
+      final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse, pointer: 0);
+      addTearDown(gesture.removePointer);
+      await gesture.moveTo(Offset.zero);
+      expect(FocusManager.instance.highlightMode, equals(FocusHighlightMode.traditional));
+    }, variant: TargetPlatformVariant.mobile());
+    testWidgets('Mouse events change initial focus highlight mode on desktop.', (WidgetTester tester) async {
+      expect(FocusManager.instance.highlightMode, equals(FocusHighlightMode.traditional));
+      RendererBinding.instance.initMouseTracker(); // Clear out the mouse state.
+      final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse, pointer: 0);
+      addTearDown(gesture.removePointer);
+      await gesture.moveTo(Offset.zero);
+      expect(FocusManager.instance.highlightMode, equals(FocusHighlightMode.traditional));
+    }, variant: TargetPlatformVariant.desktop());
+    testWidgets('Keyboard events change initial focus highlight mode.', (WidgetTester tester) async {
+      await tester.sendKeyEvent(LogicalKeyboardKey.enter);
+      expect(FocusManager.instance.highlightMode, equals(FocusHighlightMode.traditional));
+    }, variant: TargetPlatformVariant.all());
     testWidgets('Events change focus highlight mode.', (WidgetTester tester) async {
       await setupWidget(tester);
       int callCount = 0;
diff --git a/packages/flutter_test/lib/src/widget_tester.dart b/packages/flutter_test/lib/src/widget_tester.dart
@@ -225,6 +225,22 @@ class TargetPlatformVariant extends TestVariant<TargetPlatform> {
   /// the [TargetPlatform] enum.
   TargetPlatformVariant.all() : values = TargetPlatform.values.toSet();
 
+  /// Creates a [TargetPlatformVariant] that includes platforms that are
+  /// considered desktop platforms.
+  TargetPlatformVariant.desktop() : values = <TargetPlatform>{
+    TargetPlatform.linux,
+    TargetPlatform.macOS,
+    TargetPlatform.windows,
+  };
+
+  /// Creates a [TargetPlatformVariant] that includes platforms that are
+  /// considered mobile platforms.
+  TargetPlatformVariant.mobile() : values = <TargetPlatform>{
+    TargetPlatform.android,
+    TargetPlatform.iOS,
+    TargetPlatform.fuchsia,
+  };
+
   /// Creates a [TargetPlatformVariant] that tests only the given value of
   /// [TargetPlatform].
   TargetPlatformVariant.only(TargetPlatform platform) : values = <TargetPlatform>{platform};
diff --git a/packages/flutter_test/test/widget_tester_test.dart b/packages/flutter_test/test/widget_tester_test.dart
@@ -743,6 +743,13 @@ void main() {
         numberOfVariationsRun += 1;
       }
     }, variant: TargetPlatformVariant.all());
+
+    testWidgets('TargetPlatformVariant.desktop + mobile contains all TargetPlatform values', (WidgetTester tester) async {
+      final TargetPlatformVariant all = TargetPlatformVariant.all();
+      final TargetPlatformVariant desktop = TargetPlatformVariant.all();
+      final TargetPlatformVariant mobile = TargetPlatformVariant.all();
+      expect(desktop.values.union(mobile.values), equals(all.values));
+    });
   });
 }
 

Original file line number	Diff line number	Diff line change
`@@ -743,6 +743,13 @@ void main() {`
`743`	`743`	`numberOfVariationsRun += 1;`
`744`	`744`	`}`
`745`	`745`	`}, variant: TargetPlatformVariant.all());`
	`746`	`+`
	`747`	`+ testWidgets('TargetPlatformVariant.desktop + mobile contains all TargetPlatform values', (WidgetTester tester) async {`
	`748`	`+ final TargetPlatformVariant all = TargetPlatformVariant.all();`
	`749`	`+ final TargetPlatformVariant desktop = TargetPlatformVariant.all();`
	`750`	`+ final TargetPlatformVariant mobile = TargetPlatformVariant.all();`
	`751`	`+ expect(desktop.values.union(mobile.values), equals(all.values));`
	`752`	`+ });`
`746`	`753`	`});`
`747`	`754`	`}`
`748`	`755`