Unverified Commit 4f959b97 authored by Taha Tesser's avatar Taha Tesser Committed by GitHub

Fix `Slider` `onChanged` callback order & never calls `onChangeStart` on ...

Fix `Slider` `onChanged` callback order & never calls `onChangeStart` on  `SliderInteraction.slideOnly` allowed interaction (#136720)

fixes [Slider will call onChanged before onChangeStart when sliding.](https://github.com/flutter/flutter/issues/136707)

This fixes a regression from https://github.com/flutter/flutter/pull/121483

### Code sample

<details>
<summary>expand to view the code sample</summary> 

```dart
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Example(),
    );
  }
}

class Example extends StatefulWidget {
  const Example({super.key});

  @override
  State<Example> createState() => _ExampleState();
}

class _ExampleState extends State<Example> {
  double _sliderValue = 0.5;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Slider(
          // allowedInteraction: SliderInteraction.tapAndSlide,
          // allowedInteraction: SliderInteraction.tapOnly,
          // allowedInteraction: SliderInteraction.slideOnly
          // allowedInteraction: SliderInteraction.slideThumb,
          value: _sliderValue,
          onChangeStart: (newValue) {
            print("onChangeStart ......");
          },
          onChanged: (newValue) {
            print("onChanged ......");
            setState(() {
              _sliderValue = newValue;
            });
          },
          onChangeEnd: (newValue) {
            print("onChangeEnd ......");
          },
        ),
      ),
    );
  }
}
```

</details>
parent d195d90d
......@@ -1494,14 +1494,13 @@ class _RenderSlider extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
case SliderInteraction.tapOnly:
_active = true;
_currentDragValue = _getValueFromGlobalPosition(globalPosition);
onChanged!(_discretize(_currentDragValue));
case SliderInteraction.slideThumb:
if (_isPointerOnOverlay(globalPosition)) {
_active = true;
_currentDragValue = value;
}
case SliderInteraction.slideOnly:
break;
onChangeStart?.call(_discretize(value));
}
if (_active) {
......@@ -1509,6 +1508,7 @@ class _RenderSlider extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
// a tap, it consists of a call to onChangeStart with the previous value and
// a call to onChangeEnd with the new value.
onChangeStart?.call(_discretize(value));
onChanged!(_discretize(_currentDragValue));
_state.overlayController.forward();
if (showValueIndicator) {
_state.valueIndicatorController.forward();
......
......@@ -3886,6 +3886,7 @@ void main() {
// (slider's left padding (overlayRadius), windowHeight / 2)
const Offset startOfTheSliderTrack = Offset(24, 300);
const Offset centerOfTheSlideTrack = Offset(400, 300);
final List<String> logs = <String>[];
Widget buildWidget() => MaterialApp(
home: Material(
......@@ -3895,11 +3896,18 @@ void main() {
value: value,
key: sliderKey,
allowedInteraction: SliderInteraction.tapOnly,
onChangeStart: (double newValue) {
logs.add('onChangeStart');
},
onChanged: (double newValue) {
logs.add('onChanged');
setState(() {
value = newValue;
});
},
onChangeEnd: (double newValue) {
logs.add('onChangeEnd');
},
);
}),
),
......@@ -3909,26 +3917,35 @@ void main() {
// allow tap only
await tester.pumpWidget(buildWidget());
expect(logs, isEmpty);
// test tap
final TestGesture gesture = await tester.startGesture(centerOfTheSlideTrack);
await tester.pump();
// changes from 1.0 -> 0.5
expect(value, 0.5);
expect(logs, <String>['onChangeStart', 'onChanged']);
// test slide
await gesture.moveTo(startOfTheSliderTrack);
await tester.pump();
// has no effect, remains 0.5
expect(value, 0.5);
expect(logs, <String>['onChangeStart', 'onChanged']);
await gesture.up();
await tester.pump();
expect(logs, <String>['onChangeStart', 'onChanged', 'onChangeEnd']);
});
testWidgetsWithLeakTracking('SliderInteraction.tapAndSlide', (WidgetTester tester) async {
testWidgetsWithLeakTracking('SliderInteraction.tapAndSlide (default)', (WidgetTester tester) async {
double value = 1.0;
final Key sliderKey = UniqueKey();
// (slider's left padding (overlayRadius), windowHeight / 2)
const Offset startOfTheSliderTrack = Offset(24, 300);
const Offset centerOfTheSlideTrack = Offset(400, 300);
const Offset endOfTheSliderTrack = Offset(800 - 24, 300);
final List<String> logs = <String>[];
Widget buildWidget() => MaterialApp(
home: Material(
......@@ -3937,12 +3954,18 @@ void main() {
return Slider(
value: value,
key: sliderKey,
// allowedInteraction: SliderInteraction.tapAndSlide, // default
onChangeStart: (double newValue) {
logs.add('onChangeStart');
},
onChanged: (double newValue) {
logs.add('onChanged');
setState(() {
value = newValue;
});
},
onChangeEnd: (double newValue) {
logs.add('onChangeEnd');
},
);
}),
),
......@@ -3951,11 +3974,14 @@ void main() {
await tester.pumpWidget(buildWidget());
expect(logs, isEmpty);
// Test tap.
final TestGesture gesture = await tester.startGesture(centerOfTheSlideTrack);
await tester.pump();
// changes from 1.0 -> 0.5
expect(value, 0.5);
expect(logs, <String>['onChangeStart', 'onChanged']);
// test slide
await gesture.moveTo(startOfTheSliderTrack);
......@@ -3966,6 +3992,12 @@ void main() {
await tester.pump();
// changes from 0.0 -> 1.0
expect(value, 1.0);
expect(logs, <String>['onChangeStart', 'onChanged', 'onChanged', 'onChanged']);
await gesture.up();
await tester.pump();
expect(logs, <String>['onChangeStart', 'onChanged', 'onChanged', 'onChanged', 'onChangeEnd']);
});
testWidgetsWithLeakTracking('SliderInteraction.slideOnly', (WidgetTester tester) async {
......@@ -3975,6 +4007,7 @@ void main() {
const Offset startOfTheSliderTrack = Offset(24, 300);
const Offset centerOfTheSlideTrack = Offset(400, 300);
const Offset endOfTheSliderTrack = Offset(800 - 24, 300);
final List<String> logs = <String>[];
Widget buildApp() {
return MaterialApp(
......@@ -3985,11 +4018,18 @@ void main() {
value: value,
key: sliderKey,
allowedInteraction: SliderInteraction.slideOnly,
onChangeStart: (double newValue) {
logs.add('onChangeStart');
},
onChanged: (double newValue) {
logs.add('onChanged');
setState(() {
value = newValue;
});
},
onChangeEnd: (double newValue) {
logs.add('onChangeEnd');
},
);
}),
),
......@@ -3999,11 +4039,14 @@ void main() {
await tester.pumpWidget(buildApp());
expect(logs, isEmpty);
// test tap
final TestGesture gesture = await tester.startGesture(centerOfTheSlideTrack);
await tester.pump();
// has no effect as tap is disabled, remains 1.0
expect(value, 1.0);
expect(logs, <String>['onChangeStart']);
// test slide
await gesture.moveTo(startOfTheSliderTrack);
......@@ -4014,6 +4057,12 @@ void main() {
await tester.pump();
// changes from 0.0 -> 1.0
expect(value, 1.0);
expect(logs, <String>['onChangeStart', 'onChanged', 'onChanged']);
await gesture.up();
await tester.pump();
expect(logs, <String>['onChangeStart', 'onChanged', 'onChanged', 'onChangeEnd']);
});
testWidgetsWithLeakTracking('SliderInteraction.slideThumb', (WidgetTester tester) async {
......@@ -4023,6 +4072,7 @@ void main() {
const Offset startOfTheSliderTrack = Offset(24, 300);
const Offset centerOfTheSliderTrack = Offset(400, 300);
const Offset endOfTheSliderTrack = Offset(800 - 24, 300);
final List<String> logs = <String>[];
Widget buildApp() {
return MaterialApp(
......@@ -4033,11 +4083,18 @@ void main() {
value: value,
key: sliderKey,
allowedInteraction: SliderInteraction.slideThumb,
onChangeStart: (double newValue) {
logs.add('onChangeStart');
},
onChanged: (double newValue) {
logs.add('onChanged');
setState(() {
value = newValue;
});
},
onChangeEnd: (double newValue) {
logs.add('onChangeEnd');
},
);
}),
),
......@@ -4047,17 +4104,21 @@ void main() {
await tester.pumpWidget(buildApp());
expect(logs, isEmpty);
// test tap
final TestGesture gesture = await tester.startGesture(centerOfTheSliderTrack);
await tester.pump();
// has no effect, remains 1.0
expect(value, 1.0);
expect(logs, isEmpty);
// test slide
await gesture.moveTo(startOfTheSliderTrack);
await tester.pump();
// has no effect, remains 1.0
expect(value, 1.0);
expect(logs, isEmpty);
// test slide thumb
await gesture.up();
......@@ -4065,22 +4126,36 @@ void main() {
await tester.pump();
// has no effect, remains 1.0
expect(value, 1.0);
expect(logs, <String>['onChangeStart']);
await gesture.moveTo(centerOfTheSliderTrack);
await tester.pump();
// changes from 1.0 -> 0.5
expect(value, 0.5);
expect(logs, <String>['onChangeStart', 'onChanged']);
// test tap inside overlay but not on thumb, then slide
await gesture.up();
// default overlay radius is 12, so 10 is inside the overlay
await gesture.down(centerOfTheSliderTrack.translate(-10, 0));
await tester.pump();
// has no effect, remains 1.0
// changes from 1.0 -> 0.5
expect(value, 0.5);
expect(logs, <String>['onChangeStart', 'onChanged', 'onChangeEnd', 'onChangeStart']);
await gesture.moveTo(endOfTheSliderTrack.translate(-10, 0));
await tester.pump();
// changes from 0.5 -> 1.0
expect(value, 1.0);
expect(logs, <String>['onChangeStart', 'onChanged', 'onChangeEnd', 'onChangeStart', 'onChanged']);
await gesture.up();
await tester.pump();
expect(
logs,
<String>['onChangeStart', 'onChanged', 'onChangeEnd', 'onChangeStart', 'onChanged', 'onChangeEnd'],
);
});
});
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment