Skip to content

Fix Slider value indicator shape is broken if label is multiple-line text in Material 3 #173324

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.

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

huycozy
Copy link
Member

@huycozy huycozy commented Aug 6, 2025

Demo (after the fix):

Screenshots
  • year2023 true:
multiline multiline with maxLines limitation
  • year2023 false:
multiline multiline with maxLines limitation
video
multiline.mov
Sample code
import 'package:flutter/material.dart';

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

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

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: SliderExample(),
    );
  }
}

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

  @override
  State<SliderExample> createState() => _SliderExampleState();
}

class _SliderExampleState extends State<SliderExample> {
  double _currentDiscreteSliderValue = 2;
  double _currentDiscreteSliderValue2 = 2;
  double _currentDiscreteSliderValue3 = 2;
  double _currentDiscreteSliderValue4 = 2;
  double _currentDiscreteSliderValue5 = 2;
  double _currentDiscreteSliderValue6 = 2;

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      backgroundColor: Colors.grey,
      appBar: AppBar(title: const Text('Slider')),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          crossAxisAlignment: CrossAxisAlignment.start,
          children: <Widget>[
            Text('year2023: true or null'),
            Text('.   Oneline'),
            Slider(
              value: _currentDiscreteSliderValue2,
              max: 5,
              divisions: 4,
              label: _currentDiscreteSliderValue2.toString(),
              onChanged: (double value) {
                setState(() {
                  _currentDiscreteSliderValue2 = value;
                });
              },
            ),
            Text('.   Multiline'),
            Slider(
              value: _currentDiscreteSliderValue,
              max: 5,
              divisions: 4,
              label:
                  "${_currentDiscreteSliderValue.toString()}\nFirst Line\nSecond line\nThird line\nFourth line\nFifth line",
              onChanged: (double value) {
                setState(() {
                  _currentDiscreteSliderValue = value;
                });
              },
            ),
            Text('.   Multiline with maxLines'),
            Theme(
              data: ThemeData(
                sliderTheme: SliderThemeData(
                  valueIndicatorMultilineConfig: ValueIndicatorMultilineConfig(
                    maxLines: 3,
                    cornerPadding: 64,
                  ),
                ),
              ),
              child: Slider(
                value: _currentDiscreteSliderValue5,
                max: 5,
                divisions: 4,
                label:
                    "${_currentDiscreteSliderValue5.toString()}\nFirst Line\nSecond line\nThird line\nFourth line\nFifth line",
                onChanged: (double value) {
                  setState(() {
                    _currentDiscreteSliderValue5 = value;
                  });
                },
              ),
            ),
            Text('year2023: false'),
            Text('.   Oneline'),
            Slider(
              value: _currentDiscreteSliderValue4,
              max: 5,
              year2023: false,
              divisions: 4,
              label: _currentDiscreteSliderValue4.toString(),
              onChanged: (double value) {
                setState(() {
                  _currentDiscreteSliderValue4 = value;
                });
              },
            ),
            Text('.   Multiline'),
            Slider(
              value: _currentDiscreteSliderValue3,
              max: 5,
              year2023: false,
              divisions: 4,
              label:
                  "${_currentDiscreteSliderValue3.toString()}\nFirst Line\nSecond line\nThird line\nFourth line\nFifth line",
              onChanged: (double value) {
                setState(() {
                  _currentDiscreteSliderValue3 = value;
                });
              },
            ),
            Text('.   Multiline with maxLines'),
            Theme(
              data: ThemeData(
                sliderTheme: SliderThemeData(
                  valueIndicatorMultilineConfig: ValueIndicatorMultilineConfig(
                    maxLines: 3,
                    cornerPadding: 64,
                  ),
                ),
              ),
              child: Slider(
                value: _currentDiscreteSliderValue6,
                max: 5,
                year2023: false,
                divisions: 4,
                label:
                    "${_currentDiscreteSliderValue6.toString()}\nFirst Line\nSecond line\nThird line\nFourth line\nFifth line",
                onChanged: (double value) {
                  setState(() {
                    _currentDiscreteSliderValue6 = value;
                  });
                },
              ),
            ),
          ],
        ),
      ),
    );
  }
}

Description

In this PR:
- Address the issue reported where the value indicator shape does not fully surround the label when multiline text is set. The fix is applied for both cases when year2023 flag is true or false (corresponding to DropSliderValueIndicatorShape or RoundedRectSliderValueIndicatorShape).
- Add some tests.
- Enable support for an optional configuration of multiline label for the slider value indicator: introduce ValueIndicatorMultilineConfig in SliderThemeData with 3 properties: enabled, maxLines, and cornerPadding.

Pre-launch Checklist

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-assist bot 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.

@huycozy huycozy self-assigned this Aug 6, 2025
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Aug 6, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

An excellent pull request that addresses a visual bug with multiline labels on sliders and introduces a well-designed configuration API for it. The code is generally clean and the tests are quite thorough.

I've found a couple of issues that need attention: one critical bug that can lead to a crash, and another high-severity bug that causes incorrect painting.

Once these are addressed, this PR will be in great shape. Thank you for your contribution!

@huycozy huycozy force-pushed the fix-slider-multiple-lines-label branch from 4d44364 to 290af04 Compare August 6, 2025 06:16
@github-actions github-actions bot added the a: desktop Running on desktop label Aug 6, 2025
@huycozy huycozy force-pushed the fix-slider-multiple-lines-label branch from 290af04 to f196f9b Compare August 6, 2025 07:28
@github-actions github-actions bot removed the a: desktop Running on desktop label Aug 6, 2025
@huycozy huycozy force-pushed the fix-slider-multiple-lines-label branch 2 times, most recently from 3209ab5 to 3ed217e Compare August 6, 2025 11:06
@huycozy huycozy marked this pull request as ready for review August 6, 2025 11:41
@huycozy huycozy requested a review from dkwingsmt August 8, 2025 03:54
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of hardcore canvas work, thanks for taking this on @huycozy. I appreciate all of the nitty gritty details in the tests too. I think the approach looks good, but some hight level considerations here:

I see a lot of TextPainters that are created, laid out, and then quickly disposed. For performance reasons, I wonder if there are any opportunities to cache and reuse any of these TextPainters? They can cheaply paint multiple times if their parameters haven't changed and a relayout isn't needed. If this significantly increases code complexity though, then it's probably not worth it until we have a reason to think performance is a bottleneck.

Also another general comment, it looks like there is a lot of drawing code that is almost the same repeated in a couple of different places for various types/parts of slider. I wonder if there's any opportunity to reuse some generic code? This is not the fault of this PR, it was like that beforehand, but this is just a heads up in case you see any opportunities where it makes sense to DRY things up.

@huycozy
Copy link
Member Author

huycozy commented Aug 14, 2025

I see a lot of TextPainters that are created, laid out, and then quickly disposed. For performance reasons, I wonder if there are any opportunities to cache and reuse any of these TextPainters? They can cheaply paint multiple times if their parameters haven't changed and a relayout isn't needed. If this significantly increases code complexity though, then it's probably not worth it until we have a reason to think performance is a bottleneck.

Also another general comment, it looks like there is a lot of drawing code that is almost the same repeated in a couple of different places for various types/parts of slider. I wonder if there's any opportunity to reuse some generic code? This is not the fault of this PR, it was like that beforehand, but this is just a heads up in case you see any opportunities where it makes sense to DRY things up.

Thank you for reviewing this, Justin! I’ll look into these suggestions next week and consider any optimizations.

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #173324 at sha c55e20f

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Aug 14, 2025
@huycozy
Copy link
Member Author

huycozy commented Aug 21, 2025

Hi @justinmc, after looking at this fix again, I also think implementing cache for TextPainters may increase code complexity with minimal benefit and may result in higher maintenance costs. However, if you think this is necessary, please let me know.

Instead, I did improve the fix a bit with a cleanup, what I did was to do code deduplication text processing(maxLines truncation and ellipsis creation) inside ***PathPainter.paint() methods: Extract common multiline text processing inside paint methods of _DropSliderValueIndicatorPathPainter and _RoundedRectSliderValueIndicatorPathPainter into a reusable class/method.

  • Before:
    TextPainter finalLabelPainter = labelPainter;
    if (multilineConfig.enabled && multilineConfig.maxLines != null) {
      finalLabelPainter = TextPainter(
        text: labelPainter.text,
        textDirection: labelPainter.textDirection,
        textScaleFactor: textScaleFactor,
        maxLines: multilineConfig.maxLines,
      )..layout();

      // If lines were truncated, add ellipsis manually.
      if (finalLabelPainter.didExceedMaxLines) {
        finalLabelPainter.dispose(); // Dispose the intermediate painter
        finalLabelPainter = _createTextPainterWithEllipsis(
          originalPainter: labelPainter,
          originalText: labelText,
          maxLines: multilineConfig.maxLines!,
          textScaleFactor: textScaleFactor,
        );
      }
    }
...
if (finalLabelPainter != labelPainter) {
      finalLabelPainter.dispose();
    }
  • After: Create a reusable class _ValueIndicatorTextProcessor for using inside ***PathPainter.paint():
_ValueIndicatorTextProcessor
/// Utility class for processing multiline text in value indicators.
class _ValueIndicatorTextProcessor {
  const _ValueIndicatorTextProcessor._();

  /// Processes multiline text for value indicators, handling maxLines
  /// truncation and ellipsis.
  ///
  /// Returns the appropriate TextPainter based on the multiline configuration.
  /// The caller is responsible for disposing the returned TextPainter
  /// if it differs from the original.
  static TextPainter processMultilineText({
    required TextPainter originalPainter,
    required ValueIndicatorMultilineConfig multilineConfig,
    required double textScaleFactor,
  }) {
    final String labelText = originalPainter.text?.toPlainText() ?? '';

    if (!multilineConfig.enabled || multilineConfig.maxLines == null) {
      return originalPainter;
    }

    final TextPainter truncatedPainter = TextPainter(
      text: originalPainter.text,
      textDirection: originalPainter.textDirection,
      textScaleFactor: textScaleFactor,
      maxLines: multilineConfig.maxLines,
    )..layout();

    if (!truncatedPainter.didExceedMaxLines) {
      return truncatedPainter;
    }

    truncatedPainter.dispose();
    return _createTextPainterWithEllipsis(
      originalPainter: originalPainter,
      originalText: labelText,
      maxLines: multilineConfig.maxLines!,
      textScaleFactor: textScaleFactor,
    );
  }

  /// Safely disposes a TextPainter if it was created by processMultilineText.
  /// Does nothing if the painter is the same as the original.
  static void disposeIfCreated(TextPainter painter, TextPainter original) {
    if (painter != original) {
      painter.dispose();
    }
  }
}
    final TextPainter finalLabelPainter = _ValueIndicatorTextProcessor.processMultilineText(
      originalPainter: labelPainter,
      multilineConfig: multilineConfig,
      textScaleFactor: textScaleFactor,
    );
...
_ValueIndicatorTextProcessor.disposeIfCreated(finalLabelPainter, labelPainter);

Benefits we gained:

  • Reduces duplicated code in paint methods for those PathPainter classes
  • Centralized multiline text handling logic to improve maintainability and facilitate testing

Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
@huycozy huycozy force-pushed the fix-slider-multiple-lines-label branch from 3633bb7 to d859c61 Compare August 21, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slider shape is broken if label is multiple-line text in Material 3
2 participants