From 23baae0e45a56788cb543748889de711583ecbfd Mon Sep 17 00:00:00 2001 From: Greg Spencer Date: Wed, 8 May 2019 12:20:31 -0700 Subject: [PATCH] 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. --- .../lib/src/gestures/mouse_tracking.dart | 9 +- .../flutter/lib/src/rendering/proxy_box.dart | 50 +++++-- packages/flutter/lib/src/widgets/basic.dart | 21 +++ .../flutter/test/widgets/listener_test.dart | 128 +++++++++++++++++- 4 files changed, 195 insertions(+), 13 deletions(-) diff --git a/packages/flutter/lib/src/gestures/mouse_tracking.dart b/packages/flutter/lib/src/gestures/mouse_tracking.dart index 0c6c619cf0..7f6d30f053 100644 --- a/packages/flutter/lib/src/gestures/mouse_tracking.dart +++ b/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(); } } diff --git a/packages/flutter/lib/src/rendering/proxy_box.dart b/packages/flutter/lib/src/rendering/proxy_box.dart index 1037cd4a70..9e56f80a37 100644 --- a/packages/flutter/lib/src/rendering/proxy_box.dart +++ b/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 layer = AnnotatedRegionLayer( _hoverAnnotation, size: size, diff --git a/packages/flutter/lib/src/widgets/basic.dart b/packages/flutter/lib/src/widgets/basic.dart index dca7fa55be..8b07d9d73a 100644 --- a/packages/flutter/lib/src/widgets/basic.dart +++ b/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 diff --git a/packages/flutter/test/widgets/listener_test.dart b/packages/flutter/test/widgets/listener_test.dart index b07e49912c..e244c4c40f 100644 --- a/packages/flutter/test/widgets/listener_test.dart +++ b/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 onHover; + final Widget child; + + @override + HoverClientState createState() => HoverClientState(); +} + +class HoverClientState extends State { + 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 { + 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 log = []; @@ -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(), 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)); + }); }); }