From 55fd5f1561b4590c20d5f51ae1064e62080ed74c Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 8 Aug 2019 13:36:42 -0700 Subject: [PATCH] Fix mouse region crash when using closures (#37342) This PR fixes an issue where MouseRegion crashes when being passed with closures instead of methods. It changes how a RenderMouseRegion handles its MouseTrackingAnnotation. Instead of creating a new annotation every time it becomes active and destroys it when deactivated, it now creates an annotation during the constructor and holds onto it until the end of its lifecycle. Instead of directly passing the argument callbacks to the annotation, it proxies them using methods. --- .../flutter/lib/src/rendering/proxy_box.dart | 105 ++++++++---------- packages/flutter/test/material/page_test.dart | 4 +- .../test/widgets/mouse_region_test.dart | 45 ++++++++ 3 files changed, 92 insertions(+), 62 deletions(-) diff --git a/packages/flutter/lib/src/rendering/proxy_box.dart b/packages/flutter/lib/src/rendering/proxy_box.dart index 67725daade..c02f69033e 100644 --- a/packages/flutter/lib/src/rendering/proxy_box.dart +++ b/packages/flutter/lib/src/rendering/proxy_box.dart @@ -2568,15 +2568,13 @@ class RenderMouseRegion extends RenderProxyBox { }) : _onEnter = onEnter, _onHover = onHover, _onExit = onExit, + _annotationIsActive = false, super(child) { - if (_onEnter != null || _onHover != null || _onExit != null) { - _hoverAnnotation = MouseTrackerAnnotation( - onEnter: _onEnter, - onHover: _onHover, - onExit: _onExit, - ); - } - _mouseIsConnected = RendererBinding.instance.mouseTracker.mouseIsConnected; + _hoverAnnotation = MouseTrackerAnnotation( + onEnter: _handleEnter, + onHover: _handleHover, + onExit: _handleExit, + ); } /// Called when a hovering pointer enters the region for this widget. @@ -2591,6 +2589,10 @@ class RenderMouseRegion extends RenderProxyBox { } } PointerEnterEventListener _onEnter; + void _handleEnter(PointerEnterEvent event) { + if (_onEnter != null) + _onEnter(event); + } /// Called when a pointer that has not triggered an [onPointerDown] changes /// position. @@ -2604,6 +2606,10 @@ class RenderMouseRegion extends RenderProxyBox { } } PointerHoverEventListener _onHover; + void _handleHover(PointerHoverEvent event) { + if (_onHover != null) + _onHover(event); + } /// Called when a hovering pointer leaves the region for this widget. /// @@ -2617,6 +2623,10 @@ class RenderMouseRegion extends RenderProxyBox { } } PointerExitEventListener _onExit; + void _handleExit(PointerExitEvent event) { + if (_onExit != null) + _onExit(event); + } // Object used for annotation of the layer used for hover hit detection. MouseTrackerAnnotation _hoverAnnotation; @@ -2629,45 +2639,22 @@ class RenderMouseRegion extends RenderProxyBox { MouseTrackerAnnotation get hoverAnnotation => _hoverAnnotation; void _updateAnnotations() { - assert(_hoverAnnotation == null || _onEnter != _hoverAnnotation.onEnter || _onHover != _hoverAnnotation.onHover || _onExit != _hoverAnnotation.onExit, - "Shouldn't call _updateAnnotations if nothing has changed."); - bool changed = false; - final bool hadHoverAnnotation = _hoverAnnotation != null; - if (_hoverAnnotation != null && attached) { - RendererBinding.instance.mouseTracker.detachAnnotation(_hoverAnnotation); - changed = true; - } - if (_onEnter != null || _onHover != null || _onExit != null) { - _hoverAnnotation = MouseTrackerAnnotation( - onEnter: _onEnter, - onHover: _onHover, - onExit: _onExit, - ); - if (attached) { - RendererBinding.instance.mouseTracker.attachAnnotation(_hoverAnnotation); - changed = true; - } - } else { - _hoverAnnotation = null; - } - if (changed) { + final bool annotationWasActive = _annotationIsActive; + final bool annotationWillBeActive = ( + _onEnter != null || + _onHover != null || + _onExit != null + ) && + RendererBinding.instance.mouseTracker.mouseIsConnected; + if (annotationWasActive != annotationWillBeActive) { markNeedsPaint(); - } - final bool hasHoverAnnotation = _hoverAnnotation != null; - if (hadHoverAnnotation != hasHoverAnnotation) { markNeedsCompositingBitsUpdate(); - } - } - - bool _mouseIsConnected; - void _handleMouseTrackerChanged() { - final bool newState = RendererBinding.instance.mouseTracker.mouseIsConnected; - if (newState != _mouseIsConnected) { - _mouseIsConnected = newState; - if (_hoverAnnotation != null) { - markNeedsCompositingBitsUpdate(); - markNeedsPaint(); + if (annotationWillBeActive) { + RendererBinding.instance.mouseTracker.attachAnnotation(_hoverAnnotation); + } else { + RendererBinding.instance.mouseTracker.detachAnnotation(_hoverAnnotation); } + _annotationIsActive = annotationWillBeActive; } } @@ -2675,52 +2662,50 @@ class RenderMouseRegion extends RenderProxyBox { void attach(PipelineOwner owner) { super.attach(owner); // Add a listener to listen for changes in mouseIsConnected. - RendererBinding.instance.mouseTracker.addListener(_handleMouseTrackerChanged); - postActivate(); + RendererBinding.instance.mouseTracker.addListener(_updateAnnotations); + _updateAnnotations(); } /// Attaches the annotation for this render object, if any. /// - /// This is called by [attach] to attach any new annotations. - /// - /// This is also called by the [Listener]'s [Element] to tell this - /// [RenderPointerListener] that it will shortly be attached. That way, + /// This is called by the [MouseRegion]'s [Element] to tell this + /// [RenderMouseRegion] that it has transitioned from "inactive" + /// state to "active". We call it here so that /// [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) { + if (_annotationIsActive) RendererBinding.instance.mouseTracker.attachAnnotation(_hoverAnnotation); - } } /// 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, + /// This is called by the [MouseRegion]'s [Element] to tell this + /// [RenderMouseRegion] that it will shortly be transitioned from "active" + /// state to "inactive". We call it here so that /// [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) { + if (_annotationIsActive) RendererBinding.instance.mouseTracker.detachAnnotation(_hoverAnnotation); - } } @override void detach() { - RendererBinding.instance.mouseTracker.removeListener(_handleMouseTrackerChanged); + RendererBinding.instance.mouseTracker.removeListener(_updateAnnotations); super.detach(); } - bool get _hasActiveAnnotation => _hoverAnnotation != null && _mouseIsConnected; + bool _annotationIsActive; @override - bool get needsCompositing => super.needsCompositing || _hasActiveAnnotation; + bool get needsCompositing => super.needsCompositing || _annotationIsActive; @override void paint(PaintingContext context, Offset offset) { - if (_hasActiveAnnotation) { + if (_annotationIsActive) { final AnnotatedRegionLayer layer = AnnotatedRegionLayer( _hoverAnnotation, size: size, diff --git a/packages/flutter/test/material/page_test.dart b/packages/flutter/test/material/page_test.dart index d3e9cce848..ee9f32d977 100644 --- a/packages/flutter/test/material/page_test.dart +++ b/packages/flutter/test/material/page_test.dart @@ -297,7 +297,7 @@ void main() { ), ); await tester.tap(find.text('PUSH')); - expect(await tester.pumpAndSettle(const Duration(minutes: 1)), 3); + expect(await tester.pumpAndSettle(const Duration(minutes: 1)), 2); expect(find.text('PUSH'), findsNothing); expect(find.text('HELLO'), findsOneWidget); final Offset helloPosition1 = tester.getCenter(find.text('HELLO')); @@ -342,7 +342,7 @@ void main() { expect(helloPosition3.dy, helloPosition4.dy); await gesture.moveBy(const Offset(500.0, 0.0)); await gesture.up(); - expect(await tester.pumpAndSettle(const Duration(minutes: 1)), 3); + expect(await tester.pumpAndSettle(const Duration(minutes: 1)), 2); expect(find.text('PUSH'), findsOneWidget); expect(find.text('HELLO'), findsNothing); }); diff --git a/packages/flutter/test/widgets/mouse_region_test.dart b/packages/flutter/test/widgets/mouse_region_test.dart index 91991baa41..5d62bbffbd 100644 --- a/packages/flutter/test/widgets/mouse_region_test.dart +++ b/packages/flutter/test/widgets/mouse_region_test.dart @@ -276,6 +276,7 @@ void main() { final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse); addTearDown(gesture.removePointer); await gesture.moveTo(const Offset(400.0, 0.0)); + addTearDown(gesture.removePointer); await tester.pump(); await tester.pumpWidget( Column( @@ -424,6 +425,7 @@ void main() { expect(bottomLeft.dy - topLeft.dy, scaleFactor * localHeight); final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse); + addTearDown(gesture.removePointer); await gesture.addPointer(); addTearDown(gesture.removePointer); await gesture.moveTo(topLeft - const Offset(1, 1)); @@ -452,6 +454,7 @@ void main() { testWidgets('needsCompositing updates correctly and is respected', (WidgetTester tester) async { // Pretend that we have a mouse connected. final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse); + addTearDown(gesture.removePointer); await gesture.addPointer(); addTearDown(gesture.removePointer); @@ -497,6 +500,7 @@ void main() { testWidgets("Callbacks aren't called during build", (WidgetTester tester) async { final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse); + addTearDown(gesture.removePointer); await gesture.addPointer(); addTearDown(gesture.removePointer); @@ -537,6 +541,7 @@ void main() { testWidgets("MouseRegion activate/deactivate don't duplicate annotations", (WidgetTester tester) async { final GlobalKey feedbackKey = GlobalKey(); final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse); + addTearDown(gesture.removePointer); await gesture.addPointer(); addTearDown(gesture.removePointer); @@ -622,6 +627,24 @@ void main() { expect(exit.single.position, const Offset(400.0, 300.0)); expect(exit.single.delta, const Offset(0.0, 0.0)); }); + + testWidgets('detects pointer enter with closure arguments', (WidgetTester tester) async { + await tester.pumpWidget(_HoverClientWithClosures()); + expect(find.text('not hovering'), findsOneWidget); + + final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse); + addTearDown(gesture.removePointer); + await gesture.addPointer(); + // Move to a position out of MouseRegion + await gesture.moveTo(tester.getBottomRight(find.byType(MouseRegion)) + const Offset(10, -10)); + await tester.pumpAndSettle(); + expect(find.text('not hovering'), findsOneWidget); + + // Move into MouseRegion + await gesture.moveBy(const Offset(-20, 0)); + await tester.pumpAndSettle(); + expect(find.text('HOVERING'), findsOneWidget); + }); }); group('MouseRegion paints child once and only once', () { @@ -712,3 +735,25 @@ class _PaintCallbackObject extends RenderProxyBox { super.paint(context, offset); } } + +class _HoverClientWithClosures extends StatefulWidget { + @override + _HoverClientWithClosuresState createState() => _HoverClientWithClosuresState(); +} + +class _HoverClientWithClosuresState extends State<_HoverClientWithClosures> { + + bool _hovering = false; + + @override + Widget build(BuildContext context) { + return Directionality( + textDirection: TextDirection.ltr, + child: MouseRegion( + onEnter: (PointerEnterEvent _) { setState(() { _hovering = true; }); }, + onExit: (PointerExitEvent _) { setState(() { _hovering = false; }); }, + child: Text(_hovering ? 'HOVERING' : 'not hovering'), + ), + ); + } +}