fix(Slider): value may exceed max when step is large #13632

lllomh · 2025-09-17T03:20:41Z

The issue #13625 has been confirmed to exist, here is a demo that reproduces the issue:

It has been fixed. Here is the demo after the fix:

Fixed an issue where the Slider component's `step` value might exceed the `max` value when the step value is large #13625.

Issue Description

When the Slider component's step value is relatively large, the format function might return a value outside the max range.

Steps to Reproduce:

Configure the Slider: min=20, max=100, step=30
Drag the slider to the far right
The component returns value=110, which exceeds the set max=100

Cause Analysis

Original format function logic:

const format = (value: number) => {
const min = +props.min;
const max = +props.max;
const step = +props.step;

value = clamp(value, min, max);
const diff = Math.round((value - min) / step) * step;
return addNumber(min, diff); // This may exceed the `max` value
};

Problem: Although the input value is clamped to the [min, max] range, the result calculated based on the step size may exceed the max value.

Take min=20, max=100, step=30 as an example:

When the user drags to a position close to 100 (for example, 95)
Calculation: 20 + Math.round((95-20)/30) * 30 = 20 + 90 = 110
The result, 110, exceeds max=100

Solution

Modify the format function to use a smart distance comparison strategy:

Before:

const format = (value: number) => {
const min = +props.min;
const max = +props.max;
const step = +props.step;

value = clamp(value, min, max);
const diff = Math.round((value - min) / step) * step;
return addNumber(min, diff);
};

After modification:

const format = (value: number) => {
const min = +props.min;
const max = +props.max;
const step = +props.step;

value = clamp(value, min, max);

// Calculate the value in steps
const diff = Math.round((value - min) / step) * step;
const steppedValue = addNumber(min, diff);

// If the stepped value exceeds the maximum, check whether the maximum value should be returned
if (steppedValue > max) {
// Calculate the distance to the maximum value and the distance to the previous stepped value
const prevSteppedValue = addNumber(min, diff - step);
const distanceToMax = Math.abs(value - max);
const distanceToPrev = Math.abs(value - prevSteppedValue);

// If closer to the maximum value, return the maximum value
return distanceToMax <= distanceToPrev ? max : prevSteppedValue;
}

return steppedValue;
};

Core Changes

Retain original logic: First limit the input value range, then calculate the step value
Add bounds checking: Check whether the calculated result exceeds max
Smart selection strategy:

When the step value exceeds max, compare the distance between the user's actual location and max and the distance to the previous valid step value
Select the value with the closer distance as the final result
Ensure that the user can select the value up to max

Test Verification

Take min=20, max=100, step=30 as an example:

Input Value	Original Result	New Result	Description
95	110 ❌	100 ✅	Closer to max, returns max
85	110 ❌	80 ✅	Closer to previous step value, returns 80
75	80 ✅	80 ✅	Normal step size, no change

Backward Compatibility

✅ No impact on existing normal use cases
✅ Optimizes behavior only in edge cases
✅ Keeps the API unchanged

User Experience Improvements

Meets expectations: When users set max=100, they should expect to see 100 selected.
Improved usability: Even with larger step sizes, the edge values can still be selected.
Consistency: Consistent with the behavior of major UI libraries.

This PR fix addresses edge issues with larger step sizes, making component behavior more consistent with user expectations while maintaining backward compatibility.

…t with the Chinese version under the English version

…pported on pc

…y large, the component may return a value outside the max range.

codecov-commenter · 2025-09-17T03:23:52Z

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.74%. Comparing base (ec5b45b) to head (9dcc149).
⚠️ Report is 154 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/vant/src/slider/Slider.tsx	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13632      +/-   ##
==========================================
+ Coverage   89.60%   89.74%   +0.14%     
==========================================
  Files         257      257              
  Lines        7013     7052      +39     
  Branches     1736     1746      +10     
==========================================
+ Hits         6284     6329      +45     
+ Misses        384      381       -3     
+ Partials      345      342       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…y large, the component may return a value outside the max range test.

chenjiahan

Thanks for your contribution! The code changes look good to me. However, the test code seems to have too many modifications and includes some unnecessary console.log statements. Could you simplify the test code a bit? (And make sure to use English for comments.)

lllomh · 2025-09-21T09:44:09Z

Thanks for your contribution! The code changes look good to me. However, the test code seems to have too many modifications and includes some unnecessary console.log statements. Could you simplify the test code a bit? (And make sure to use English for comments.)

Yes， Because the test coverage never reached 100%, the test was modified many times. I will make the necessary changes immediately. @chenjiahan

chenjiahan · 2025-09-21T09:50:16Z

If you’d like to help improve test coverage, feel free to submit a separate PR adding test cases — this will make the code review process easier.

lllomh · 2025-09-21T09:52:18Z

If you’d like to help improve test coverage, feel free to submit a separate PR adding test cases — this will make the code review process easier.

Okay, this test is already quite comprehensive. I will submit the simplified test code as you suggested shortly, and also remove the unnecessary console.log statements. It should take about 5 minutes.
@chenjiahan

…y large, the component may return a value outside the max range test.

lllomh · 2025-09-21T10:01:24Z

Thanks for your contribution! The code changes look good to me. However, the test code seems to have too many modifications and includes some unnecessary console.log statements. Could you simplify the test code a bit? (And make sure to use English for comments.)

Yes， Because the test coverage never reached 100%, the test was modified many times. I will make the necessary changes immediately. @chenjiahan

I have modified the test code to ensure there are no redundant console.logs. Please check it. Thank you. The test code is appended after the original test code to ensure that it does not affect the previous code.

@chenjiahan

inottn · 2025-09-22T02:37:58Z

packages/vant/src/slider/Slider.tsx

+      if (steppedValue > max) {
+        const prevSteppedValue = addNumber(min, diff - step);
+        const distanceToMax = Math.abs(value - max);
+        const distanceToPrev = Math.abs(value - prevSteppedValue);
+
+        return distanceToMax <= distanceToPrev ? max : prevSteppedValue;
+      }


Under what circumstances would distanceToMax be greater than distanceToPrev?

distanceToMax > distanceToPrev only occurs if prevSteppedValue is closer to value than max.

Specific conditions:
value must be between prevSteppedValue and max, and closer to prevSteppedValue.

Almost impossible

Could you provide specific examples? Because it seems like the added unit tests don't cover it.

I think the added unit test cases are a bit excessive. In my opinion, it’s sufficient to add test cases for just two scenarios: distanceToMax <= distanceToPrev and distanceToMax > distanceToPrev.

Could you provide specific examples? Because it seems like the added unit tests don't cover it.

@inottn Yes, in the description above I explained the problem of test coverage. Could you read it? #13632 (comment)

I understand. What I mean is that the current PR adds 20 test cases, which seems excessive for a single fix. Refer to #13425 and #13008, for minor fixes, adding one or two test cases is usually sufficient. The changes involved in this fix aren't related to whether the Slider is in range or vertical mode. I think adding just 2 test cases would suffice—one to test the scenario where distanceToMax <= distanceToPrev, and another for distanceToMax > distanceToPrev.

We only need to ensure minimal test cases cover the logic of this fix, rather than increasing the test coverage of the entire component (which should be done in a separate PR).

I understand. What I mean is that the current PR adds 20 test cases, which seems excessive for a single fix. Refer to #13425 and #13008, for minor fixes, adding one or two test cases is usually sufficient. The changes involved in this fix aren't related to whether the Slider is in range or vertical mode. I think adding just 2 test cases would suffice—one to test the scenario where distanceToMax <= distanceToPrev, and another for distanceToMax > distanceToPrev.

We only need to ensure minimal test cases cover the logic of this fix, rather than increasing the test coverage of the entire component (which should be done in a separate PR).

@inottn OK, I will modify it as you said. It will take 5 to 10 minutes.

I understand. What I mean is that the current PR adds 20 test cases, which seems excessive for a single fix. Refer to #13425 and #13008, for minor fixes, adding one or two test cases is usually sufficient. The changes involved in this fix aren't related to whether the Slider is in range or vertical mode. I think adding just 2 test cases would suffice—one to test the scenario where distanceToMax <= distanceToPrev, and another for distanceToMax > distanceToPrev.

We only need to ensure minimal test cases cover the logic of this fix, rather than increasing the test coverage of the entire component (which should be done in a separate PR).

@inottn Hi I have modified it. Many test cases I wrote earlier were to improve patch coverage. Now they have been deleted and only 2 are kept, but the patch coverage has been reduced. If you think this is appropriate!

@chenjiahan @inottn Hi, do I need to change something about this?

inottn · 2025-09-22T02:39:43Z

packages/vant/src/submit-bar/test/index.spec.ts

+afterEach(() => {
+  vi.clearAllTimers();
+});


Why was this file modified?

Because there was an error when running the test locally, I have deleted it in order to submit the test coverage. Please check again. Thank you

I didn't encounter this issue when running it locally. What's your operating system and the command you're running?

I just used test coverage I did run npm run test:coverage,

Now that the test coverage is perfect, I've deleted this line. It's likely a problem with my environment. If you're wondering about the error, I can share it with you. I'm running Windows 11 and running Node v18.17.0. Adding this line allows me to test coverage locally, rather than uploading it to GitHub for Codecov to run. See the image below:

…y large, the component may return a value outside the max range test.

inottn · 2025-09-22T08:55:34Z

packages/vant/src/slider/test/index.spec.ts

+test('should return prevSteppedValue when distanceToMax > distanceToPrev', () => {
+  const wrapper = mount(Slider, {
+    props: { min: 0, max: 8, step: 12, modelValue: 2 },
+  });
+
+  const emitted = wrapper.emitted('update:modelValue');
+  if (emitted && emitted.length > 0) {
+    const result = emitted[emitted.length - 1][0] as number;
+    expect(result).toBe(0); // Should return prevSteppedValue
+  }
+});


The newly added second test case did not enter the if(steppedValue > max) conditional branch. When I commented out the Slider-related modifications and ran the new test case, the test still passed.

Yes, that's it. I updated this.

This new test corrected test cases:

test('should enter steppedValue > max branch', () => { const wrapper = mount(Slider, { props: { min: 0, max: 12, step: 20, modelValue: 0 }, }); wrapper.setProps({ modelValue: 11 }); const emitted = wrapper.emitted('update:modelValue'); if (emitted && emitted.length > 0) { const result = emitted[emitted.length - 1][0] as number; expect(result).toBeGreaterThanOrEqual(0); expect(result).toBeLessThanOrEqual(12); } });

@inottn @chenjiahan

…y large, the component may return a value outside the max range test.

lllomh added 7 commits September 18, 2023 18:23

fix(Picker): fix bug the content of the popup window is not consisten…

30af04b

…t with the Chinese version under the English version

fix(Picker): Fix the issue that scroll wheel selection list is not su…

37fc0b8

…pported on pc

fix(Picker): Rollback of new commits last one

72c750b

Merge branch 'main' into main

f7685da

Merge branch 'youzan:main' into main

c5b0ba9

Merge branch 'youzan:main' into main

3430626

fix(Button): When the step value of the Slider component is relativel…

be2e46d

…y large, the component may return a value outside the max range.

github-actions bot added the change: fix label Sep 17, 2025

lllomh closed this Sep 17, 2025

fix(Button): When the step value of the Slider component is relativel…

c7fe1a6

…y large, the component may return a value outside the max range test.

lllomh reopened this Sep 17, 2025

fix(Button): When the step value of the Slider component is relativel…

13248f7

…y large, the component may return a value outside the max range test.

lllomh closed this Sep 17, 2025

lllomh reopened this Sep 17, 2025

lllomh added 15 commits September 17, 2025 11:44

fix(Button): When the step value of the Slider component is relativel…

42d9458

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

5d85c3b

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

8137a85

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

5a48f52

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

d24b1e7

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

e37c8fe

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

91259b9

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

a3774da

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

288cd25

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

b5ee53a

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

06b35df

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

22ceb87

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

a7e2f52

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

bc40733

…y large, the component may return a value outside the max range test.

fix(Button): When the step value of the Slider component is relativel…

6bbbb14

…y large, the component may return a value outside the max range test.

lllomh added 12 commits September 21, 2025 09:50

fix(Slider): When the step value of the Slider component is relativel…

aeadef6

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

f9a1d0d

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

f8c6668

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

835676e

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

e996104

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

9282a87

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

728aae9

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

e8c4b50

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

aa4f540

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

46318fe

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

7f7ce3f

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

a5e8351

…y large, the component may return a value outside the max range test.

chenjiahan changed the title ~~fix(Slider): fix the Slider component issue. When the step value is relatively large, the component may return a value outside the max range. #13625~~ fix(Slider): value may exceed max when step is large Sep 21, 2025

chenjiahan reviewed Sep 21, 2025

View reviewed changes

fix(Slider): When the step value of the Slider component is relativel…

a90cace

…y large, the component may return a value outside the max range test.

Merge branch 'main' into main

572efca

inottn reviewed Sep 22, 2025

View reviewed changes

lllomh added 3 commits September 22, 2025 10:51

fix(Slider): When the step value of the Slider component is relativel…

d0cafc7

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

8de91ac

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

c039ba7

…y large, the component may return a value outside the max range test.

inottn reviewed Sep 22, 2025

View reviewed changes

lllomh added 2 commits September 22, 2025 17:21

fix(Slider): When the step value of the Slider component is relativel…

ac685ee

…y large, the component may return a value outside the max range test.

fix(Slider): When the step value of the Slider component is relativel…

9dcc149

…y large, the component may return a value outside the max range test.

lllomh requested review from chenjiahan and inottn September 23, 2025 04:34

Uh oh!

fix(Slider): value may exceed max when step is large #13632

Are you sure you want to change the base?

fix(Slider): value may exceed max when step is large #13632

Conversation

lllomh commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed an issue where the Slider component's step value might exceed the max value when the step value is large #13625.

Issue Description

Cause Analysis

Solution

Before:

After modification:

Core Changes

Test Verification

Backward Compatibility

User Experience Improvements

This PR fix addresses edge issues with larger step sizes, making component behavior more consistent with user expectations while maintaining backward compatibility.

Uh oh!

codecov-commenter commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chenjiahan left a comment

Choose a reason for hiding this comment

Uh oh!

lllomh commented Sep 21, 2025

Uh oh!

chenjiahan commented Sep 21, 2025

Uh oh!

lllomh commented Sep 21, 2025

Uh oh!

lllomh commented Sep 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lllomh Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lllomh Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lllomh Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lllomh commented Sep 17, 2025 •

edited

Loading

Fixed an issue where the Slider component's `step` value might exceed the `max` value when the step value is large #13625.

codecov-commenter commented Sep 17, 2025 •

edited

Loading

lllomh Sep 22, 2025 •

edited

Loading

lllomh Sep 22, 2025 •

edited

Loading

lllomh Sep 22, 2025 •

edited

Loading