Relax `OverlayPortal` asserts (#129053)
Fixes https://github.com/flutter/flutter/issues/129025
Also
- simplifies OverlayPortal code a bit and adds an assert.
- `Tooltip` shouldn't rebuild when hiding/showing the tooltip
diff --git a/packages/flutter/lib/src/material/tooltip.dart b/packages/flutter/lib/src/material/tooltip.dart
index ff73b26..045d97d 100644
--- a/packages/flutter/lib/src/material/tooltip.dart
+++ b/packages/flutter/lib/src/material/tooltip.dart
@@ -425,30 +425,27 @@
// _handleMouseExit. The set is cleared in _handleTapToDismiss, typically when
// a PointerDown event interacts with some other UI component.
final Set<int> _activeHoveringPointerDevices = <int>{};
+
+ static bool _isTooltipVisible(AnimationStatus status) {
+ return switch (status) {
+ AnimationStatus.completed || AnimationStatus.forward || AnimationStatus.reverse => true,
+ AnimationStatus.dismissed => false,
+ };
+ }
+
AnimationStatus _animationStatus = AnimationStatus.dismissed;
void _handleStatusChanged(AnimationStatus status) {
assert(mounted);
- final bool entryNeedsUpdating;
- switch (status) {
- case AnimationStatus.dismissed:
- entryNeedsUpdating = _animationStatus != AnimationStatus.dismissed;
- if (entryNeedsUpdating) {
- Tooltip._openedTooltips.remove(this);
- _overlayController.hide();
- }
- case AnimationStatus.completed:
- case AnimationStatus.forward:
- case AnimationStatus.reverse:
- entryNeedsUpdating = _animationStatus == AnimationStatus.dismissed;
- if (entryNeedsUpdating) {
- _overlayController.show();
- Tooltip._openedTooltips.add(this);
- SemanticsService.tooltip(_tooltipMessage);
- }
- }
-
- if (entryNeedsUpdating) {
- setState(() { /* Rebuild to update the OverlayEntry */ });
+ switch ((_isTooltipVisible(_animationStatus), _isTooltipVisible(status))) {
+ case (true, false):
+ Tooltip._openedTooltips.remove(this);
+ _overlayController.hide();
+ case (false, true):
+ _overlayController.show();
+ Tooltip._openedTooltips.add(this);
+ SemanticsService.tooltip(_tooltipMessage);
+ case (true, true) || (false, false):
+ break;
}
_animationStatus = status;
}
@@ -753,10 +750,7 @@
decoration: widget.decoration ?? tooltipTheme.decoration ?? defaultDecoration,
textStyle: widget.textStyle ?? tooltipTheme.textStyle ?? defaultTextStyle,
textAlign: widget.textAlign ?? tooltipTheme.textAlign ?? _defaultTextAlign,
- animation: CurvedAnimation(
- parent: _controller,
- curve: Curves.fastOutSlowIn,
- ),
+ animation: CurvedAnimation(parent: _controller, curve: Curves.fastOutSlowIn),
target: target,
verticalOffset: widget.verticalOffset ?? tooltipTheme.verticalOffset ?? _defaultVerticalOffset,
preferBelow: widget.preferBelow ?? tooltipTheme.preferBelow ?? _defaultPreferBelow,
diff --git a/packages/flutter/lib/src/painting/geometry.dart b/packages/flutter/lib/src/painting/geometry.dart
index a5f1487..e57d3cf 100644
--- a/packages/flutter/lib/src/painting/geometry.dart
+++ b/packages/flutter/lib/src/painting/geometry.dart
@@ -48,7 +48,7 @@
// VERTICAL DIRECTION
final bool fitsBelow = target.dy + verticalOffset + childSize.height <= size.height - margin;
final bool fitsAbove = target.dy - verticalOffset - childSize.height >= margin;
- final bool tooltipBelow = preferBelow ? fitsBelow || !fitsAbove : !(fitsAbove || !fitsBelow);
+ final bool tooltipBelow = fitsAbove == fitsBelow ? preferBelow : fitsBelow;
final double y;
if (tooltipBelow) {
y = math.min(target.dy + verticalOffset, size.height - margin);
@@ -56,19 +56,11 @@
y = math.max(target.dy - verticalOffset - childSize.height, margin);
}
// HORIZONTAL DIRECTION
- final double x;
- if (size.width - margin * 2.0 < childSize.width) {
- x = (size.width - childSize.width) / 2.0;
- } else {
- final double normalizedTargetX = clampDouble(target.dx, margin, size.width - margin);
- final double edge = margin + childSize.width / 2.0;
- if (normalizedTargetX < edge) {
- x = margin;
- } else if (normalizedTargetX > size.width - edge) {
- x = size.width - margin - childSize.width;
- } else {
- x = normalizedTargetX - childSize.width / 2.0;
- }
- }
+ final double flexibleSpace = size.width - childSize.width;
+ final double x = flexibleSpace <= 2 * margin
+ // If there's not enough horizontal space for margin + child, center the
+ // child.
+ ? flexibleSpace / 2.0
+ : clampDouble(target.dx - childSize.width / 2, margin, flexibleSpace - margin);
return Offset(x, y);
}
diff --git a/packages/flutter/lib/src/widgets/overlay.dart b/packages/flutter/lib/src/widgets/overlay.dart
index 8a432ad..c3e71df 100644
--- a/packages/flutter/lib/src/widgets/overlay.dart
+++ b/packages/flutter/lib/src/widgets/overlay.dart
@@ -1500,47 +1500,35 @@
// used as the slot of the overlay child widget.
//
// The developer must call `show` to reveal the overlay so we can get a unique
- // timestamp of the user interaction for sorting.
+ // timestamp of the user interaction for determining the z-index of the
+ // overlay child in the overlay.
//
// Avoid invalidating the cache if possible, since the framework uses `==` to
// compare slots, and _OverlayEntryLocation can't override that operator since
- // it's mutable.
+ // it's mutable. Changing slots can be relatively slow.
bool _childModelMayHaveChanged = true;
_OverlayEntryLocation? _locationCache;
+ static bool _isTheSameLocation(_OverlayEntryLocation locationCache, _RenderTheaterMarker marker) {
+ return locationCache._childModel == marker.overlayEntryWidgetState
+ && locationCache._theater == marker.theater;
+ }
+
_OverlayEntryLocation _getLocation(int zOrderIndex, bool targetRootOverlay) {
final _OverlayEntryLocation? cachedLocation = _locationCache;
- if (cachedLocation != null && !_childModelMayHaveChanged) {
+ late final _RenderTheaterMarker marker = _RenderTheaterMarker.of(context, targetRootOverlay: targetRootOverlay);
+ final bool isCacheValid = cachedLocation != null
+ && (!_childModelMayHaveChanged || _isTheSameLocation(cachedLocation, marker));
+ _childModelMayHaveChanged = false;
+ if (isCacheValid) {
assert(cachedLocation._zOrderIndex == zOrderIndex);
+ assert(cachedLocation._debugIsLocationValid());
return cachedLocation;
}
- _childModelMayHaveChanged = false;
- final _RenderTheaterMarker? marker = _RenderTheaterMarker.maybeOf(context, targetRootOverlay: targetRootOverlay);
- if (marker == null) {
- throw FlutterError.fromParts(<DiagnosticsNode>[
- ErrorSummary('No Overlay widget found.'),
- ErrorDescription(
- '${widget.runtimeType} widgets require an Overlay widget ancestor.\n'
- 'An overlay lets widgets float on top of other widget children.',
- ),
- ErrorHint(
- 'To introduce an Overlay widget, you can either directly '
- 'include one, or use a widget that contains an Overlay itself, '
- 'such as a Navigator, WidgetApp, MaterialApp, or CupertinoApp.',
- ),
- ...context.describeMissingAncestor(expectedAncestorType: Overlay),
- ]);
- }
- final _OverlayEntryLocation returnValue;
- if (cachedLocation == null) {
- returnValue = _OverlayEntryLocation(zOrderIndex, marker.overlayEntryWidgetState, marker.theater);
- } else if (cachedLocation._childModel != marker.overlayEntryWidgetState || cachedLocation._theater != marker.theater) {
- cachedLocation._dispose();
- returnValue = _OverlayEntryLocation(zOrderIndex, marker.overlayEntryWidgetState, marker.theater);
- } else {
- returnValue = cachedLocation;
- }
- assert(returnValue._zOrderIndex == zOrderIndex);
- return _locationCache = returnValue;
+ // Otherwise invalidate the cache and create a new location.
+ cachedLocation?._debugMarkLocationInvalid();
+ final _OverlayEntryLocation newLocation = _OverlayEntryLocation(zOrderIndex, marker.overlayEntryWidgetState, marker.theater);
+ assert(newLocation._zOrderIndex == zOrderIndex);
+ return _locationCache = newLocation;
}
@override
@@ -1582,7 +1570,7 @@
@override
void dispose() {
widget.controller._attachTarget = null;
- _locationCache?._dispose();
+ _locationCache?._debugMarkLocationInvalid();
_locationCache = null;
super.dispose();
}
@@ -1593,14 +1581,14 @@
'${widget.controller.runtimeType}.show() should not be called during build.'
);
setState(() { _zOrderIndex = zOrderIndex; });
- _locationCache?._dispose();
+ _locationCache?._debugMarkLocationInvalid();
_locationCache = null;
}
void hide() {
assert(SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks);
setState(() { _zOrderIndex = null; });
- _locationCache?._dispose();
+ _locationCache?._debugMarkLocationInvalid();
_locationCache = null;
}
@@ -1681,7 +1669,7 @@
}
void _addChild(_RenderDeferredLayoutBox child) {
- assert(_debugNotDisposed());
+ assert(_debugIsLocationValid());
_addToChildModel(child);
_theater._addDeferredChild(child);
assert(child.parent == _theater);
@@ -1696,7 +1684,7 @@
void _moveChild(_RenderDeferredLayoutBox child, _OverlayEntryLocation fromLocation) {
assert(fromLocation != this);
- assert(_debugNotDisposed());
+ assert(_debugIsLocationValid());
final _RenderTheater fromTheater = fromLocation._theater;
final _OverlayEntryWidgetState fromModel = fromLocation._childModel;
@@ -1712,34 +1700,54 @@
}
void _activate(_RenderDeferredLayoutBox child) {
- assert(_debugNotDisposed());
+ // This call is allowed even when this location is invalidated.
+ // See _OverlayPortalElement.activate.
assert(_overlayChildRenderBox == null, '$_overlayChildRenderBox');
_theater._adoptDeferredLayoutBoxChild(child);
_overlayChildRenderBox = child;
}
void _deactivate(_RenderDeferredLayoutBox child) {
- assert(_debugNotDisposed());
+ // This call is allowed even when this location is invalidated.
_theater._dropDeferredLayoutBoxChild(child);
_overlayChildRenderBox = null;
}
- bool _debugNotDisposed() {
- if (_debugDisposedStackTrace == null) {
+ // Throws a StateError if this location is already invalidated and shouldn't
+ // be used as an OverlayPortal slot. Must be used in asserts.
+ //
+ // Generally, `assert(_debugIsLocationValid())` should be used to prevent
+ // invalid accesses to an invalid `_OverlayEntryLocation` object. Exceptions
+ // to this rule are _removeChild, _deactive, which will be called when the
+ // OverlayPortal is being removed from the widget tree and may use the
+ // location information to perform cleanup tasks.
+ //
+ // Another exception is the _activate method which is called by
+ // _OverlayPortalElement.activate. See the comment in _OverlayPortalElement.activate.
+ bool _debugIsLocationValid() {
+ if (_debugMarkLocationInvalidStackTrace == null) {
return true;
}
- throw StateError('$this is already disposed. Stack trace: $_debugDisposedStackTrace');
+ throw StateError('$this is already disposed. Stack trace: $_debugMarkLocationInvalidStackTrace');
}
- StackTrace? _debugDisposedStackTrace;
+ // The StackTrace of the first _debugMarkLocationInvalid call. It's only for
+ // debugging purposes and the StackTrace will only be captured in debug builds.
+ //
+ // The effect of this method is not reversible. Once marked invalid, this
+ // object can't be marked as valid again.
+ StackTrace? _debugMarkLocationInvalidStackTrace;
@mustCallSuper
- void _dispose() {
- assert(_debugNotDisposed());
+ void _debugMarkLocationInvalid() {
+ assert(_debugIsLocationValid());
assert(() {
- _debugDisposedStackTrace = StackTrace.current;
+ _debugMarkLocationInvalidStackTrace = StackTrace.current;
return true;
}());
}
+
+ @override
+ String toString() => '${objectRuntimeType(this, '_OverlayEntryLocation')}[${shortHash(this)}] ${_debugMarkLocationInvalidStackTrace != null ? "(INVALID)":""}';
}
class _RenderTheaterMarker extends InheritedWidget {
@@ -1758,13 +1766,31 @@
|| oldWidget.overlayEntryWidgetState != overlayEntryWidgetState;
}
- static _RenderTheaterMarker? maybeOf(BuildContext context, { bool targetRootOverlay = false }) {
+ static _RenderTheaterMarker of(BuildContext context, { bool targetRootOverlay = false }) {
+ final _RenderTheaterMarker? marker;
if (targetRootOverlay) {
final InheritedElement? ancestor = _rootRenderTheaterMarkerOf(context.getElementForInheritedWidgetOfExactType<_RenderTheaterMarker>());
assert(ancestor == null || ancestor.widget is _RenderTheaterMarker);
- return ancestor != null ? context.dependOnInheritedElement(ancestor) as _RenderTheaterMarker? : null;
+ marker = ancestor != null ? context.dependOnInheritedElement(ancestor) as _RenderTheaterMarker? : null;
+ } else {
+ marker = context.dependOnInheritedWidgetOfExactType<_RenderTheaterMarker>();
}
- return context.dependOnInheritedWidgetOfExactType<_RenderTheaterMarker>();
+ if (marker != null) {
+ return marker;
+ }
+ throw FlutterError.fromParts(<DiagnosticsNode>[
+ ErrorSummary('No Overlay widget found.'),
+ ErrorDescription(
+ '${context.widget.runtimeType} widgets require an Overlay widget ancestor.\n'
+ 'An overlay lets widgets float on top of other widget children.',
+ ),
+ ErrorHint(
+ 'To introduce an Overlay widget, you can either directly '
+ 'include one, or use a widget that contains an Overlay itself, '
+ 'such as a Navigator, WidgetApp, MaterialApp, or CupertinoApp.',
+ ),
+ ...context.describeMissingAncestor(expectedAncestorType: Overlay),
+ ]);
}
static InheritedElement? _rootRenderTheaterMarkerOf(InheritedElement? theaterMarkerElement) {
@@ -1792,7 +1818,7 @@
required this.overlayChild,
required this.child,
}) : assert(overlayChild == null || overlayLocation != null),
- assert(overlayLocation == null || overlayLocation._debugNotDisposed());
+ assert(overlayLocation == null || overlayLocation._debugIsLocationValid());
final Widget? overlayChild;
@@ -1863,6 +1889,9 @@
if (box != null) {
assert(!box.attached);
assert(renderObject._deferredLayoutChild == box);
+ // updateChild has not been called at this point so the RenderTheater in
+ // the overlay location could be detached. Adding children to a detached
+ // RenderObject is still allowed however this isn't the most efficient.
(overlayChild.slot! as _OverlayEntryLocation)._activate(box);
}
}
@@ -1872,12 +1901,8 @@
void deactivate() {
final Element? overlayChild = _overlayChild;
// Instead of just detaching the render objects, removing them from the
- // render subtree entirely such that if the widget gets reparented to a
- // different overlay entry, the overlay child is inserted in the right
- // position in the overlay's child list.
- //
- // This is also a workaround for the !renderObject.attached assert in the
- // `RenderObjectElement.deactive()` method.
+ // render subtree entirely. This is a workaround for the
+ // !renderObject.attached assert in the `super.deactive()` method.
if (overlayChild != null) {
final _RenderDeferredLayoutBox? box = overlayChild.renderObject as _RenderDeferredLayoutBox?;
if (box != null) {
@@ -1902,7 +1927,7 @@
// reparenting between _overlayChild and _child, thus the non-null-typed slots.
@override
void moveRenderObjectChild(_RenderDeferredLayoutBox child, _OverlayEntryLocation oldSlot, _OverlayEntryLocation newSlot) {
- assert(newSlot._debugNotDisposed());
+ assert(newSlot._debugIsLocationValid());
newSlot._moveChild(child, oldSlot);
}
diff --git a/packages/flutter/test/material/tooltip_test.dart b/packages/flutter/test/material/tooltip_test.dart
index 08140ff..544f883 100644
--- a/packages/flutter/test/material/tooltip_test.dart
+++ b/packages/flutter/test/material/tooltip_test.dart
@@ -2245,6 +2245,39 @@
reason: 'Tooltip should NOT be visible when hovered and tapped, when trigger mode is tap',
);
});
+
+ testWidgetsWithLeakTracking('Tooltip does not rebuild for fade in / fade out animation', (WidgetTester tester) async {
+ await tester.pumpWidget(
+ const MaterialApp(
+ home: Center(
+ child: SizedBox.square(
+ dimension: 10.0,
+ child: Tooltip(
+ message: tooltipText,
+ waitDuration: Duration(seconds: 1),
+ triggerMode: TooltipTriggerMode.longPress,
+ child: SizedBox.expand(),
+ ),
+ ),
+ ),
+ ),
+ );
+ final TooltipState tooltipState = tester.state(find.byType(Tooltip));
+ final Element element = tooltipState.context as Element;
+ // The Tooltip widget itself is almost stateless thus doesn't need
+ // rebuilding.
+ expect(element.dirty, isFalse);
+
+ expect(tooltipState.ensureTooltipVisible(), isTrue);
+ expect(element.dirty, isFalse);
+ await tester.pump(const Duration(seconds: 1));
+ expect(element.dirty, isFalse);
+
+ expect(Tooltip.dismissAllToolTips(), isTrue);
+ expect(element.dirty, isFalse);
+ await tester.pump(const Duration(seconds: 1));
+ expect(element.dirty, isFalse);
+ });
}
Future<void> setWidgetForTooltipMode(
diff --git a/packages/flutter/test/widgets/overlay_portal_test.dart b/packages/flutter/test/widgets/overlay_portal_test.dart
index e280aeb..1d90ecf 100644
--- a/packages/flutter/test/widgets/overlay_portal_test.dart
+++ b/packages/flutter/test/widgets/overlay_portal_test.dart
@@ -164,6 +164,97 @@
await tester.pumpWidget(SizedBox(child: widget));
});
+ testWidgets('Safe to hide overlay child and remove OverlayPortal in the same frame', (WidgetTester tester) async {
+ // Regression test for https://github.com/flutter/flutter/issues/129025.
+ final Widget widget = Directionality(
+ key: GlobalKey(debugLabel: 'key'),
+ textDirection: TextDirection.ltr,
+ child: Overlay(
+ initialEntries: <OverlayEntry>[
+ OverlayEntry(
+ builder: (BuildContext context) {
+ return OverlayPortal(
+ controller: controller1,
+ overlayChildBuilder: (BuildContext context) => const SizedBox(),
+ child: const SizedBox(),
+ );
+ },
+ ),
+ ],
+ ),
+ );
+
+ controller1.show();
+ await tester.pumpWidget(widget);
+
+ controller1.hide();
+ await tester.pumpWidget(const SizedBox());
+ expect(tester.takeException(), isNull);
+ });
+
+ testWidgets('Safe to hide overlay child and reparent OverlayPortal in the same frame', (WidgetTester tester) async {
+ final OverlayPortal overlayPortal = OverlayPortal(
+ key: GlobalKey(debugLabel: 'key'),
+ controller: controller1,
+ overlayChildBuilder: (BuildContext context) => const SizedBox(),
+ child: const SizedBox(),
+ );
+
+ List<Widget> children = <Widget>[ const SizedBox(), overlayPortal ];
+
+ late StateSetter setState;
+ final Widget widget = Directionality(
+ textDirection: TextDirection.ltr,
+ child: Overlay(
+ initialEntries: <OverlayEntry>[
+ OverlayStatefulEntry(
+ builder: (BuildContext context, StateSetter setter) {
+ setState = setter;
+ return Column(children: children);
+ },
+ ),
+ ],
+ ),
+ );
+
+ controller1.show();
+ await tester.pumpWidget(widget);
+
+ controller1.hide();
+ setState(() {
+ children = <Widget>[ overlayPortal, const SizedBox() ];
+ });
+ await tester.pumpWidget(widget);
+ expect(tester.takeException(), isNull);
+ });
+
+ testWidgets('Safe to hide overlay child and reparent OverlayPortal in the same frame 2', (WidgetTester tester) async {
+ final Widget widget = Directionality(
+ key: GlobalKey(debugLabel: 'key'),
+ textDirection: TextDirection.ltr,
+ child: Overlay(
+ initialEntries: <OverlayEntry>[
+ OverlayEntry(
+ builder: (BuildContext context) {
+ return OverlayPortal(
+ controller: controller1,
+ overlayChildBuilder: (BuildContext context) => const SizedBox(),
+ child: const SizedBox(),
+ );
+ },
+ ),
+ ],
+ ),
+ );
+
+ controller1.show();
+ await tester.pumpWidget(widget);
+
+ controller1.hide();
+ await tester.pumpWidget(SizedBox(child: widget));
+ expect(tester.takeException(), isNull);
+ });
+
testWidgets('Throws when the same controller is attached to multiple OverlayPortal', (WidgetTester tester) async {
final OverlayPortalController controller = OverlayPortalController(debugLabel: 'local controller');
final Widget widget = Directionality(
@@ -1407,6 +1498,64 @@
expect(counter2.layoutCount, 3);
});
});
+
+ testWidgets('Safe to move the overlay child to a different Overlay and remove the old Overlay', (WidgetTester tester) async {
+ controller1.show();
+ final GlobalKey key = GlobalKey(debugLabel: 'key');
+ final GlobalKey oldOverlayKey = GlobalKey(debugLabel: 'old overlay');
+ final GlobalKey newOverlayKey = GlobalKey(debugLabel: 'new overlay');
+ final GlobalKey overlayChildKey = GlobalKey(debugLabel: 'overlay child key');
+ await tester.pumpWidget(
+ Directionality(
+ textDirection: TextDirection.ltr,
+ child: Overlay(
+ key: oldOverlayKey,
+ initialEntries: <OverlayEntry>[
+ OverlayEntry(
+ builder: (BuildContext context) {
+ return OverlayPortal(
+ key: key,
+ controller: controller1,
+ overlayChildBuilder: (BuildContext context) => SizedBox(key: overlayChildKey),
+ child: const SizedBox(),
+ );
+ },
+ ),
+ ],
+ ),
+ ),
+ );
+
+ expect(find.byKey(overlayChildKey), findsOneWidget);
+ expect(find.byKey(newOverlayKey), findsNothing);
+ expect(find.byKey(oldOverlayKey), findsOneWidget);
+
+ await tester.pumpWidget(
+ Directionality(
+ textDirection: TextDirection.ltr,
+ child: Overlay(
+ key: newOverlayKey,
+ initialEntries: <OverlayEntry>[
+ OverlayEntry(
+ builder: (BuildContext context) {
+ return OverlayPortal(
+ key: key,
+ controller: controller1,
+ overlayChildBuilder: (BuildContext context) => SizedBox(key: overlayChildKey),
+ child: const SizedBox(),
+ );
+ },
+ ),
+ ],
+ ),
+ ),
+ );
+
+ expect(tester.takeException(), isNull);
+ expect(find.byKey(overlayChildKey), findsOneWidget);
+ expect(find.byKey(newOverlayKey), findsOneWidget);
+ expect(find.byKey(oldOverlayKey), findsNothing);
+ });
});
group('Paint order', () {
