From 27f29e75ca4cd892e5ab7c7245b3b430dd238124 Mon Sep 17 00:00:00 2001 From: Kate Lovett Date: Wed, 23 Dec 2020 17:29:02 -0600 Subject: [PATCH] Fix scrollbar config for hover events (#72833) --- .../flutter/lib/src/material/scrollbar.dart | 14 +---- .../flutter/lib/src/widgets/scrollbar.dart | 60 ++++++++++--------- .../flutter/test/material/scrollbar_test.dart | 42 +++++++++++-- 3 files changed, 69 insertions(+), 47 deletions(-) diff --git a/packages/flutter/lib/src/material/scrollbar.dart b/packages/flutter/lib/src/material/scrollbar.dart index 70fb3d9292..b836a87217 100644 --- a/packages/flutter/lib/src/material/scrollbar.dart +++ b/packages/flutter/lib/src/material/scrollbar.dart @@ -98,9 +98,6 @@ class _ScrollbarState extends RawScrollbarState { late ColorScheme _colorScheme; // On Android, scrollbars should match native appearance. late bool _useAndroidScrollbar; - // Hover events should be ignored on mobile, the exit event cannot be - // triggered, but the enter event can on tap. - late bool _isMobile; Set get _states => { if (_dragIsActive) MaterialState.dragged, @@ -174,7 +171,7 @@ class _ScrollbarState extends RawScrollbarState { if (states.contains(MaterialState.hovered) && widget.showTrackOnHover) return widget.hoverThickness ?? _kScrollbarThicknessWithTrack; // The default scrollbar thickness is smaller on mobile. - return widget.thickness ?? (_kScrollbarThickness / (_isMobile ? 2 : 1)); + return widget.thickness ?? (_kScrollbarThickness / (_useAndroidScrollbar ? 2 : 1)); }); } @@ -196,18 +193,13 @@ class _ScrollbarState extends RawScrollbarState { switch (theme.platform) { case TargetPlatform.android: _useAndroidScrollbar = true; - _isMobile = true; break; case TargetPlatform.iOS: - _useAndroidScrollbar = false; - _isMobile = true; - break; case TargetPlatform.linux: case TargetPlatform.fuchsia: case TargetPlatform.macOS: case TargetPlatform.windows: _useAndroidScrollbar = false; - _isMobile = false; break; } super.didChangeDependencies(); @@ -242,8 +234,6 @@ class _ScrollbarState extends RawScrollbarState { @override void handleHover(PointerHoverEvent event) { - // Hover events should not be triggered on mobile. - assert(!_isMobile); super.handleHover(event); // Check if the position of the pointer falls over the painted scrollbar if (isPointerOverScrollbar(event.position)) { @@ -259,8 +249,6 @@ class _ScrollbarState extends RawScrollbarState { @override void handleHoverExit(PointerExitEvent event) { - // Hover events should not be triggered on mobile. - assert(!_isMobile); super.handleHoverExit(event); setState(() { _hoverIsActive = false; }); _hoverAnimationController.reverse(); diff --git a/packages/flutter/lib/src/widgets/scrollbar.dart b/packages/flutter/lib/src/widgets/scrollbar.dart index 98d9334375..5a712b6a4a 100644 --- a/packages/flutter/lib/src/widgets/scrollbar.dart +++ b/packages/flutter/lib/src/widgets/scrollbar.dart @@ -782,7 +782,6 @@ class RawScrollbarState extends State with TickerProv late Animation _fadeoutOpacityAnimation; final GlobalKey _scrollbarPainterKey = GlobalKey(); bool _hoverIsActive = false; - late bool _isMobile; /// Used to paint the scrollbar. @@ -813,18 +812,6 @@ class RawScrollbarState extends State with TickerProv @override void didChangeDependencies() { super.didChangeDependencies(); - switch (defaultTargetPlatform) { - case TargetPlatform.android: - case TargetPlatform.iOS: - _isMobile = true; - break; - case TargetPlatform.fuchsia: - case TargetPlatform.linux: - case TargetPlatform.macOS: - case TargetPlatform.windows: - _isMobile = false; - break; - } _maybeTriggerScrollbar(); } @@ -1165,27 +1152,42 @@ class RawScrollbarState extends State with TickerProv Widget build(BuildContext context) { updateScrollbarPainter(); - Widget child = CustomPaint( - key: _scrollbarPainterKey, - foregroundPainter: scrollbarPainter, - child: RepaintBoundary(child: widget.child), - ); - - if (!_isMobile) { - // Hover events not supported on mobile. - child = MouseRegion( - onExit: handleHoverExit, - onHover: handleHover, - child: child - ); - } - return NotificationListener( onNotification: _handleScrollNotification, child: RepaintBoundary( child: RawGestureDetector( gestures: _gestures, - child: child, + child: MouseRegion( + onExit: (PointerExitEvent event) { + switch(event.kind) { + case PointerDeviceKind.mouse: + handleHoverExit(event); + break; + case PointerDeviceKind.stylus: + case PointerDeviceKind.invertedStylus: + case PointerDeviceKind.unknown: + case PointerDeviceKind.touch: + break; + } + }, + onHover: (PointerHoverEvent event) { + switch(event.kind) { + case PointerDeviceKind.mouse: + handleHover(event); + break; + case PointerDeviceKind.stylus: + case PointerDeviceKind.invertedStylus: + case PointerDeviceKind.unknown: + case PointerDeviceKind.touch: + break; + } + }, + child: CustomPaint( + key: _scrollbarPainterKey, + foregroundPainter: scrollbarPainter, + child: RepaintBoundary(child: widget.child), + ) + ), ), ), ); diff --git a/packages/flutter/test/material/scrollbar_test.dart b/packages/flutter/test/material/scrollbar_test.dart index b388b1bf2b..d1590d53f7 100644 --- a/packages/flutter/test/material/scrollbar_test.dart +++ b/packages/flutter/test/material/scrollbar_test.dart @@ -842,7 +842,7 @@ void main() { }), ); - testWidgets('Hover animation is not triggered on mobile', (WidgetTester tester) async { + testWidgets('Hover animation is not triggered by tap gestures', (WidgetTester tester) async { final ScrollController scrollController = ScrollController(); await tester.pumpWidget( MaterialApp( @@ -864,7 +864,7 @@ void main() { find.byType(Scrollbar), paints..rrect( rrect: RRect.fromRectAndRadius( - const Rect.fromLTRB(794.0, 0.0, 798.0, 90.0), + const Rect.fromLTRB(790.0, 0.0, 798.0, 90.0), const Radius.circular(8.0), ), color: const Color(0x1a000000), @@ -873,18 +873,50 @@ void main() { await tester.tapAt(const Offset(794.0, 5.0)); await tester.pumpAndSettle(); - // Tapping on mobile triggers a hover enter event. In this case, the - // Scrollbar should be unchanged since it ignores hover events on mobile. + // Tapping triggers a hover enter event. In this case, the Scrollbar should + // be unchanged since it ignores hover events that aren't from a mouse. expect( find.byType(Scrollbar), paints..rrect( rrect: RRect.fromRectAndRadius( - const Rect.fromLTRB(794.0, 0.0, 798.0, 90.0), + const Rect.fromLTRB(790.0, 0.0, 798.0, 90.0), const Radius.circular(8.0), ), color: const Color(0x1a000000), ), ); + + // Now trigger hover with a mouse. + final TestGesture gesture = await tester.createGesture(kind: ui.PointerDeviceKind.mouse); + await gesture.addPointer(); + addTearDown(gesture.removePointer); + await gesture.moveTo(const Offset(794.0, 5.0)); + await tester.pump(); + + expect( + find.byType(Scrollbar), + paints + ..rect( + rect: const Rect.fromLTRB(784.0, 0.0, 800.0, 600.0), + color: const Color(0x08000000), + ) + ..line( + p1: const Offset(784.0, 0.0), + p2: const Offset(784.0, 600.0), + strokeWidth: 1.0, + color: const Color(0x1a000000), + ) + ..rrect( + rrect: RRect.fromRectAndRadius( + // Scrollbar thumb is larger + const Rect.fromLTRB(786.0, 0.0, 798.0, 90.0), + const Radius.circular(8.0), + ), + // Hover color + color: const Color(0x80000000), + ), + ); + }, variant: const TargetPlatformVariant({ TargetPlatform.iOS,