firka/flutter
firka/flutter
5
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Remove a bad assert from tooltip implementation (#127629)

Browse Source 
Fixes https://github.com/flutter/flutter/issues/127575

The `_handleMouseEnter` and `_handleMouseExit` calls are balanced, but the tooltip could be dismissed by an external `PointerDown` event interacting with a different UI component so the ` assert(_activeHoveringPointerDevices.isNotEmpty);` does not really make sense.
This commit is contained in:
LongCatIsLooong 2023-05-25 17:29:27 -07:00 committed by GitHub
parent 43de3365f5
commit 56808b486d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 194 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

74
packages/flutter/lib/src/material/tooltip.dart
View File

@ -425,6 +425,10 @@ class TooltipState extends State<Tooltip> with SingleTickerProviderStateMixin {
TapGestureRecognizer? _tapRecognizer;
// The ids of mouse devices that are keeping the tooltip from being dismissed.
//
// Device ids are added to this set in _handleMouseEnter, and removed in
// _handleMouseExit. The set is cleared in _handleTapToDismiss, typically when
// a PointerDown event interacts with some other UI component.
final Set<int> _activeHoveringPointerDevices = <int>{};
AnimationStatus _animationStatus = AnimationStatus.dismissed;
void _handleStatusChanged(AnimationStatus status) {
@ -469,18 +473,14 @@ class TooltipState extends State<Tooltip> with SingleTickerProviderStateMixin {
'timer must not be active when the tooltip is fading out',
);
switch (_controller.status) {
case AnimationStatus.dismissed:
if (withDelay.inMicroseconds > 0) {
_timer ??= Timer(withDelay, show);
} else {
show();
}
case AnimationStatus.dismissed when withDelay.inMicroseconds > 0:
_timer ??= Timer(withDelay, show);
// If the tooltip is already fading in or fully visible, skip the
// animation and show the tooltip immediately.
case AnimationStatus.dismissed:
case AnimationStatus.forward:
case AnimationStatus.reverse:
case AnimationStatus.completed:
// Fade in if needed and schedule to hide.
show();
}
}
@ -646,10 +646,9 @@ class TooltipState extends State<Tooltip> with SingleTickerProviderStateMixin {
}
void _handleMouseExit(PointerExitEvent event) {
if (!mounted) {
if (!mounted || _activeHoveringPointerDevices.isEmpty) {
return;
}
assert(_activeHoveringPointerDevices.isNotEmpty);
_activeHoveringPointerDevices.remove(event.device);
if (_activeHoveringPointerDevices.isEmpty) {
_scheduleDismissTooltip(withDelay: _hoverShowDuration);
@ -699,45 +698,36 @@ class TooltipState extends State<Tooltip> with SingleTickerProviderStateMixin {
// https://material.io/components/tooltips#specs
double _getDefaultTooltipHeight() {
final ThemeData theme = Theme.of(context);
switch (theme.platform) {
case TargetPlatform.macOS:
case TargetPlatform.linux:
case TargetPlatform.windows:
return 24.0;
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.iOS:
return 32.0;
}
return switch (Theme.of(context).platform) {
TargetPlatform.macOS ||
TargetPlatform.linux ||
TargetPlatform.windows => 24.0,
TargetPlatform.android ||
TargetPlatform.fuchsia ||
TargetPlatform.iOS => 32.0,
};
}
EdgeInsets _getDefaultPadding() {
final ThemeData theme = Theme.of(context);
switch (theme.platform) {
case TargetPlatform.macOS:
case TargetPlatform.linux:
case TargetPlatform.windows:
return const EdgeInsets.symmetric(horizontal: 8.0, vertical: 4.0);
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.iOS:
return const EdgeInsets.symmetric(horizontal: 16.0, vertical: 4.0);
}
return switch (Theme.of(context).platform) {
TargetPlatform.macOS ||
TargetPlatform.linux ||
TargetPlatform.windows => const EdgeInsets.symmetric(horizontal: 8.0, vertical: 4.0),
TargetPlatform.android ||
TargetPlatform.fuchsia ||
TargetPlatform.iOS => const EdgeInsets.symmetric(horizontal: 16.0, vertical: 4.0),
};
}
double _getDefaultFontSize() {
final ThemeData theme = Theme.of(context);
switch (theme.platform) {
case TargetPlatform.macOS:
case TargetPlatform.linux:
case TargetPlatform.windows:
return 12.0;
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.iOS:
return 14.0;
}
return switch (Theme.of(context).platform) {
TargetPlatform.macOS ||
TargetPlatform.linux ||
TargetPlatform.windows => 12.0,
TargetPlatform.android ||
TargetPlatform.fuchsia ||
TargetPlatform.iOS => 14.0,
};
}
void _createNewEntry() {

165
packages/flutter/test/material/tooltip_test.dart
View File

@ -1993,7 +1993,7 @@ void main() {
}
});
testWidgets('Tooltip trigger mode ignores mouse events', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Tooltip trigger mode ignores mouse events', (WidgetTester tester) async {
await tester.pumpWidget(
const MaterialApp(
home: Tooltip(
@ -2021,7 +2021,7 @@ void main() {
expect(find.text(tooltipText), findsOneWidget);
});
testWidgets('Tooltip does not block other mouse regions', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Tooltip does not block other mouse regions', (WidgetTester tester) async {
bool entered = false;
await tester.pumpWidget(
@ -2046,7 +2046,7 @@ void main() {
expect(entered, isTrue);
});
testWidgets('Does not rebuild on mouse connect/disconnect', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Does not rebuild on mouse connect/disconnect', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/117627
int buildCount = 0;
await tester.pumpWidget(
@ -2100,6 +2100,165 @@ void main() {
await _testGestureTap(tester, textSpan);
expect(isTapped, isTrue);
});
testWidgetsWithLeakTracking('Hold mouse button down and hover over the Tooltip widget', (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 TestGesture mouseGesture = await tester.startGesture(Offset.zero, kind: PointerDeviceKind.mouse);
addTearDown(mouseGesture.removePointer);
await mouseGesture.moveTo(tester.getCenter(find.byTooltip(tooltipText)));
await tester.pump(const Duration(seconds: 1));
expect(
find.text(tooltipText), findsOneWidget, reason: 'Tooltip should be visible when hovered.');
await mouseGesture.up();
await tester.pump(const Duration(days: 10));
await tester.pumpAndSettle();
expect(
find.text(tooltipText),
findsOneWidget,
reason: 'Tooltip should be visible even when there is a PointerUp when hovered.',
);
await mouseGesture.moveTo(Offset.zero);
await tester.pump(const Duration(seconds: 1));
await tester.pumpAndSettle();
expect(
find.text(tooltipText),
findsNothing,
reason: 'Tooltip should be dismissed with no hovering mouse cursor.' ,
);
});
testWidgetsWithLeakTracking('Hovered text should dismiss when clicked outside', (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(),
),
),
),
),
);
// Avoid using startGesture here to avoid the PointDown event from also being
// interpreted as a PointHover event by the Tooltip.
final TestGesture mouseGesture1 = await tester.createGesture(kind: PointerDeviceKind.mouse);
addTearDown(mouseGesture1.removePointer);
await mouseGesture1.moveTo(tester.getCenter(find.byTooltip(tooltipText)));
await tester.pump(const Duration(seconds: 1));
expect(find.text(tooltipText), findsOneWidget, reason: 'Tooltip should be visible when hovered.');
// Tapping on the Tooltip widget should dismiss the tooltip, since the
// trigger mode is longPress.
await tester.tap(find.byTooltip(tooltipText));
await tester.pump();
await tester.pumpAndSettle();
expect(find.text(tooltipText), findsNothing);
await mouseGesture1.removePointer();
final TestGesture mouseGesture2 = await tester.createGesture(kind: PointerDeviceKind.mouse);
addTearDown(mouseGesture2.removePointer);
await mouseGesture2.moveTo(tester.getCenter(find.byTooltip(tooltipText)));
await tester.pump(const Duration(seconds: 1));
expect(find.text(tooltipText), findsOneWidget, reason: 'Tooltip should be visible when hovered.');
await tester.tapAt(Offset.zero);
await tester.pump();
await tester.pumpAndSettle();
expect(find.text(tooltipText), findsNothing, reason: 'Tapping outside of the Tooltip widget should dismiss the tooltip.');
});
testWidgetsWithLeakTracking('Mouse tap and hover over the Tooltip widget', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/127575 .
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(),
),
),
),
),
);
// The PointDown event is also interpreted as a PointHover event by the
// Tooltip. This should be pretty rare but since it's more of a tap event
// than a hover event, the tooltip shouldn't show unless the triggerMode
// is set to tap.
final TestGesture mouseGesture1 = await tester.startGesture(
tester.getCenter(find.byTooltip(tooltipText)),
kind: PointerDeviceKind.mouse,
);
addTearDown(mouseGesture1.removePointer);
await tester.pump(const Duration(seconds: 1));
expect(
find.text(tooltipText),
findsNothing,
reason: 'Tooltip should NOT be visible when hovered and tapped, when trigger mode is not tap',
);
await mouseGesture1.up();
await mouseGesture1.removePointer();
await tester.pump(const Duration(days: 10));
await tester.pumpAndSettle();
await tester.pumpWidget(
const MaterialApp(
home: Center(
child: SizedBox.square(
dimension: 10.0,
child: Tooltip(
message: tooltipText,
waitDuration: Duration(seconds: 1),
triggerMode: TooltipTriggerMode.tap,
child: SizedBox.expand(),
),
),
),
),
);
final TestGesture mouseGesture2 = await tester.startGesture(
tester.getCenter(find.byTooltip(tooltipText)),
kind: PointerDeviceKind.mouse,
);
addTearDown(mouseGesture2.removePointer);
// The tap should be ignored, since Tooltip does not track "trigger gestures"
// for mouse devices.
await tester.pump(const Duration(milliseconds: 100));
await mouseGesture2.up();
await tester.pump(const Duration(seconds: 1));
expect(
find.text(tooltipText),
findsNothing,
reason: 'Tooltip should NOT be visible when hovered and tapped, when trigger mode is tap',
);
});
}
Future<void> setWidgetForTooltipMode(