Fix RenderPointerListener so that callbacks aren't called at the wrong time. (#32142)

I recently added some code to keep hover events from being propagated when a mouse wasn't attached. While that works, there are times when it can fire callbacks during the building of other components, since they can now be called from detach/attach. This is not ideal, since it will assert then. This changes the code so that it won't update the annotations during attach/detach, but also won't push the annotation layer unless a mouse is connected, achieving the same result as before, but with better semantics.

The basic problem is that in the detach for RenderPointerListener, it would detach the annotation, which could cause onExit to be called on the annotation, since the widget was disappearing under the mouse, and thus needs to receive an onExit, but that onExit might be (and probably will be) calling setState, which marks the owning widget as needing to be built, sometimes when it already has been.

The fix creates a new _ListenerElement that overrides activate and deactivate in order to tell the render object ahead of the detach that it might be detached, and so the onExit gets called before the detach instead of during it.

In addition, I now avoid scheduling more than one check for mouse positions per frame.

packages/flutter/lib/src/gestures/mouse_tracking.dart

@@ -131,12 +131,17 @@ class MouseTracker extends ChangeNotifier {
     _trackedAnnotations.remove(annotation);
   }
 
+  bool _postFrameCheckScheduled = false;
   void _scheduleMousePositionCheck() {
     // If we're not tracking anything, then there is no point in registering a
     // frame callback or scheduling a frame. By definition there are no active
     // annotations that need exiting, either.
-    if (_trackedAnnotations.isNotEmpty) {
-      SchedulerBinding.instance.addPostFrameCallback((Duration _) => collectMousePositions());
+    if (_trackedAnnotations.isNotEmpty && !_postFrameCheckScheduled) {
+      _postFrameCheckScheduled = true;
+      SchedulerBinding.instance.addPostFrameCallback((Duration _) {
+        _postFrameCheckScheduled = false;
+        collectMousePositions();
+      });
       SchedulerBinding.instance.scheduleFrame();
     }
   }

packages/flutter/lib/src/rendering/proxy_box.dart

@@ -2571,8 +2571,7 @@ class RenderPointerListener extends RenderProxyBoxWithHitTestBehavior {
       RendererBinding.instance.mouseTracker.detachAnnotation(_hoverAnnotation);
       changed = true;
     }
-    if (RendererBinding.instance.mouseTracker.mouseIsConnected &&
-        (_onPointerEnter != null || _onPointerHover != null || _onPointerExit != null)) {
+    if (_onPointerEnter != null || _onPointerHover != null || _onPointerExit != null) {
       _hoverAnnotation = MouseTrackerAnnotation(
         onEnter: _onPointerEnter,
         onHover: _onPointerHover,
@@ -2594,31 +2593,64 @@ class RenderPointerListener extends RenderProxyBoxWithHitTestBehavior {
     }
   }
 
+  void _handleMouseTrackerChanged() {
+    if (attached)
+      markNeedsPaint();
+  }
+
   @override
   void attach(PipelineOwner owner) {
     super.attach(owner);
     // Add a listener to listen for changes in mouseIsConnected.
-    RendererBinding.instance.mouseTracker.addListener(_updateAnnotations);
+    RendererBinding.instance.mouseTracker.addListener(_handleMouseTrackerChanged);
+    postActivate();
+  }
+
+  /// Attaches the annotation for this render object, if any.
+  ///
+  /// This is called by [attach] to attach and new annotations.
+  ///
+  /// This is also called by the [Listener]'s [Element] to tell this
+  /// [RenderPointerListener] that it will shortly be attached. That way,
+  /// [MouseTrackerAnnotation.onEnter] isn't called during the build step for
+  /// the widget that provided the callback, and [State.setState] can safely be
+  /// called within that callback.
+  void postActivate() {
     if (_hoverAnnotation != null) {
       RendererBinding.instance.mouseTracker.attachAnnotation(_hoverAnnotation);
     }
   }
 
-  @override
-  void detach() {
+  /// Detaches the annotation for this render object, if any.
+  ///
+  /// This is called by the [Listener]'s [Element] to tell this
+  /// [RenderPointerListener] that it will shortly be attached. That way,
+  /// [MouseTrackerAnnotation.onExit] isn't called during the build step for the
+  /// widget that provided the callback, and [State.setState] can safely be
+  /// called within that callback.
+  void preDeactivate() {
     if (_hoverAnnotation != null) {
       RendererBinding.instance.mouseTracker.detachAnnotation(_hoverAnnotation);
     }
-    RendererBinding.instance.mouseTracker.removeListener(_updateAnnotations);
-    super.detach();
   }
 
   @override
-  bool get needsCompositing => _hoverAnnotation != null;
+  void detach() {
+    RendererBinding.instance.mouseTracker.removeListener(_handleMouseTrackerChanged);
+    super.detach();
+  }
+
+  bool get _hasActiveAnnotation {
+    return _hoverAnnotation != null
+      && RendererBinding.instance.mouseTracker.mouseIsConnected;
+  }
+
+  @override
+  bool get needsCompositing => _hasActiveAnnotation;
 
   @override
   void paint(PaintingContext context, Offset offset) {
-    if (_hoverAnnotation != null) {
+    if (_hasActiveAnnotation) {
       final AnnotatedRegionLayer<MouseTrackerAnnotation> layer = AnnotatedRegionLayer<MouseTrackerAnnotation>(
         _hoverAnnotation,
         size: size,

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

@@ -5401,6 +5401,9 @@ class Listener extends SingleChildRenderObjectWidget {
   /// How to behave during hit testing.
   final HitTestBehavior behavior;
 
+  @override
+  _ListenerElement createElement() => _ListenerElement(this);
+
   @override
   RenderPointerListener createRenderObject(BuildContext context) {
     return RenderPointerListener(
@@ -5455,6 +5458,24 @@ class Listener extends SingleChildRenderObjectWidget {
   }
 }
 
+class _ListenerElement extends SingleChildRenderObjectElement {
+  _ListenerElement(SingleChildRenderObjectWidget widget) : super(widget);
+
+  @override
+  void activate() {
+    super.activate();
+    final RenderPointerListener renderPointerListener = renderObject;
+    renderPointerListener.postActivate();
+  }
+
+  @override
+  void deactivate() {
+    final RenderPointerListener renderPointerListener = renderObject;
+    renderPointerListener.preDeactivate();
+    super.deactivate();
+  }
+}
+
 /// A widget that creates a separate display list for its child.
 ///
 /// This widget creates a separate display list for its child, which

packages/flutter/test/widgets/listener_test.dart

@@ -8,6 +8,67 @@ import 'package:flutter_test/flutter_test.dart';
 import 'package:flutter/widgets.dart';
 import 'package:flutter/gestures.dart';
 
+
+class HoverClient extends StatefulWidget {
+  const HoverClient({Key key, this.onHover, this.child}) : super(key: key);
+
+  final ValueChanged<bool> onHover;
+  final Widget child;
+
+  @override
+  HoverClientState createState() => HoverClientState();
+}
+
+class HoverClientState extends State<HoverClient> {
+  static int numEntries = 0;
+  static int numExits = 0;
+
+  void _onExit(PointerExitEvent details) {
+    numExits++;
+    if (widget.onHover != null) {
+      widget.onHover(false);
+    }
+  }
+
+  void _onEnter(PointerEnterEvent details) {
+    numEntries++;
+    if (widget.onHover != null) {
+      widget.onHover(true);
+    }
+  }
+
+  @override
+  Widget build(BuildContext context) {
+    return Listener(
+      onPointerEnter: _onEnter,
+      onPointerExit: _onExit,
+      child: widget.child,
+    );
+  }
+}
+
+class HoverFeedback extends StatefulWidget {
+  HoverFeedback({Key key}) : super(key: key);
+
+  @override
+  _HoverFeedbackState createState() => _HoverFeedbackState();
+}
+
+class _HoverFeedbackState extends State<HoverFeedback> {
+  bool _hovering = false;
+
+  @override
+  Widget build(BuildContext context) {
+    return Directionality(
+      textDirection: TextDirection.ltr,
+      child: HoverClient(
+        onHover: (bool hovering) => setState(() => _hovering = hovering),
+        child: Text(_hovering ? 'HOVERING' : 'not hovering'),
+      ),
+    );
+  }
+}
+
 void main() {
   testWidgets('Events bubble up the tree', (WidgetTester tester) async {
     final List<String> log = <String>[];
@@ -44,6 +105,11 @@ void main() {
   });
 
   group('Listener hover detection', () {
+    setUp((){
+      HoverClientState.numExits = 0;
+      HoverClientState.numEntries = 0;
+    });
+
     testWidgets('detects pointer enter', (WidgetTester tester) async {
       PointerEnterEvent enter;
       PointerHoverEvent move;
@@ -302,8 +368,8 @@ void main() {
 
     testWidgets('needsCompositing updates correctly and is respected', (WidgetTester tester) async {
       // Pretend that we have a mouse connected.
-      final TestGesture gesture = await tester.startGesture(Offset.zero, kind: PointerDeviceKind.mouse);
-      await gesture.up();
+      final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse);
+      await gesture.addPointer();
 
       await tester.pumpWidget(
         Transform.scale(
@@ -348,5 +414,63 @@ void main() {
       // executed directly on the canvas.
       expect(tester.layers.whereType<TransformLayer>(), hasLength(1));
     });
+
+    testWidgets("Callbacks aren't called during build", (WidgetTester tester) async {
+      final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse);
+      await gesture.addPointer();
+
+      await tester.pumpWidget(
+        Center(child: HoverFeedback()),
+      );
+
+      await gesture.moveTo(tester.getCenter(find.byType(Text)));
+      await tester.pumpAndSettle();
+      expect(HoverClientState.numEntries, equals(1));
+      expect(HoverClientState.numExits, equals(0));
+      expect(find.text('HOVERING'), findsOneWidget);
+
+      await tester.pumpWidget(
+        Container(),
+      );
+      await tester.pump();
+      expect(HoverClientState.numEntries, equals(1));
+      expect(HoverClientState.numExits, equals(1));
+
+      await tester.pumpWidget(
+        Center(child: HoverFeedback()),
+      );
+      await tester.pump();
+      expect(HoverClientState.numEntries, equals(2));
+      expect(HoverClientState.numExits, equals(1));
+    });
+
+    testWidgets("Listener activate/deactivate don't duplicate annotations", (WidgetTester tester) async {
+      final GlobalKey feedbackKey = GlobalKey();
+      final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse);
+      await gesture.addPointer();
+
+      await tester.pumpWidget(
+        Center(child: HoverFeedback(key: feedbackKey)),
+      );
+
+      await gesture.moveTo(tester.getCenter(find.byType(Text)));
+      await tester.pumpAndSettle();
+      expect(HoverClientState.numEntries, equals(1));
+      expect(HoverClientState.numExits, equals(0));
+      expect(find.text('HOVERING'), findsOneWidget);
+
+      await tester.pumpWidget(
+        Center(child: Container(child: HoverFeedback(key: feedbackKey))),
+      );
+      await tester.pump();
+      expect(HoverClientState.numEntries, equals(2));
+      expect(HoverClientState.numExits, equals(1));
+      await tester.pumpWidget(
+        Container(),
+      );
+      await tester.pump();
+      expect(HoverClientState.numEntries, equals(2));
+      expect(HoverClientState.numExits, equals(2));
+    });
   });
 }