Skip to content

Fix Slider thumb doesn't align with divisions, thumb padding, and rounded corners #149594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 3 commits into from
Jul 15, 2024
Merged

Fix Slider thumb doesn't align with divisions, thumb padding, and rounded corners #149594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c411101
Fix `Slider` thumb doesn't align with divisions, thumb padding, and r…
TahaTesser Jun 3, 2024
36e073c
Kick tests
TahaTesser Jul 8, 2024
31d14d3
Kick tests
TahaTesser Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions packages/flutter/lib/src/material/slider.dart
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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 @@ -3246,11 +3246,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 @@ -3282,18 +3282,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