Repository navigation
[material_ui] Paint the active RoundedRectSliderTrackShape segment on top in RTL - #13225
Conversation
… top in RTL Both track segments extend past the thumb center by half of the track height. In right-to-left layouts the inactive segment was painted after the active one, so its rounded end covered the end of the active segment next to the thumb. Paint the inactive segment first in both text directions. Fixes flutter/flutter#194278
github-actions
Bot
added
p: material_ui
triage-design
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates RoundedRectSliderTrackShape to paint the active track segment on top of the inactive track segment in both left-to-right (LTR) and right-to-left (RTL) text directions, and adds a regression test to verify this behavior. Review feedback suggests refactoring the implementation to avoid defining local helper functions inside the paint method, which can cause performance-impacting closure allocations during animations, and proposes a loop-based alternative.
| switch (textDirection) { | ||
| case TextDirection.ltr: | ||
| paintRightTrack(); | ||
| paintLeftTrack(); | ||
| case TextDirection.rtl: | ||
| paintLeftTrack(); | ||
| paintRightTrack(); | ||
| } |
There was a problem hiding this comment.
Defining local helper functions (paintRightTrack and paintLeftTrack) inside the paint method introduces closure allocations on every frame during slider animations or interactions. This can lead to increased garbage collection (GC) activity and potential performance jank on low-end devices.
To avoid closure allocations and code duplication, we can use a loop over a compile-time constant list of booleans indicating whether to paint the left track first, based on the text direction:
final bool drawRightTrack = thumbCenter.dx < (trackRect.right - (sliderTheme.trackHeight! / 2));
final bool drawLeftTrack = thumbCenter.dx > (trackRect.left + (sliderTheme.trackHeight! / 2));
for (final bool paintLeft in isLTR ? const <bool>[false, true] : const <bool>[true, false]) {
if (paintLeft) {
if (drawLeftTrack) {
context.canvas.drawRRect(
RRect.fromLTRBR(
trackRect.left,
trackRect.top,
thumbCenter.dx + (sliderTheme.trackHeight! / 2),
trackRect.bottom,
Radius.circular(trackRect.height / 2),
),
leftTrackPaint,
);
}
} else {
if (drawRightTrack) {
context.canvas.drawRRect(
RRect.fromLTRBR(
thumbCenter.dx - (sliderTheme.trackHeight! / 2),
trackRect.top,
trackRect.right,
trackRect.bottom,
Radius.circular(trackRect.height / 2),
),
rightTrackPaint,
);
}
}
}This approach completely eliminates closure allocations during the paint phase while keeping the code clean and DRY.
There was a problem hiding this comment.
The bot has a fair point about avoiding local closure allocations inside paint, but please don't use its for-loop suggestion (which is awkward and also accidentally drops additionalActiveTrackHeight and activeTrackRadius).
A cleaner way to avoid the closures is to draw by role (inactive track first, then active track second) rather than by left/right side. Now that the inactive segment is always painted first and the active segment is always painted second in both text directions:
- The first drawRRect is always the inactive track, so it unconditionally uses trackRect.top, trackRect.bottom, trackRadius, and inactivePaint.
- The second drawRRect is always the active track, so it unconditionally uses the additionalActiveTrackHeight-expanded top/bottom, activeTrackRadius, and activePaint.
Only the horizontal left/right bounds (and whether each segment is wide enough to draw) depend on textDirection. Computing those from textDirection up front and drawing inactive then active directly avoids the closure allocations while also letting you remove leftTrackPaint, rightTrackPaint, isRTL, and all the isLTR/isRTL ternaries for top, bottom, and radius.
There was a problem hiding this comment.
Thanks, done in 4f7a038. The paint now goes by role: inactive first, then active. Only the horizontal bounds depend on textDirection. The closures, leftTrackPaint/rightTrackPaint, isRTL and the top/bottom/radius ternaries are gone. The existing RTL regression test passes unchanged.
Draw the inactive segment first and the active segment second, deriving only the horizontal bounds from the text direction. This avoids the local closures in paint and removes the left/right paints and the per-direction top, bottom and radius ternaries.
QuncCccccc
added
the
autosubmit
…6210) flutter/packages@63881f9...565c657 2026-11-08 [email protected] Roll Flutter from e36b370 to acade0f (1 revision) (flutter/packages#13590) 2026-11-07 [email protected] [material_ui] Fix DropdownMenu losing text cursor when re-enabled (flutter/packages#13179) 2026-11-07 [email protected] Roll Flutter from 09175cd to e36b370 (22 revisions) (flutter/packages#13586) 2026-11-07 [email protected] [material_ui] Add Material motion springs (flutter/packages#13338) 2026-11-06 [email protected] [material_ui] Paint the active RoundedRectSliderTrackShape segment on top in RTL (flutter/packages#13225) 2026-11-06 [email protected] [pigeon] Replace Experimental with Preview (flutter/packages#13579) 2026-11-06 [email protected] [material_ui] Fix minimum width for Material 3 primary tab indicator (flutter/packages#13322) 2026-11-06 [email protected] [material_ui] Dispose drag-end CurvedAnimation in modal bottom sheet (flutter/packages#13167) 2026-11-06 [email protected] [camera_web] Re: Support for camera stream on web (flutter/packages#7950) 2026-11-06 [email protected] Roll Flutter from 3623d2d to 09175cd (61 revisions) (flutter/packages#13578) If this roll has caused a breakage, revert this CL and set the roller to dry run mode using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
RoundedRectSliderTrackShapepaints its two segments in a fixed order: the right-hand segment first, then the left-hand one. Both segments extend past the thumb center by half of the track height, so the segment painted last has its rounded end drawn over the other one.In LTR the left-hand segment is the active one, so the active segment's rounded end is on top. In RTL the left-hand segment is the inactive one, so the inactive segment's rounded end was painted over the active segment, which left a pale sliver beside the thumb. This is only visible when the track is thicker than the thumb.
This PR paints the inactive segment first in both text directions. The LTR paint order is unchanged. The rounded
RangeSlidertrack already paints its active segment last in both directions.Before and after, LTR on top and RTL below, with
trackHeight: 24:Screenshots demonstration
⬅ RTL
⬅ RTL
I added an RTL case to the existing
RoundedRectSliderTrackShapetest group. It checks the exact track rects and their paint order, and it fails without the fix.Fixes flutter/flutter#194278
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2