From 2da353a59ae756e286ae131a04e25e4ccf321f31 Mon Sep 17 00:00:00 2001 From: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Wed, 12 Jul 2023 17:35:57 -0700 Subject: [PATCH] Exclude `Tooltip`'s overlay child from SelectableRegion (#130181) Fixes https://github.com/flutter/flutter/issues/129969 by making tooltip text unselectable (for now). Also fixes some other issues uncovered when I was writing the tests. Currently `getTransformTo` only works on ancestors. I'll try to add a new method that computes the transform from 2 arbitrary render objects in the same render tree in a follow-up PR and make `Selectable` use that method instead. --- .../lib/src/material/selection_area.dart | 28 ++---- .../flutter/lib/src/material/tooltip.dart | 19 +++- packages/flutter/lib/src/widgets/overlay.dart | 1 + .../flutter/test/material/tooltip_test.dart | 91 +++++++++++++++++++ 4 files changed, 115 insertions(+), 24 deletions(-) diff --git a/packages/flutter/lib/src/material/selection_area.dart b/packages/flutter/lib/src/material/selection_area.dart index 625f3aa1afaa..2334f545cc3b 100644 --- a/packages/flutter/lib/src/material/selection_area.dart +++ b/packages/flutter/lib/src/material/selection_area.dart @@ -103,13 +103,7 @@ class SelectionArea extends StatefulWidget { } class _SelectionAreaState extends State { - FocusNode get _effectiveFocusNode { - if (widget.focusNode != null) { - return widget.focusNode!; - } - _internalNode ??= FocusNode(); - return _internalNode!; - } + FocusNode get _effectiveFocusNode => widget.focusNode ?? (_internalNode ??= FocusNode()); FocusNode? _internalNode; @override @@ -121,20 +115,12 @@ class _SelectionAreaState extends State { @override Widget build(BuildContext context) { assert(debugCheckHasMaterialLocalizations(context)); - TextSelectionControls? controls = widget.selectionControls; - switch (Theme.of(context).platform) { - case TargetPlatform.android: - case TargetPlatform.fuchsia: - controls ??= materialTextSelectionHandleControls; - case TargetPlatform.iOS: - controls ??= cupertinoTextSelectionHandleControls; - case TargetPlatform.linux: - case TargetPlatform.windows: - controls ??= desktopTextSelectionHandleControls; - case TargetPlatform.macOS: - controls ??= cupertinoDesktopTextSelectionHandleControls; - } - + final TextSelectionControls controls = widget.selectionControls ?? switch (Theme.of(context).platform) { + TargetPlatform.android || TargetPlatform.fuchsia => materialTextSelectionHandleControls, + TargetPlatform.linux || TargetPlatform.windows => desktopTextSelectionHandleControls, + TargetPlatform.iOS => cupertinoTextSelectionHandleControls, + TargetPlatform.macOS => cupertinoDesktopTextSelectionHandleControls, + }; return SelectableRegion( selectionControls: controls, focusNode: _effectiveFocusNode, diff --git a/packages/flutter/lib/src/material/tooltip.dart b/packages/flutter/lib/src/material/tooltip.dart index 045d97d767c2..2be81e1ba51c 100644 --- a/packages/flutter/lib/src/material/tooltip.dart +++ b/packages/flutter/lib/src/material/tooltip.dart @@ -482,13 +482,16 @@ class TooltipState extends State with SingleTickerProviderStateMixin { void _scheduleDismissTooltip({ required Duration withDelay }) { assert(mounted); assert( - !(_timer?.isActive ?? false) || _controller.status != AnimationStatus.reverse, + !(_timer?.isActive ?? false) || _backingController?.status != AnimationStatus.reverse, 'timer must not be active when the tooltip is fading out', ); _timer?.cancel(); _timer = null; - switch (_controller.status) { + // Use _backingController instead of _controller to prevent the lazy getter + // from instaniating an AnimationController unnecessarily. + switch (_backingController?.status) { + case null: case AnimationStatus.reverse: case AnimationStatus.dismissed: break; @@ -740,7 +743,7 @@ class TooltipState extends State with SingleTickerProviderStateMixin { }; final TooltipThemeData tooltipTheme = _tooltipTheme; - return _TooltipOverlay( + final _TooltipOverlay overlayChild = _TooltipOverlay( richMessage: widget.richMessage ?? TextSpan(text: widget.message), height: widget.height ?? tooltipTheme.height ?? _getDefaultTooltipHeight(), padding: widget.padding ?? tooltipTheme.padding ?? _getDefaultPadding(), @@ -755,13 +758,23 @@ class TooltipState extends State with SingleTickerProviderStateMixin { verticalOffset: widget.verticalOffset ?? tooltipTheme.verticalOffset ?? _defaultVerticalOffset, preferBelow: widget.preferBelow ?? tooltipTheme.preferBelow ?? _defaultPreferBelow, ); + + return SelectionContainer.maybeOf(context) == null + ? overlayChild + : SelectionContainer.disabled(child: overlayChild); } @override void dispose() { GestureBinding.instance.pointerRouter.removeGlobalRoute(_handleGlobalPointerEvent); Tooltip._openedTooltips.remove(this); + // _longPressRecognizer.dispose() and _tapRecognizer.dispose() may call + // their registered onCancel callbacks if there's a gesture in progress. + // Remove the onCancel callbacks to prevent the registered callbacks from + // triggering unnecessary side effects (such as animations). + _longPressRecognizer?.onLongPressCancel = null; _longPressRecognizer?.dispose(); + _tapRecognizer?.onTapCancel = null; _tapRecognizer?.dispose(); _timer?.cancel(); _backingController?.dispose(); diff --git a/packages/flutter/lib/src/widgets/overlay.dart b/packages/flutter/lib/src/widgets/overlay.dart index c3e71dfcafea..6b2427662a63 100644 --- a/packages/flutter/lib/src/widgets/overlay.dart +++ b/packages/flutter/lib/src/widgets/overlay.dart @@ -1569,6 +1569,7 @@ class _OverlayPortalState extends State { @override void dispose() { + assert(widget.controller._attachTarget == this); widget.controller._attachTarget = null; _locationCache?._debugMarkLocationInvalid(); _locationCache = null; diff --git a/packages/flutter/test/material/tooltip_test.dart b/packages/flutter/test/material/tooltip_test.dart index 544f88324720..84d637190d76 100644 --- a/packages/flutter/test/material/tooltip_test.dart +++ b/packages/flutter/test/material/tooltip_test.dart @@ -2278,6 +2278,97 @@ void main() { await tester.pump(const Duration(seconds: 1)); expect(element.dirty, isFalse); }); + + testWidgets('Tooltip does not initialize animation controller in dispose process', (WidgetTester tester) async { + await tester.pumpWidget( + const MaterialApp( + home: Center( + child: Tooltip( + message: tooltipText, + waitDuration: Duration(seconds: 1), + triggerMode: TooltipTriggerMode.longPress, + child: SizedBox.square(dimension: 50), + ), + ), + ), + ); + + await tester.startGesture(tester.getCenter(find.byType(Tooltip))); + await tester.pumpWidget(const SizedBox()); + expect(tester.takeException(), isNull); + }); + + testWidgets('Tooltip does not crash when showing the tooltip but the OverlayPortal is unmounted, during dispose', (WidgetTester tester) async { + await tester.pumpWidget( + const MaterialApp( + home: SelectionArea( + child: Center( + child: Tooltip( + message: tooltipText, + waitDuration: Duration(seconds: 1), + triggerMode: TooltipTriggerMode.longPress, + child: SizedBox.square(dimension: 50), + ), + ), + ), + ), + ); + + final TooltipState tooltipState = tester.state(find.byType(Tooltip)); + await tester.startGesture(tester.getCenter(find.byType(Tooltip))); + tooltipState.ensureTooltipVisible(); + await tester.pumpWidget(const SizedBox()); + expect(tester.takeException(), isNull); + }); + + testWidgets('Tooltip is not selectable', (WidgetTester tester) async { + const String tooltipText = 'AAAAAAAAAAAAAAAAAAAAAAA'; + String? selectedText; + await tester.pumpWidget( + MaterialApp( + home: SelectionArea( + onSelectionChanged: (SelectedContent? content) { selectedText = content?.plainText; }, + child: const Center( + child: Column( + children: [ + Text('Select Me'), + Tooltip( + message: tooltipText, + waitDuration: Duration(seconds: 1), + triggerMode: TooltipTriggerMode.longPress, + child: SizedBox.square(dimension: 50), + ), + ], + ), + ), + ), + ), + ); + + final TooltipState tooltipState = tester.state(find.byType(Tooltip)); + + final Rect textRect = tester.getRect(find.text('Select Me')); + final TestGesture gesture = await tester.startGesture(Alignment.centerLeft.alongSize(textRect.size) + textRect.topLeft); + // Drag from centerLeft to centerRight to select the text. + await tester.pump(const Duration(seconds: 1)); + await gesture.moveTo(Alignment.centerRight.alongSize(textRect.size) + textRect.topLeft); + await tester.pump(); + + tooltipState.ensureTooltipVisible(); + await tester.pump(); + // Make sure the tooltip becomes visible. + expect(find.text(tooltipText), findsOneWidget); + assert(selectedText != null); + + final Rect tooltipTextRect = tester.getRect(find.text(tooltipText)); + // Now drag from centerLeft to centerRight to select the tooltip text. + await gesture.moveTo(Alignment.centerLeft.alongSize(tooltipTextRect.size) + tooltipTextRect.topLeft); + await tester.pump(); + await gesture.moveTo(Alignment.centerRight.alongSize(tooltipTextRect.size) + tooltipTextRect.topLeft); + await tester.pump(); + + expect(selectedText, isNot(contains('A'))); + }); } Future setWidgetForTooltipMode(