Allow the Slider to always show the value indicator. #162223

yiiim · 2025-01-26T09:21:10Z

Modified enum ShowValueIndicator
before:

enum ShowValueIndicator {
  onlyForDiscrete,
  onlyForContinuous,
  always,
  never,
}

after:

enum ShowValueIndicator {
  onlyForDiscrete,
  onlyForContinuous,
  @Deprecated(
    'Use ShowValueIndicator.onDrag. '
    'This feature was deprecated after v3.28.0-1.0.pre.',
  )
  always,
  onDrag,
  alwaysVisible,
  never,
}

To maintain previous behavior, ShowValueIndicator.onlyForDiscrete and ShowValueIndicator.onlyForContinuous are still retained.

The behavior of ShowValueIndicator.always remains consistent with the previous implementation, but it is marked as deprecated and should be replaced with ShowValueIndicator.onDrag. A new ShowValueIndicator.alwaysVisible has been added to show the value indicator even when not dragging.

Additionally, the value indicator is now rendered by OverlayPortal.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Piinks

Thanks for another PR @yiiim!
FYI @TahaTesser who is verry familiar with sliders. :)

packages/flutter/lib/src/material/range_slider.dart

packages/flutter/lib/src/material/slider_theme.dart

packages/flutter/test/material/range_slider_test.dart

packages/flutter/lib/src/material/range_slider.dart

TahaTesser

This PR does a fair bit of refactoring to add OverlayPortal, this makes sliders more complex.

We had previous show value indicator behaviors without OverlayPortal. Please provide more details on OverlayPortal addition and benefits.

packages/flutter/lib/src/material/range_slider.dart

yiiim · 2025-02-17T14:38:16Z

This PR does a fair bit of refactoring to add OverlayPortal, this makes sliders more complex.

We had previous show value indicator behaviors without OverlayPortal. Please provide more details on OverlayPortal addition and benefits.

Now, when ShowValueIndicator changes, it is necessary to update the child on the overlay simultaneously. OverlayPortal can easily achieve this.

And when we change ShowValueIndicator through setState, we cannot insert or remove OverlayEntry during the build phase.

TahaTesser

Great work! Just some initial thoughts.

TahaTesser · 2025-02-19T16:09:07Z

packages/flutter/lib/src/material/range_slider.dart

-
-  OverlayEntry? overlayEntry;
+  Widget _buildValueIndicator(ShowValueIndicator? showValueIndicator) {
+    late final Widget valueIndicator = CompositedTransformFollower(


Suggested change

late final Widget valueIndicator = CompositedTransformFollower(

late final Widget valueIndicator = CompositedTransformFollower(

Is late required since the value is assigned here.

If valueIndicator is never accessed, the assignment expression for valueIndicator will not be executed. It is not necessarily required to create it in the subsequent code.

the late isn't required to execute this code.

TahaTesser · 2025-02-19T16:09:36Z

packages/flutter/lib/src/material/range_slider.dart

+      link: _layerLink,
+      child: _ValueIndicatorRenderObjectWidget(state: this),
+    );
+    late final Widget valueIndicatorWhenDragged = ValueListenableBuilder<bool>(


similarly, late is used but value is also assigned here.

TahaTesser · 2025-02-19T16:15:21Z

packages/flutter/lib/src/material/range_slider.dart

+  void hideValueIndicator() {
+    assert(mounted);
+    _showValueIndicatorForDragged.value = false;


Just making sure. Once the portal controller is created and assigned to shown, are we disposing it when value indicator is hidden? Considering a screen might have a bunch sliders and each of one of them might create a portal controller without being disposed.

Do you mean OverlayPortalController? OverlayPortalController does not need to dispose.

The OverlayChild will always be displayed. When ShowValueIndicator is set to be shown during dragging, _showValueIndicatorForDragged controls whether _ValueIndicatorRenderObjectWidget is mounted to the tree within the OverlayChild.

TahaTesser · 2025-02-21T15:35:14Z

FYI @LongCatIsLooong on the overlay portal implementation. I was told you're working on something that would help Slider.

TahaTesser · 2025-02-25T14:45:04Z

FYI @LongCatIsLooong on the overlay portal implementation. I was told you're working on something that would help Slider.

Here is the PR #163575

LongCatIsLooong · 2025-02-25T19:02:34Z

packages/flutter/lib/src/material/range_slider.dart

+  late final OverlayPortalController _valueIndicatorOverlayPortalController =
+      OverlayPortalController()..show();
+  // Control whether the value indicator is visible only when it needs to be shown during dragging.
+  late final ValueNotifier<bool> _showValueIndicatorForDragged = ValueNotifier<bool>(false);


nit: I'm not familiar with the implementation, but adding the new state variable would probably make the implementation more complicated than checking if the animation status is .dismissed?

That's awesome! I completely overlooked that. It cleaned up a lot of code.

packages/flutter/lib/src/material/range_slider.dart

LongCatIsLooong

I think I looked into doing that before, the slider theming API could have been much cleaner if we could use inherited widgets, so thanks for converting the implementation to using OverlayPortal! The OverlayPortal change LGTM.

TahaTesser

LGTM with some nits

packages/flutter/lib/src/material/range_slider.dart

TahaTesser · 2025-03-05T14:24:58Z

packages/flutter/lib/src/material/slider_theme.dart

+  @Deprecated(
+    'Use ShowValueIndicator.onDrag. '
+    'This feature was deprecated after v3.28.0-1.0.pre.',
+  )


Do you plan to write a dart fix for this deprecation? It'd be best to deprecate this property in a separate PR with dart fix.

I will write this in a separate PR.

The dart fix should be included in this PR, not a separate change.

If the property is deprecated in this PR then dart fix should be included.

Perhaps, I wasn't clear. I suggested to mark it as deprecated and include dart fix in a separate PR so the dart fix can be included with the deprecation.

dart fix is included in this PR.

packages/flutter/test/material/range_slider_test.dart

TahaTesser · 2025-03-05T14:37:56Z

@yiiim Could you please also update #162223 (comment) with new API changes. isDragged is now onDrag for instance.

yiiim · 2025-03-06T07:34:38Z

@Piinks Can you help find out the reason why the Google test failed?

TahaTesser

I just realised I approved it without making sure deprecation mark was reverted after @Piinks comment.

Please include dart fix in this PR if you plan to keep deprecation mark.

Piinks

@yiiim it looks like the Google testing shows the on tap functionality is broken by this change. When the slider is tapped on, it shows the value indicator in response. Can you take a look?

Piinks · 2025-03-17T15:01:29Z

packages/flutter/lib/fix_data/fix_material/fix_slider_theme.yaml

+          constant: 'onDrag'
+          inEnum: 'ShowValueIndicator'
+
+# Before adding a new fix: read instructions at the top of this file.


Nit: missing a newline here.

Piinks · 2025-03-17T15:02:30Z

packages/flutter/lib/fix_data/fix_material/fix_slider_theme.yaml

@@ -0,0 +1,35 @@
+# Copyright 2014 The Flutter Authors. All rights reserved.


I am not sure we need a new file just for one fix, can we just add this to the fixes in fix_material.yaml?

Dart fixes also need to be tested. Can you please add tests under the test_fixes directory? They demonstrate the code before and after the tool applies the migration.

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jan 26, 2025

yiiim added the c: new feature Nothing broken; request for a new capability label Jan 27, 2025

Piinks self-requested a review January 29, 2025 19:10

Piinks reviewed Feb 14, 2025

View reviewed changes

TahaTesser self-requested a review February 17, 2025 09:23

TahaTesser reviewed Feb 17, 2025

View reviewed changes

packages/flutter/lib/src/material/range_slider.dart Outdated Show resolved Hide resolved

sider value indicator

da97b9b

yiiim force-pushed the sider_value_indicator branch from bb3d73d to da97b9b Compare February 17, 2025 15:14

TahaTesser self-requested a review February 19, 2025 16:05

TahaTesser reviewed Feb 19, 2025

View reviewed changes

++

549330c

TahaTesser requested a review from LongCatIsLooong February 25, 2025 14:46

LongCatIsLooong reviewed Feb 25, 2025

View reviewed changes

packages/flutter/lib/src/material/range_slider.dart Outdated Show resolved Hide resolved

LongCatIsLooong reviewed Feb 25, 2025

View reviewed changes

packages/flutter/lib/src/material/range_slider.dart Outdated Show resolved Hide resolved

LongCatIsLooong reviewed Feb 25, 2025

View reviewed changes

++

e049229

TahaTesser self-requested a review March 4, 2025 15:29

TahaTesser approved these changes Mar 5, 2025

View reviewed changes

yiiim added 2 commits March 6, 2025 14:29

++

9e64894

Merge branch 'master' into sider_value_indicator

8e39ad3

yiiim requested a review from Piinks March 6, 2025 07:34

Merge branch 'master' into sider_value_indicator

8f15216

TahaTesser self-requested a review March 11, 2025 18:03

TahaTesser requested changes Mar 11, 2025

View reviewed changes

dartfix

d6009de

Piinks reviewed Mar 17, 2025

View reviewed changes

github-actions bot added the c: tech-debt Technical debt, code quality, testing, etc. label Mar 18, 2025

dart fix test

0fd3f15

yiiim force-pushed the sider_value_indicator branch from b6b9699 to 0fd3f15 Compare March 19, 2025 01:57

Merge branch 'master' into sider_value_indicator

5affbdd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the Slider to always show the value indicator. #162223

Allow the Slider to always show the value indicator. #162223

yiiim commented Jan 26, 2025 •

edited

Loading

Piinks left a comment

TahaTesser left a comment •

edited

Loading

yiiim commented Feb 17, 2025 •

edited

Loading

TahaTesser left a comment

TahaTesser Feb 19, 2025

yiiim Feb 20, 2025

TahaTesser Mar 5, 2025

TahaTesser Feb 19, 2025

TahaTesser Feb 19, 2025

yiiim Feb 20, 2025

TahaTesser commented Feb 21, 2025 •

edited

Loading

TahaTesser commented Feb 25, 2025

LongCatIsLooong Feb 25, 2025

yiiim Feb 26, 2025

LongCatIsLooong left a comment

TahaTesser left a comment

TahaTesser Mar 5, 2025

yiiim Mar 6, 2025

Piinks Mar 11, 2025

TahaTesser Mar 11, 2025

yiiim Mar 13, 2025

TahaTesser commented Mar 5, 2025

yiiim commented Mar 6, 2025

TahaTesser left a comment

Piinks left a comment

Piinks Mar 17, 2025

Piinks Mar 17, 2025

Piinks Mar 17, 2025

	late final Widget valueIndicator = CompositedTransformFollower(
	late final Widget valueIndicator = CompositedTransformFollower(

		@@ -0,0 +1,35 @@
		# Copyright 2014 The Flutter Authors. All rights reserved.

Allow the Slider to always show the value indicator. #162223

Are you sure you want to change the base?

Allow the Slider to always show the value indicator. #162223

Conversation

yiiim commented Jan 26, 2025 • edited Loading

Pre-launch Checklist

Piinks left a comment

Choose a reason for hiding this comment

TahaTesser left a comment • edited Loading

Choose a reason for hiding this comment

yiiim commented Feb 17, 2025 • edited Loading

TahaTesser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TahaTesser commented Feb 21, 2025 • edited Loading

TahaTesser commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong left a comment

Choose a reason for hiding this comment

TahaTesser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TahaTesser commented Mar 5, 2025

yiiim commented Mar 6, 2025

TahaTesser left a comment

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiiim commented Jan 26, 2025 •

edited

Loading

TahaTesser left a comment •

edited

Loading

yiiim commented Feb 17, 2025 •

edited

Loading

TahaTesser commented Feb 21, 2025 •

edited

Loading