From bd06749edccc9c65151e4ce25c79e9db099fd37c Mon Sep 17 00:00:00 2001 From: Jose Alba Date: Wed, 27 May 2020 18:27:01 -0400 Subject: [PATCH] Slider value indicator gets disposed if it is activated (#57535) --- .../lib/src/material/range_slider.dart | 82 +++++++++------- packages/flutter/lib/src/material/slider.dart | 42 +++++--- .../lib/src/material/slider_theme.dart | 1 + .../test/material/range_slider_test.dart | 95 +++++++++++++++++++ .../flutter/test/material/slider_test.dart | 90 ++++++++++++++++++ 5 files changed, 263 insertions(+), 47 deletions(-) diff --git a/packages/flutter/lib/src/material/range_slider.dart b/packages/flutter/lib/src/material/range_slider.dart index 215984ccd28..c394cce2e7d 100644 --- a/packages/flutter/lib/src/material/range_slider.dart +++ b/packages/flutter/lib/src/material/range_slider.dart @@ -472,6 +472,10 @@ class _RangeSliderState extends State with TickerProviderStateMixin enableController.dispose(); startPositionController.dispose(); endPositionController.dispose(); + if (overlayEntry != null) { + overlayEntry.remove(); + overlayEntry = null; + } super.dispose(); } @@ -1154,6 +1158,10 @@ class _RenderRangeSlider extends RenderBox with RelayoutWhenSystemFontsChangeMix } void _handleDragUpdate(DragUpdateDetails details) { + if (!_state.mounted) { + return; + } + final double dragValue = _getValueFromGlobalPosition(details.globalPosition); // If no selection has been made yet, test for thumb selection again now @@ -1190,7 +1198,10 @@ class _RenderRangeSlider extends RenderBox with RelayoutWhenSystemFontsChangeMix } void _endInteraction() { - _state.overlayController.reverse(); + if (!_state.mounted) { + return; + } + if (showValueIndicator && _state.interactionTimer == null) { _state.valueIndicatorController.reverse(); } @@ -1202,6 +1213,7 @@ class _RenderRangeSlider extends RenderBox with RelayoutWhenSystemFontsChangeMix } _active = false; } + _state.overlayController.reverse(); } void _handleDragStart(DragStartDetails details) { @@ -1388,22 +1400,24 @@ class _RenderRangeSlider extends RenderBox with RelayoutWhenSystemFontsChangeMix if (shouldPaintValueIndicators) { _state.paintBottomValueIndicator = (PaintingContext context, Offset offset) { - _sliderTheme.rangeValueIndicatorShape.paint( - context, - bottomThumbCenter, - activationAnimation: _valueIndicatorAnimation, - enableAnimation: _enableAnimation, - isDiscrete: isDiscrete, - isOnTop: false, - labelPainter: bottomLabelPainter, - parentBox: this, - sliderTheme: _sliderTheme, - textDirection: _textDirection, - thumb: bottomThumb, - value: bottomValue, - textScaleFactor: textScaleFactor, - sizeWithOverflow: resolvedscreenSize, - ); + if (attached) { + _sliderTheme.rangeValueIndicatorShape.paint( + context, + bottomThumbCenter, + activationAnimation: _valueIndicatorAnimation, + enableAnimation: _enableAnimation, + isDiscrete: isDiscrete, + isOnTop: false, + labelPainter: bottomLabelPainter, + parentBox: this, + sliderTheme: _sliderTheme, + textDirection: _textDirection, + thumb: bottomThumb, + value: bottomValue, + textScaleFactor: textScaleFactor, + sizeWithOverflow: resolvedscreenSize, + ); + } }; } @@ -1462,22 +1476,24 @@ class _RenderRangeSlider extends RenderBox with RelayoutWhenSystemFontsChangeMix } _state.paintTopValueIndicator = (PaintingContext context, Offset offset) { - _sliderTheme.rangeValueIndicatorShape.paint( - context, - topThumbCenter, - activationAnimation: _valueIndicatorAnimation, - enableAnimation: _enableAnimation, - isDiscrete: isDiscrete, - isOnTop: thumbDelta < innerOverflow, - labelPainter: topLabelPainter, - parentBox: this, - sliderTheme: _sliderTheme, - textDirection: _textDirection, - thumb: topThumb, - value: topValue, - textScaleFactor: textScaleFactor, - sizeWithOverflow: resolvedscreenSize, - ); + if (attached) { + _sliderTheme.rangeValueIndicatorShape.paint( + context, + topThumbCenter, + activationAnimation: _valueIndicatorAnimation, + enableAnimation: _enableAnimation, + isDiscrete: isDiscrete, + isOnTop: thumbDelta < innerOverflow, + labelPainter: topLabelPainter, + parentBox: this, + sliderTheme: _sliderTheme, + textDirection: _textDirection, + thumb: topThumb, + value: topValue, + textScaleFactor: textScaleFactor, + sizeWithOverflow: resolvedscreenSize, + ); + } }; } diff --git a/packages/flutter/lib/src/material/slider.dart b/packages/flutter/lib/src/material/slider.dart index 234b949e492..4498934a564 100644 --- a/packages/flutter/lib/src/material/slider.dart +++ b/packages/flutter/lib/src/material/slider.dart @@ -512,6 +512,10 @@ class _SliderState extends State with TickerProviderStateMixin { valueIndicatorController.dispose(); enableController.dispose(); positionController.dispose(); + if (overlayEntry != null) { + overlayEntry.remove(); + overlayEntry = null; + } super.dispose(); } @@ -1238,6 +1242,10 @@ class _RenderSlider extends RenderBox with RelayoutWhenSystemFontsChangeMixin { } void _endInteraction() { + if (!_state.mounted) { + return; + } + if (_active && _state.mounted) { if (onChangeEnd != null) { onChangeEnd(_discretize(_currentDragValue)); @@ -1255,6 +1263,10 @@ class _RenderSlider extends RenderBox with RelayoutWhenSystemFontsChangeMixin { void _handleDragStart(DragStartDetails details) => _startInteraction(details.globalPosition); void _handleDragUpdate(DragUpdateDetails details) { + if (!_state.mounted) { + return; + } + if (isInteractive) { final double valueDelta = details.primaryDelta / _trackRect.width; switch (textDirection) { @@ -1396,20 +1408,22 @@ class _RenderSlider extends RenderBox with RelayoutWhenSystemFontsChangeMixin { if (isInteractive && label != null && !_valueIndicatorAnimation.isDismissed) { if (showValueIndicator) { _state.paintValueIndicator = (PaintingContext context, Offset offset) { - _sliderTheme.valueIndicatorShape.paint( - context, - offset + thumbCenter, - activationAnimation: _valueIndicatorAnimation, - enableAnimation: _enableAnimation, - isDiscrete: isDiscrete, - labelPainter: _labelPainter, - parentBox: this, - sliderTheme: _sliderTheme, - textDirection: _textDirection, - value: _value, - textScaleFactor: textScaleFactor, - sizeWithOverflow: screenSize.isEmpty ? size : screenSize, - ); + if (attached) { + _sliderTheme.valueIndicatorShape.paint( + context, + offset + thumbCenter, + activationAnimation: _valueIndicatorAnimation, + enableAnimation: _enableAnimation, + isDiscrete: isDiscrete, + labelPainter: _labelPainter, + parentBox: this, + sliderTheme: _sliderTheme, + textDirection: _textDirection, + value: _value, + textScaleFactor: textScaleFactor, + sizeWithOverflow: screenSize.isEmpty ? size : screenSize, + ); + } }; } } diff --git a/packages/flutter/lib/src/material/slider_theme.dart b/packages/flutter/lib/src/material/slider_theme.dart index 2b7fc8a5abf..3b0c18dae00 100644 --- a/packages/flutter/lib/src/material/slider_theme.dart +++ b/packages/flutter/lib/src/material/slider_theme.dart @@ -2791,6 +2791,7 @@ class _RectangularSliderValueIndicatorPathPainter { double scale, }) { assert(!sizeWithOverflow.isEmpty); + const double edgePadding = 8.0; final double rectangleWidth = _upperRectangleWidth(labelPainter, scale, textScaleFactor); /// Value indicator draws on the Overlay and by using the global Offset diff --git a/packages/flutter/test/material/range_slider_test.dart b/packages/flutter/test/material/range_slider_test.dart index 28380bc8c03..a92b64ec228 100644 --- a/packages/flutter/test/material/range_slider_test.dart +++ b/packages/flutter/test/material/range_slider_test.dart @@ -1338,6 +1338,101 @@ void main() { }); + testWidgets('Range Slider removes value indicator from overlay if Slider gets disposed without value indicator animation completing.', (WidgetTester tester) async { + final ThemeData theme = _buildTheme(); + final SliderThemeData sliderTheme = theme.sliderTheme; + RangeValues values = const RangeValues(0.5, 0.75); + + Widget buildApp({ + Color activeColor, + Color inactiveColor, + int divisions, + bool enabled = true, + }) { + final ValueChanged onChanged = (RangeValues newValues) { + values = newValues; + }; + return MaterialApp( + home: Directionality( + textDirection: TextDirection.ltr, + child: Material( + child: Navigator(onGenerateRoute: (RouteSettings settings) { + return MaterialPageRoute(builder: (BuildContext context) { + return Column( + children: [ + Theme( + data: theme, + child: RangeSlider( + values: values, + labels: RangeLabels(values.start.toStringAsFixed(2), + values.end.toStringAsFixed(2)), + divisions: divisions, + onChanged: onChanged, + ), + ), + RaisedButton( + child: const Text('Next'), + onPressed: () { + Navigator.of(context).pushReplacement( + MaterialPageRoute( + builder: (BuildContext context) { + return RaisedButton( + child: const Text('Inner page'), + onPressed: () => Navigator.of(context).pop(), + ); + }, + ), + ); + }, + ), + ], + ); + }); + }), + ), + ), + ); + } + + await tester.pumpWidget(buildApp(divisions: 3)); + + /// The value indicator is added to the overlay when it is clicked or dragged. + /// Because both of these gestures are occurring then it adds same value indicator + /// twice into the overlay. + final RenderBox valueIndicatorBox = tester.firstRenderObject(find.byType(Overlay)); + final Offset topRight = tester.getTopRight(find.byType(RangeSlider)).translate(-24, 0); + final TestGesture gesture = await tester.startGesture(topRight); + // Wait for value indicator animation to finish. + await tester.pumpAndSettle(); + + expect(find.byType(RangeSlider), isNotNull); + expect( + valueIndicatorBox, + paints + ..rrect(color: sliderTheme.inactiveTrackColor) + ..rect(color: sliderTheme.activeTrackColor) + ..rrect(color: sliderTheme.inactiveTrackColor), + ); + + await tester.tap(find.text('Next')); + await tester.pumpAndSettle(); + + expect(find.byType(RangeSlider), findsNothing); + expect( + valueIndicatorBox, + isNot( + paints + ..rrect(color: sliderTheme.inactiveTrackColor) + ..rect(color: sliderTheme.activeTrackColor) + ..rrect(color: sliderTheme.inactiveTrackColor) + ), + ); + + // Don't stop holding the value indicator. + await gesture.up(); + await tester.pumpAndSettle(); + }); + testWidgets('Range Slider top thumb gets stroked when overlapping', (WidgetTester tester) async { RangeValues values = const RangeValues(0.3, 0.7); diff --git a/packages/flutter/test/material/slider_test.dart b/packages/flutter/test/material/slider_test.dart index 699f5545da3..54453470615 100644 --- a/packages/flutter/test/material/slider_test.dart +++ b/packages/flutter/test/material/slider_test.dart @@ -1949,6 +1949,96 @@ void main() { await gesture.up(); }); + testWidgets('Slider removes value indicator from overlay if Slider gets disposed without value indicator animation completing.', (WidgetTester tester) async { + final Key sliderKey = UniqueKey(); + double value = 0.0; + + Widget buildApp({ + Color activeColor, + Color inactiveColor, + int divisions, + bool enabled = true, + }) { + return MaterialApp( + home: Directionality( + textDirection: TextDirection.ltr, + child: Material( + child: Navigator(onGenerateRoute: (RouteSettings settings) { + return MaterialPageRoute(builder: (BuildContext context) { + return Column( + children: [ + Slider( + key: sliderKey, + min: 0.0, + max: 100.0, + divisions: divisions, + label: '${value.round()}', + value: value, + onChanged: (double newValue) { + value = newValue; + }, + ), + RaisedButton( + child: const Text('Next'), + onPressed: () { + Navigator.of(context).pushReplacement( + MaterialPageRoute( + builder: (BuildContext context) { + return RaisedButton( + child: const Text('Inner page'), + onPressed: () => Navigator.of(context).pop(), + ); + }, + ), + ); + }, + ), + ], + ); + }); + }), + ), + ), + ); + } + + await tester.pumpWidget(buildApp(divisions: 3)); + + /// The value indicator is added to the overlay when it is clicked or dragged. + /// Because both of these gestures are occurring then it adds same value indicator + /// twice into the overlay. + final RenderBox valueIndicatorBox = tester.firstRenderObject(find.byType(Overlay)); + final Offset topRight = tester.getTopRight(find.byType(Slider)).translate(-24, 0); + final TestGesture gesture = await tester.startGesture(topRight); + // Wait for value indicator animation to finish. + await tester.pumpAndSettle(); + + expect(find.byType(Slider), isNotNull); + expect( + valueIndicatorBox, + paints + ..rrect(color: const Color(0xff2196f3)) // Active track. + ..rrect(color: const Color(0x3d2196f3)), // Inactive track. + ); + + await tester.tap(find.text('Next')); + await tester.pumpAndSettle(); + + expect(find.byType(Slider), findsNothing); + expect( + valueIndicatorBox, + isNot( + paints + ..rrect(color: const Color(0xff2196f3)) // Active track. + ..rrect(color: const Color(0x3d2196f3)) // Inactive track. + ), + ); + + // Don't stop holding the value indicator. + await gesture.up(); + await tester.pumpAndSettle(); + }); + testWidgets('Slider.adaptive', (WidgetTester tester) async { double value = 0.5;