Skip to content
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

ion-range's ionChange event emitter emits fractional numbers unexpectedly #21968

Closed
cmer4 opened this issue Aug 24, 2020 · 3 comments · Fixed by #27375
Closed

ion-range's ionChange event emitter emits fractional numbers unexpectedly #21968

cmer4 opened this issue Aug 24, 2020 · 3 comments · Fixed by #27375
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@cmer4
Copy link

cmer4 commented Aug 24, 2020

Ionic version:
[ ] 4.x
[x] 5.3.1

Current behavior:
Given the following code:

    <ion-range [min]="option.min" [max]="option.max"
      [step]="option.step" (ionChange)="log($event); option.value = $event.target.value">
    </ion-range>
export class AppComponent {

  option: = {
          type: "range",
          label: "Distance",
          name: "distance",
          step: 0.05,
          min: 0,
          max: 1,
          value: 0.1
  }

  log(event) {
    console.log(event.target.value)
  }

}

We can observe fractional numbers (0.35000000000000003) assigned as input value.

Expected behavior:
The value should not be fractional number.

Steps to reproduce:
https://stackblitz.com/edit/ionic-angular-v5-ic1yf2?file=src/app/app.component.ts

Or just create ion-range with steps below 1 (e.g. 0.05 as a step)

I guess this relates to Javascript's specificity (quirk)?:
Numbers in JavaScript are "double-precision 64-bit format IEEE 754 values", according to the spec...

So if we would use standard HTML input type range:

<input type="range" [min]="option.min" [max]="option.max"
      [step]="option.step" onchange="console.log(event.target.value)"/>

Then there is no fractional value.

Not sure what is the best to address this but at the least we could remind of the 0.1 + 0.2 = 0.30000000000000004 issue in Javascript and that developer has to "toFixed" this situation?

@ionitron-bot ionitron-bot bot added the triage label Aug 24, 2020
@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Aug 24, 2020
@ionitron-bot ionitron-bot bot removed the triage label Aug 24, 2020
@ReneZeidler
Copy link

The issue stems from the function ratioToValue:

if (step > 0) {
  value = Math.round(value / step) * step + min;
}

For example, I have a range input with min = 0.00, max = 3.00 and step = 0.01. If I move the slider to value 2.80, the caculation is as follows:

> value = Math.round(2.8 / 0.01) * 0.01 + 0;
< 2.8000000000000003

That's because the multiplication by 0.01 as the last step is inaccurate, because 0.01 can't be represented exactly as a floating point number. If instead we divide by the inverse of the step, we get the correct value.

> inverse = 1 / 0.01
< 100
> value = Math.round(2.8 * inverse) / inverse + 0;
< 2.8

This works because 100 can be represented exactly as a floating point number, so the result will always be the closest floating point number to the true result.

The two results are actually different floating point values, it's not only an issue with the string representation:

> 2.8000000000000003 === 2.8
< false

Of course taking the inverse doesn't always make sense. If the step is an integer, it can always be represented exactly so there is no issue.

I think it would make sense to use the inverse when 0 < step < 1, because in 99% of practical use cases, step will be the inverse of an integer (e.g. 0.5, 0.25, 0.1, etc.), so the inverse is definitely exact. I'm not fluent enough in floating point math to know if using the inverse is always strictly better in cases where step is not the inverse of an integer. (It definitely shouldn't be used for values > 1, because it would turn exact values like 100 into inexact values like 0.01).

So an improved function would look like this:

if (step >= 1) {
  value = Math.round(value / step) * step + min;
} else if (step > 0) {
  const inverseStep = 1 / step;
  value = Math.round(value * inverseStep) / inverseStep + min;
}

The reason that the standard browser range input doesn't have those issues is that it uses decimals instead of floating point numbers internally, e.g. in Chromium/Blink.


The reason why this is an issue in the first place is that when the ion-range is bound to a FormControl or via [(ngModel)], the bound value will be incorrect, and there is no easy way to correct it when being set. Instead it has to be corrected wherever the value is used, which can be in many different places.

@liamdebeasi liamdebeasi added type: bug a confirmed bug report and removed type: bug a confirmed bug report labels Apr 21, 2023
averyjohnston added a commit that referenced this issue May 8, 2023
…avoid floating point rounding errors (#27375)

Issue number: Resolves #21968

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

When using fractional values for `min`, `max`, or `step`, it is possible
for floating point rounding errors to cause unexpected values to be
emitted. For example, `step="0.05" min="0.1" max="1"` emits a value of
`0.150000000004` after moving one step.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

Values are rounded to the max number of decimal places between the three
props. Note that it isn't mathematically possible to arrive at a value
with more decimal places than the props*, since addition (i.e. starting
at `min` and adding multiples of `step`) can't increase the precision of
a number.

\* Unless the `value` is set manually, but in that case, `ion-range`
currently snaps to a multiple of `step` as soon as the slider is moved,
resuming normal behavior.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->
@averyjohnston
Copy link
Contributor

This has been resolved via #27375 and the fix will be available in a future release of Ionic. Thank you!

@ionitron-bot
Copy link

ionitron-bot bot commented Jun 7, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jun 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
4 participants