Skip to content

Commit

Permalink
Fix Slider thumb doesn't align with divisions, thumb padding, and r…
Browse files Browse the repository at this point in the history 
…ounded corners (flutter#149594)

fixes [[Slider] Thumb's center doesn't align with division's center](flutter#62567)
fixes [Slider thumb doesn't respect round slider track shape](flutter#149591)
fixes [`RoundedRectSliderTrackShape` corners are not rendered correctly](flutter#149589)

(Verified these behaviors with Android components implementation)

### 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 StatefulWidget {
  const MyApp({super.key});

  @OverRide
  State<MyApp> createState() => _MyAppState();
}

class _MyAppState extends State<MyApp> {
  double _value = 5.0;

  @OverRide
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      theme: ThemeData(
        sliderTheme: const SliderThemeData(
          trackHeight: 32,
          thumbColor: Colors.green,
          activeTrackColor: Colors.deepPurple,
          inactiveTrackColor: Colors.amber,
        ),
      ),
      home: Scaffold(
        body: Slider(
          value: _value,
          // divisions: 10,
          // ignore: avoid_redundant_argument_values
          min: 0,
          max: 10,
          onChanged: (double value) {
            setState(() {
              _value = value;
            });
          },
        ),
      ),
    );
  }
}
```

</details>

### Description
This PR fixes several core `Sliders` issues which are apparent in flutter#147783. As a result, fixing the these bugs will unblock it.

### 1. Fixes the thumb doesn't align with `Slider` divisions.

![Group 8](https://github.com/flutter/flutter/assets/48603081/9aa138ae-9525-4af4-8fc7-3cea0692a6d7)

![Group 9](https://github.com/flutter/flutter/assets/48603081/e97940ae-a1c8-4b8b-9971-1cf417d32e40)

### 2.  Fixes `RoundedRectSliderTrackShape` corners are not rendered correctly.

![Group 10](https://github.com/flutter/flutter/assets/48603081/ed20a6bb-d5c9-486b-a020-2c9ca7de55da)

### 3.  Fixes round track shape corners when the thumb is at the start or end of the round track shape.

![Group 4](https://github.com/flutter/flutter/assets/48603081/37a2e820-402d-4964-a206-717ccf1c5c02)

![Group 3](https://github.com/flutter/flutter/assets/48603081/5d36d523-5fb7-466f-9d53-b6928963fcab)

![Group 7](https://github.com/flutter/flutter/assets/48603081/8f3b4c48-f04d-4681-a62f-a7ea5a3e19fa)
  • Loading branch information
@TahaTesser
TahaTesser authored Jul 15, 2024
1 parent 82b63ff commit 9e88446
Show file tree
Hide file tree
Showing 5 changed files with 275 additions and 116 deletions.
18 changes: 16 additions & 2 deletions packages/flutter/lib/src/material/slider.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1650,7 +1650,22 @@ class _RenderSlider extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
sliderTheme: _sliderTheme,
isDiscrete: isDiscrete,
);
final Offset thumbCenter = Offset(trackRect.left + visualPosition * trackRect.width, trackRect.center.dy);
final double padding = isDiscrete || _sliderTheme.trackShape!.isRounded ? trackRect.height : 0.0;
final double thumbPosition = isDiscrete
? trackRect.left + visualPosition * (trackRect.width - padding) + padding / 2
: trackRect.left + visualPosition * trackRect.width;
// Apply padding to trackRect.left and trackRect.right if the track height is
// greater than the thumb radius to ensure the thumb is drawn within the track.
final Size thumbSize = _sliderTheme.thumbShape!.getPreferredSize(isInteractive, isDiscrete);
final double thumbPadding = (padding > thumbSize.width / 2 ? padding / 2 : 0);
final Offset thumbCenter = Offset(
clampDouble(
thumbPosition,
trackRect.left + thumbPadding,
trackRect.right - thumbPadding,
),
trackRect.center.dy,
);
if (isInteractive) {
final Size overlaySize = sliderTheme.overlayShape!.getPreferredSize(isInteractive, false);
overlayRect = Rect.fromCircle(center: thumbCenter, radius: overlaySize.width / 2.0);
Expand Down Expand Up @@ -1692,7 +1707,6 @@ class _RenderSlider extends RenderBox with RelayoutWhenSystemFontsChangeMixin {
isEnabled: isInteractive,
sliderTheme: _sliderTheme,
).width;
final double padding = trackRect.height;
final double adjustedTrackWidth = trackRect.width - padding;
// If the tick marks would be too dense, don't bother painting them.
if (adjustedTrackWidth / divisions! >= 3.0 * tickMarkWidth) {
Expand Down
72 changes: 45 additions & 27 deletions packages/flutter/lib/src/material/slider_theme.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1117,6 +1117,11 @@ abstract class SliderTrackShape {
bool isDiscrete,
required TextDirection textDirection,
});

/// Whether the track shape is rounded.
///
/// This is used to determine the correct position of the thumb in relation to the track.
bool get isRounded => false;
}

/// Base class for [RangeSlider] thumb shapes.
Expand Down Expand Up @@ -1534,6 +1539,10 @@ mixin BaseSliderTrackShape {
// If the parentBox's size less than slider's size the trackRight will be less than trackLeft, so switch them.
return Rect.fromLTRB(math.min(trackLeft, trackRight), trackTop, math.max(trackLeft, trackRight), trackBottom);
}

/// Whether the track shape is rounded. This is used to determine the correct
/// position of the thumb in relation to the track. Defaults to false.
bool get isRounded => false;
}

/// A [Slider] track that's a simple rectangle.
Expand Down Expand Up @@ -1714,39 +1723,45 @@ class RoundedRectSliderTrackShape extends SliderTrackShape with BaseSliderTrackS
);
final Radius trackRadius = Radius.circular(trackRect.height / 2);
final Radius activeTrackRadius = Radius.circular((trackRect.height + additionalActiveTrackHeight) / 2);

context.canvas.drawRRect(
RRect.fromLTRBAndCorners(
trackRect.left,
(textDirection == TextDirection.ltr) ? trackRect.top - (additionalActiveTrackHeight / 2): trackRect.top,
thumbCenter.dx,
(textDirection == TextDirection.ltr) ? trackRect.bottom + (additionalActiveTrackHeight / 2) : trackRect.bottom,
topLeft: (textDirection == TextDirection.ltr) ? activeTrackRadius : trackRadius,
bottomLeft: (textDirection == TextDirection.ltr) ? activeTrackRadius: trackRadius,
),
leftTrackPaint,
);
context.canvas.drawRRect(
RRect.fromLTRBAndCorners(
thumbCenter.dx,
(textDirection == TextDirection.rtl) ? trackRect.top - (additionalActiveTrackHeight / 2) : trackRect.top,
trackRect.right,
(textDirection == TextDirection.rtl) ? trackRect.bottom + (additionalActiveTrackHeight / 2) : trackRect.bottom,
topRight: (textDirection == TextDirection.rtl) ? activeTrackRadius : trackRadius,
bottomRight: (textDirection == TextDirection.rtl) ? activeTrackRadius : trackRadius,
),
rightTrackPaint,
);
final bool isLTR = textDirection == TextDirection.ltr;
final bool isRTL = textDirection == TextDirection.rtl;

final bool drawInactiveTrack = thumbCenter.dx < (trackRect.right - (sliderTheme.trackHeight! / 2));
if (drawInactiveTrack) {
// Draw the inactive track segment.
context.canvas.drawRRect(
RRect.fromLTRBR(
thumbCenter.dx - (sliderTheme.trackHeight! / 2),
isRTL ? trackRect.top - (additionalActiveTrackHeight / 2) : trackRect.top,
trackRect.right,
isRTL ? trackRect.bottom + (additionalActiveTrackHeight / 2) : trackRect.bottom,
isLTR ? trackRadius : activeTrackRadius,
),
rightTrackPaint,
);
}
final bool drawActiveTrack = thumbCenter.dx > (trackRect.left + (sliderTheme.trackHeight! / 2));
if (drawActiveTrack) {
// Draw the active track segment.
context.canvas.drawRRect(
RRect.fromLTRBR(
trackRect.left,
isLTR ? trackRect.top - (additionalActiveTrackHeight / 2): trackRect.top,
thumbCenter.dx + (sliderTheme.trackHeight! / 2),
isLTR ? trackRect.bottom + (additionalActiveTrackHeight / 2) : trackRect.bottom,
isLTR ? activeTrackRadius : trackRadius,
),
leftTrackPaint,
);
}

final bool showSecondaryTrack = (secondaryOffset != null) &&
((textDirection == TextDirection.ltr)
? (secondaryOffset.dx > thumbCenter.dx)
: (secondaryOffset.dx < thumbCenter.dx));
(isLTR ? (secondaryOffset.dx > thumbCenter.dx) : (secondaryOffset.dx < thumbCenter.dx));

if (showSecondaryTrack) {
final ColorTween secondaryTrackColorTween = ColorTween(begin: sliderTheme.disabledSecondaryActiveTrackColor, end: sliderTheme.secondaryActiveTrackColor);
final Paint secondaryTrackPaint = Paint()..color = secondaryTrackColorTween.evaluate(enableAnimation)!;
if (textDirection == TextDirection.ltr) {
if (isLTR) {
context.canvas.drawRRect(
RRect.fromLTRBAndCorners(
thumbCenter.dx,
Expand All @@ -1773,6 +1788,9 @@ class RoundedRectSliderTrackShape extends SliderTrackShape with BaseSliderTrackS
}
}
}

@override
bool get isRounded => true;
}


Expand Down
4 changes: 2 additions & 2 deletions packages/flutter/test/material/inherited_theme_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ void main() {
await tester.tap(find.text('push wrapped'));
await tester.pumpAndSettle(); // route animation
RenderBox sliderBox = tester.firstRenderObject<RenderBox>(find.byType(Slider));
expect(sliderBox, paints..rrect(color: activeTrackColor)..rrect(color: inactiveTrackColor));
expect(sliderBox, paints..rrect(color: inactiveTrackColor)..rrect(color: activeTrackColor));
expect(sliderBox, paints..circle(color: thumbColor));

Navigator.of(navigatorContext).pop();
Expand All @@ -567,7 +567,7 @@ void main() {
await tester.tap(find.text('push unwrapped'));
await tester.pumpAndSettle(); // route animation
sliderBox = tester.firstRenderObject<RenderBox>(find.byType(Slider));
expect(sliderBox, isNot(paints..rrect(color: activeTrackColor)..rrect(color: inactiveTrackColor)));
expect(sliderBox, isNot(paints..rrect(color: inactiveTrackColor)..rrect(color: activeTrackColor)));
expect(sliderBox, isNot(paints..circle(color: thumbColor)));
});

Expand Down
56 changes: 32 additions & 24 deletions packages/flutter/test/material/slider_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ void main() {

expect(value, equals(0.20));
expect(log.length, 1);
expect(log[0], const Offset(212.0, 300.0));
expect(log[0], const Offset(213.0, 300.0));
});

testWidgets('Slider can move when tapped (LTR)', (WidgetTester tester) async {
Expand Down Expand Up @@ -417,8 +417,8 @@ void main() {
);

final List<Offset> expectedLog = <Offset>[
const Offset(24.0, 300.0),
const Offset(24.0, 300.0),
const Offset(26.0, 300.0),
const Offset(26.0, 300.0),
const Offset(400.0, 300.0),
];
final TestGesture gesture = await tester.startGesture(tester.getCenter(find.byKey(sliderKey)));
Expand All @@ -439,13 +439,13 @@ void main() {
await tester.pump(const Duration(milliseconds: 10));
expect(value, equals(0.0));
expect(log.length, 7);
expect(log.last.dx, moreOrLessEquals(344.5, epsilon: 0.1));
expect(log.last.dx, moreOrLessEquals(344.8, epsilon: 0.1));
// Final position.
await tester.pump(const Duration(milliseconds: 80));
expectedLog.add(const Offset(24.0, 300.0));
expectedLog.add(const Offset(26.0, 300.0));
expect(value, equals(0.0));
expect(log.length, 8);
expect(log.last.dx, moreOrLessEquals(24.0, epsilon: 0.1));
expect(log.last.dx, moreOrLessEquals(26.0, epsilon: 0.1));
await gesture.up();
});

Expand Down Expand Up @@ -490,7 +490,7 @@ void main() {
expect(updates, equals(1));
});

testWidgets('discrete Slider repaints when dragged', (WidgetTester tester) async {
testWidgets('Discrete Slider repaints when dragged', (WidgetTester tester) async {
final Key sliderKey = UniqueKey();
double value = 0.0;
final List<Offset> log = <Offset>[];
Expand Down Expand Up @@ -526,8 +526,8 @@ void main() {
);

final List<Offset> expectedLog = <Offset>[
const Offset(24.0, 300.0),
const Offset(24.0, 300.0),
const Offset(26.0, 300.0),
const Offset(26.0, 300.0),
const Offset(400.0, 300.0),
];
final TestGesture gesture = await tester.startGesture(tester.getCenter(find.byKey(sliderKey)));
Expand All @@ -548,13 +548,13 @@ void main() {
await tester.pump(const Duration(milliseconds: 10));
expect(value, equals(0.0));
expect(log.length, 7);
expect(log.last.dx, moreOrLessEquals(344.5, epsilon: 0.1));
expect(log.last.dx, moreOrLessEquals(344.8, epsilon: 0.1));
// Final position.
await tester.pump(const Duration(milliseconds: 80));
expectedLog.add(const Offset(24.0, 300.0));
expectedLog.add(const Offset(26.0, 300.0));
expect(value, equals(0.0));
expect(log.length, 8);
expect(log.last.dx, moreOrLessEquals(24.0, epsilon: 0.1));
expect(log.last.dx, moreOrLessEquals(26.0, epsilon: 0.1));
await gesture.up();
});

Expand Down Expand Up @@ -1175,7 +1175,7 @@ void main() {
..circle(x: 400.0, y: 24.0, radius: 1.0)
..circle(x: 587.0, y: 24.0, radius: 1.0)
..circle(x: 774.0, y: 24.0, radius: 1.0)
..circle(x: 24.0, y: 24.0, radius: 10.0),
..circle(x: 26.0, y: 24.0, radius: 10.0),
);

gesture = await tester.startGesture(center);
Expand All @@ -1186,13 +1186,13 @@ void main() {
expect(
material,
paints
..circle(x: 111.20703125, y: 24.0, radius: 5.687664985656738)
..circle(x: 112.7431640625, y: 24.0, radius: 5.687664985656738)
..circle(x: 26.0, y: 24.0, radius: 1.0)
..circle(x: 213.0, y: 24.0, radius: 1.0)
..circle(x: 400.0, y: 24.0, radius: 1.0)
..circle(x: 587.0, y: 24.0, radius: 1.0)
..circle(x: 774.0, y: 24.0, radius: 1.0)
..circle(x: 111.20703125, y: 24.0, radius: 10.0),
..circle(x: 112.7431640625, y: 24.0, radius: 10.0),
);

// Reparenting in the middle of an animation should do nothing.
Expand All @@ -1206,13 +1206,13 @@ void main() {
expect(
material,
paints
..circle(x: 190.0135726928711, y: 24.0, radius: 12.0)
..circle(x: 191.130521774292, y: 24.0, radius: 12.0)
..circle(x: 26.0, y: 24.0, radius: 1.0)
..circle(x: 213.0, y: 24.0, radius: 1.0)
..circle(x: 400.0, y: 24.0, radius: 1.0)
..circle(x: 587.0, y: 24.0, radius: 1.0)
..circle(x: 774.0, y: 24.0, radius: 1.0)
..circle(x: 190.0135726928711, y: 24.0, radius: 10.0),
..circle(x: 191.130521774292, y: 24.0, radius: 10.0),
);
// Wait for animations to finish.
await tester.pumpAndSettle();
Expand Down Expand Up @@ -3249,11 +3249,11 @@ void main() {
expect(
renderObject,
paints
// active track RRect
..rrect(rrect: RRect.fromLTRBAndCorners(-14.0, 2.0, 5.0, 8.0, topLeft: const Radius.circular(3.0), bottomLeft: const Radius.circular(3.0)))
// inactive track RRect
..rrect(rrect: RRect.fromLTRBAndCorners(5.0, 3.0, 24.0, 7.0, topRight: const Radius.circular(2.0), bottomRight: const Radius.circular(2.0)))
// thumb
// Inactive track RRect.
..rrect(rrect: RRect.fromLTRBR(3.0, 3.0, 24.0, 7.0, const Radius.circular(2.0)))
// Active track RRect.
..rrect(rrect: RRect.fromLTRBR(-14.0, 2.0, 7.0, 8.0, const Radius.circular(3.0)))
// Thumb.
..circle(x: 5.0, y: 5.0, radius: 10.0, ),
);
});
Expand Down Expand Up @@ -3285,18 +3285,26 @@ void main() {
await tester.pumpAndSettle(); // Finish the animation.

late RRect activeTrackRRect;
expect(renderObject, paints..something((Symbol method, List<dynamic> arguments) {
expect(renderObject, paints..rrect()..something((Symbol method, List<dynamic> arguments) {
if (method != #drawRRect) {
return false;
}
activeTrackRRect = arguments[0] as RRect;
return true;
}));

const double padding = 4.0;
// The thumb should at one-third(5 / 15) of the Slider.
// The right of the active track shape is the position of the thumb.
// 24.0 is the default margin, (800.0 - 24.0 - 24.0) is the slider's width.
expect(nearEqual(activeTrackRRect.right, (800.0 - 24.0 - 24.0) * (5 / 15) + 24.0, 0.01), true);
expect(
nearEqual(
activeTrackRRect.right,
(800.0 - 24.0 - 24.0 + (padding / 2)) * (5 / 15) + 24.0 + padding / 2,
0.01,
),
true,
);
});

testWidgets('Slider paints thumbColor', (WidgetTester tester) async {
Expand Down
Loading

0 comments on commit 9e88446

Please sign in to comment.