Fix tooltip crash when route has secondary animation #172678

victorsanni · 2025-07-24T00:06:35Z

Makes the tooltip interactive if the route is the current route and the route is not animating out.

Added _route to cache ModalRoute.of(context).

Consolidates Fix the issue with Tooltip and Delay showing tooltip during page transition

Fix [Desktop] [Web] [Regression] [3.32] AppBar back - RenderBox was not laid out - TooltipState._buildTooltipOverlay

justinmc

LGTM with nits 👍 .

justinmc · 2025-07-24T18:31:27Z

packages/flutter/lib/src/material/tooltip.dart

@@ -607,7 +608,7 @@ class TooltipState extends State<Tooltip> with SingleTickerProviderStateMixin {

  void _handlePointerDown(PointerDownEvent event) {
    assert(mounted);
-    if (!(ModalRoute.isCurrentOf(context) ?? true)) {
+    if (!(_route?.isCurrent ?? true) || (_route?.secondaryAnimation?.isAnimating ?? false)) {


The secondaryAnimation is animating, meaning that the route is on its way out, so the tooltip shouldn't be interactive? Maybe consider leaving a comment explaining this if you think it's worth it. I guess you'd have to do that in all 3 places here.

I'm curious to understand this too.

I think pulling this logic out as a small private helper method would be helpful in any case; and that'd make a good home for the comment.

(In particular, I think even just writing down a name for that helper function will be helpful for clarifying what this logic is meant to do.)

justinmc · 2025-07-24T18:36:44Z

packages/flutter/test/material/tooltip_test.dart

+      final TestGesture gesture = await tester.createGesture(kind: PointerDeviceKind.mouse);
+      await gesture.addPointer();
+      await tester.tap(find.byType(BackButton));
+      await tester.pump(const Duration(milliseconds: 250));


This is supposed to be a fraction of the page transition right? Use PageTransitionObserver.transitionDuration. That way if the transition changes to be shorter than 250ms, this test will still work.

It's unlikely to be a problem but I was just dealing with this so I'm eager to use PageTransitionObserver :)

justinmc · 2025-07-24T18:39:12Z

packages/flutter/test/material/tooltip_test.dart

+      await gesture.moveTo(tester.getCenter(find.text('World')));
+      await tester.pumpAndSettle();
+
+      expect(tester.takeException(), isNull);


Is it worth it to also test that the tooltip is not visible? Up to you, it's possible that would unnecessarily make the test more brittle.

justinmc · 2025-07-24T18:41:33Z

packages/flutter/test/material/tooltip_test.dart

+      await tester.tap(find.text('Go to Second Page'));
+      await tester.pumpAndSettle();
+      await tester.tap(find.text('Go to Third Page'));
+      await tester.pumpAndSettle();


Usually pumpAndSettle is not enough to pump through a page transition, though it appears to be working here. Maybe add expects for text that's present on each page so you verify which page(s) are visible at any time.

gnprice

Thanks for fixing this!

packages/flutter/lib/src/material/tooltip.dart

gnprice · 2025-07-24T19:44:42Z

packages/flutter/lib/src/material/tooltip.dart

+    // If the route's secondary animation is animating, the route is on its way
+    // out. So, the tooltip should be non-interactive.
+    final bool routeIsAnimating = _route?.secondaryAnimation?.isAnimating ?? false;


Very helpful, thanks!

gnprice · 2025-07-24T19:46:13Z

packages/flutter/lib/src/material/tooltip.dart

+  bool _isTooltipInteractive() {
+    final bool routeIsCurrent = _route?.isCurrent ?? false;
+    // If the route's secondary animation is animating, the route is on its way
+    // out. So, the tooltip should be non-interactive.
+    final bool routeIsAnimating = _route?.secondaryAnimation?.isAnimating ?? false;
+    return routeIsCurrent && !routeIsAnimating;


If _route is null, then both branches of this fall back, but they end up doing so in opposite directions.

I think it'll probably be easiest to think through the logic if there's an if (_route == null) condition at the top that returns the desired result for that case. Then the rest can say _route! and not think about the null case.

In particular it looks like this PR currently flips the answer that's used when the route is null. I'm not sure what the right answer for that is — but I'm also not sure if if that change was intended, because this logic is a bit hard to read for that case 🙂

(With your update, I believe it no longer flips the answer for when the route is null — instead that continues to be treated as meaning the tooltip should be interactive, as if all this logic weren't here. Which I think is the right answer.)

gnprice

Thanks! LGTM.

LongCatIsLooong · 2025-07-24T20:37:55Z

packages/flutter/lib/src/material/tooltip.dart

@@ -605,9 +606,19 @@ class TooltipState extends State<Tooltip> with SingleTickerProviderStateMixin {
    }
  }

+  bool _isTooltipInteractive() {


Is this the right fix? Tooltip ideally should not know about route. This feels too specific and the problem may come up again in a different set up.

Good question! I agree Tooltip ideally shouldn't know about Route. There's probably a cleaner solution by using some other abstraction.

The same comment applies already to the previous fix #168546, which this PR fixes a bug in.

This is the core issue: #167359 (comment). Basically the tooltip shouldn't be displayed mid-transition.

Tooltip uses OverlayPortal behind the scenes:

flutter/packages/flutter/lib/src/material/tooltip.dart

Line 950 in 9f90740

return OverlayPortal(

So maybe the deeper solution is to have OverlayPortal not show its overlaychild in the middle of a route transition? But is that always desired behavior? Or can we hide it behind a flag?

I used the workaround in this PR previously in another PR for the CupertinoContextMenu overlay entry.

The root issue is that RenderObject.getTransformTo should not be called during build, it's unsafe even if in a LayoutBuilder.

#170186 feels okish to me because the framework doesn't have a lifecycle callback to tell an Element that they're going offstage. So you kinda have to make Route special there to get the signal that the text field is going offstage (hopefully we can introduce new APIs to avoid making this assumption, I think we get bitten by that a lot).

Fix tooltip crash

0bf182d

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 24, 2025

victorsanni added 3 commits July 23, 2025 17:07

Merge branch 'master' into modal-route-fix

f0406cc

Add test

5d820e3

Merge branch 'master' into modal-route-fix

326faec

victorsanni marked this pull request as ready for review July 24, 2025 18:19

victorsanni requested review from yiiim, justinmc and gnprice July 24, 2025 18:20

victorsanni changed the title ~~Fix tooltip crash~~ Fix tooltip crash when route has secondary animation Jul 24, 2025

More tweaks

464f0ad

justinmc approved these changes Jul 24, 2025

View reviewed changes

justinmc reviewed Jul 24, 2025

View reviewed changes

gnprice reviewed Jul 24, 2025

View reviewed changes

packages/flutter/lib/src/material/tooltip.dart Show resolved Hide resolved

Apply review suggestions

2288b42

victorsanni requested a review from gnprice July 24, 2025 19:47

gnprice reviewed Jul 24, 2025

View reviewed changes

Null check at the top of helper method

8c8b508

victorsanni requested a review from gnprice July 24, 2025 19:58

gnprice approved these changes Jul 24, 2025

View reviewed changes

LongCatIsLooong reviewed Jul 24, 2025

View reviewed changes

Fix tooltip crash when route has secondary animation #172678

Are you sure you want to change the base?

Fix tooltip crash when route has secondary animation #172678

Conversation

victorsanni commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

victorsanni commented Jul 24, 2025 •

edited

Loading