From ab99bb0cf29cd1450246b1295560214899743de2 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Wed, 3 May 2023 13:31:33 -0500 Subject: [PATCH 1/8] add test --- .../components/range/test/basic/range.e2e.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 core/src/components/range/test/basic/range.e2e.ts diff --git a/core/src/components/range/test/basic/range.e2e.ts b/core/src/components/range/test/basic/range.e2e.ts new file mode 100644 index 00000000000..1bd57032618 --- /dev/null +++ b/core/src/components/range/test/basic/range.e2e.ts @@ -0,0 +1,24 @@ +import { expect } from '@playwright/test'; +import { configs, test } from '@utils/test/playwright'; + +configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('range: basic'), () => { + test('should not emit fractional values between steps', async ({ page, pageUtils }) => { + test.info().annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/21968', + }); + + await page.setContent(` + + `, config); + + const range = page.locator('ion-range'); + await pageUtils.pressKeys('Tab'); // focus range + await pageUtils.pressKeys('ArrowRight'); // note that bug doesn't trigger if you set value manually + await page.waitForChanges(); + + await expect(range).toHaveJSProperty('value', 0.15); + }); + }); +}); \ No newline at end of file From a2420ee69db8ecde774001990ed7a0ba77c0c664 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Wed, 3 May 2023 15:16:44 -0500 Subject: [PATCH 2/8] lint --- core/src/components/range/test/basic/range.e2e.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/src/components/range/test/basic/range.e2e.ts b/core/src/components/range/test/basic/range.e2e.ts index 1bd57032618..6c7701a1ae5 100644 --- a/core/src/components/range/test/basic/range.e2e.ts +++ b/core/src/components/range/test/basic/range.e2e.ts @@ -9,9 +9,10 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { description: 'https://github.com/ionic-team/ionic-framework/issues/21968', }); - await page.setContent(` - - `, config); + await page.setContent( + ``, + config + ); const range = page.locator('ion-range'); await pageUtils.pressKeys('Tab'); // focus range @@ -21,4 +22,4 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { await expect(range).toHaveJSProperty('value', 0.15); }); }); -}); \ No newline at end of file +}); From ae1f2ea7b718f29a11f9c0d8f2ccd35cb030628a Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Wed, 3 May 2023 15:17:02 -0500 Subject: [PATCH 3/8] round to max num of decimal places between min, max, and step --- core/src/components/range/range.tsx | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/core/src/components/range/range.tsx b/core/src/components/range/range.tsx index 6e7e04283c5..a8e59e4cc27 100644 --- a/core/src/components/range/range.tsx +++ b/core/src/components/range/range.tsx @@ -902,10 +902,26 @@ const renderKnob = ( const ratioToValue = (ratio: number, min: number, max: number, step: number): number => { let value = (max - min) * ratio; + if (step > 0) { + // round to nearest multiple of step, then add min value = Math.round(value / step) * step + min; } - return clamp(min, value, max); + const clampedValue = clamp(min, value, max); + + const getDecimalPlaces = (n: number) => { + if (n % 1 === 0) return 0; + return n.toString().split('.')[1].length; + }; + + const maxPlaces = Math.max(getDecimalPlaces(min), getDecimalPlaces(max), getDecimalPlaces(step)); + + /** + * Avoid floating point rounding errors like 0.3 becoming + * 0.300000004. Note that it isn't possible for the "correct" + * value to have more decimal places than all three props. + */ + return Number(clampedValue.toFixed(maxPlaces)); }; const valueToRatio = (value: number, min: number, max: number): number => { From 3537810e01b967f72a264642aabeb478335b599a Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Thu, 4 May 2023 12:56:51 -0500 Subject: [PATCH 4/8] abstract logic into util --- core/src/components/range/range.tsx | 15 ++------- .../components/range/test/basic/range.e2e.ts | 25 --------------- .../floating-point/floating-point.spec.ts | 22 +++++++++++++ core/src/utils/floating-point/index.ts | 32 +++++++++++++++++++ 4 files changed, 56 insertions(+), 38 deletions(-) delete mode 100644 core/src/components/range/test/basic/range.e2e.ts create mode 100644 core/src/utils/floating-point/floating-point.spec.ts create mode 100644 core/src/utils/floating-point/index.ts diff --git a/core/src/components/range/range.tsx b/core/src/components/range/range.tsx index a8e59e4cc27..5f855a46ae0 100644 --- a/core/src/components/range/range.tsx +++ b/core/src/components/range/range.tsx @@ -4,6 +4,7 @@ import { Component, Element, Event, Host, Prop, State, Watch, h } from '@stencil import { getIonMode } from '../../global/ionic-global'; import type { Color, Gesture, GestureDetail, StyleEventDetail } from '../../interface'; import { findClosestIonContent, disableContentScrollY, resetContentScrollY } from '../../utils/content'; +import { fixRoundingErrors } from '../../utils/floating-point'; import type { LegacyFormController } from '../../utils/forms'; import { createLegacyFormController } from '../../utils/forms'; import type { Attributes } from '../../utils/helpers'; @@ -909,19 +910,7 @@ const ratioToValue = (ratio: number, min: number, max: number, step: number): nu } const clampedValue = clamp(min, value, max); - const getDecimalPlaces = (n: number) => { - if (n % 1 === 0) return 0; - return n.toString().split('.')[1].length; - }; - - const maxPlaces = Math.max(getDecimalPlaces(min), getDecimalPlaces(max), getDecimalPlaces(step)); - - /** - * Avoid floating point rounding errors like 0.3 becoming - * 0.300000004. Note that it isn't possible for the "correct" - * value to have more decimal places than all three props. - */ - return Number(clampedValue.toFixed(maxPlaces)); + return fixRoundingErrors(clampedValue, min, max, step); }; const valueToRatio = (value: number, min: number, max: number): number => { diff --git a/core/src/components/range/test/basic/range.e2e.ts b/core/src/components/range/test/basic/range.e2e.ts deleted file mode 100644 index 6c7701a1ae5..00000000000 --- a/core/src/components/range/test/basic/range.e2e.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { expect } from '@playwright/test'; -import { configs, test } from '@utils/test/playwright'; - -configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { - test.describe(title('range: basic'), () => { - test('should not emit fractional values between steps', async ({ page, pageUtils }) => { - test.info().annotations.push({ - type: 'issue', - description: 'https://github.com/ionic-team/ionic-framework/issues/21968', - }); - - await page.setContent( - ``, - config - ); - - const range = page.locator('ion-range'); - await pageUtils.pressKeys('Tab'); // focus range - await pageUtils.pressKeys('ArrowRight'); // note that bug doesn't trigger if you set value manually - await page.waitForChanges(); - - await expect(range).toHaveJSProperty('value', 0.15); - }); - }); -}); diff --git a/core/src/utils/floating-point/floating-point.spec.ts b/core/src/utils/floating-point/floating-point.spec.ts new file mode 100644 index 00000000000..33fbfca5a49 --- /dev/null +++ b/core/src/utils/floating-point/floating-point.spec.ts @@ -0,0 +1,22 @@ +import { getDecimalPlaces, fixRoundingErrors } from './index'; + +describe('floating point utils', () => { + describe('getDecimalPlaces', () => { + it('should calculate decimal places for a float', async () => { + const n = getDecimalPlaces(5.001); + expect(n).toBe(3); + }); + + it('should return no decimal places for a whole number', async () => { + const n = getDecimalPlaces(5); + expect(n).toBe(0); + }); + }); + + describe('fixRoundingErrors', () => { + it('should round to the highest number of places as references', async () => { + const n = fixRoundingErrors(5.12345, 1.12, 2.123); + expect(n).toBe(5.123); + }); + }); +}); diff --git a/core/src/utils/floating-point/index.ts b/core/src/utils/floating-point/index.ts new file mode 100644 index 00000000000..17a73678087 --- /dev/null +++ b/core/src/utils/floating-point/index.ts @@ -0,0 +1,32 @@ +export function getDecimalPlaces(n: number) { + if (n % 1 === 0) return 0; + return n.toString().split('.')[1].length; +} + +/** + * Fixes floating point rounding errors in a result by rounding + * to the same specificity, or number of decimal places (*not* + * significant figures) as provided reference numbers. If multiple + * references are provided, the highest number of decimal places + * between them will be used. + * + * The main use case is when numbers x and y are added to produce n, + * but x and y are floats, so n may have rounding errors (such as + * 3.1000000004 instead of 3.1). As long as only addition/subtraction + * occurs between x and y, the specificity of the result will never + * increase, so x and y should be passed in as the references. + * + * If multiplication, division, or other operations were used to + * calculate n, the rounded result may have less specificity than + * desired. For example, 1 / 3 = 0.33333(...), but + * fixRoundingErrors((1 / 3), 1, 3) will return 0, since both 1 and 3 + * are whole numbers. + * + * @param n The number to round. + * @param references Number(s) used to calculate n, or that should otherwise + * be used as a reference for the desired specificity. + */ +export function fixRoundingErrors(n: number, ...references: number[]) { + const maxPlaces = Math.max(...references.map((r) => getDecimalPlaces(r))); + return Number(n.toFixed(maxPlaces)); +} From 47d071175bc8543770c2b9da4edef722a1a63ea0 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Thu, 4 May 2023 15:13:03 -0500 Subject: [PATCH 5/8] rename util --- core/src/components/range/range.tsx | 4 ++-- core/src/utils/floating-point/floating-point.spec.ts | 4 ++-- core/src/utils/floating-point/index.ts | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/core/src/components/range/range.tsx b/core/src/components/range/range.tsx index 5f855a46ae0..a9e00cb6cc3 100644 --- a/core/src/components/range/range.tsx +++ b/core/src/components/range/range.tsx @@ -4,7 +4,7 @@ import { Component, Element, Event, Host, Prop, State, Watch, h } from '@stencil import { getIonMode } from '../../global/ionic-global'; import type { Color, Gesture, GestureDetail, StyleEventDetail } from '../../interface'; import { findClosestIonContent, disableContentScrollY, resetContentScrollY } from '../../utils/content'; -import { fixRoundingErrors } from '../../utils/floating-point'; +import { roundToMaxDecimalPlaces } from '../../utils/floating-point'; import type { LegacyFormController } from '../../utils/forms'; import { createLegacyFormController } from '../../utils/forms'; import type { Attributes } from '../../utils/helpers'; @@ -910,7 +910,7 @@ const ratioToValue = (ratio: number, min: number, max: number, step: number): nu } const clampedValue = clamp(min, value, max); - return fixRoundingErrors(clampedValue, min, max, step); + return roundToMaxDecimalPlaces(clampedValue, min, max, step); }; const valueToRatio = (value: number, min: number, max: number): number => { diff --git a/core/src/utils/floating-point/floating-point.spec.ts b/core/src/utils/floating-point/floating-point.spec.ts index 33fbfca5a49..9bc76377dad 100644 --- a/core/src/utils/floating-point/floating-point.spec.ts +++ b/core/src/utils/floating-point/floating-point.spec.ts @@ -1,4 +1,4 @@ -import { getDecimalPlaces, fixRoundingErrors } from './index'; +import { getDecimalPlaces, roundToMaxDecimalPlaces } from './index'; describe('floating point utils', () => { describe('getDecimalPlaces', () => { @@ -15,7 +15,7 @@ describe('floating point utils', () => { describe('fixRoundingErrors', () => { it('should round to the highest number of places as references', async () => { - const n = fixRoundingErrors(5.12345, 1.12, 2.123); + const n = roundToMaxDecimalPlaces(5.12345, 1.12, 2.123); expect(n).toBe(5.123); }); }); diff --git a/core/src/utils/floating-point/index.ts b/core/src/utils/floating-point/index.ts index 17a73678087..de103a2ae9b 100644 --- a/core/src/utils/floating-point/index.ts +++ b/core/src/utils/floating-point/index.ts @@ -19,14 +19,14 @@ export function getDecimalPlaces(n: number) { * If multiplication, division, or other operations were used to * calculate n, the rounded result may have less specificity than * desired. For example, 1 / 3 = 0.33333(...), but - * fixRoundingErrors((1 / 3), 1, 3) will return 0, since both 1 and 3 - * are whole numbers. + * roundToMaxDecimalPlaces((1 / 3), 1, 3) will return 0, since both + * 1 and 3 are whole numbers. * * @param n The number to round. * @param references Number(s) used to calculate n, or that should otherwise * be used as a reference for the desired specificity. */ -export function fixRoundingErrors(n: number, ...references: number[]) { +export function roundToMaxDecimalPlaces(n: number, ...references: number[]) { const maxPlaces = Math.max(...references.map((r) => getDecimalPlaces(r))); return Number(n.toFixed(maxPlaces)); } From e48cf1c2eed770bc4f3112d44678610a2b3db8b4 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Thu, 4 May 2023 15:14:27 -0500 Subject: [PATCH 6/8] fix wrong function name in test --- core/src/utils/floating-point/floating-point.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/utils/floating-point/floating-point.spec.ts b/core/src/utils/floating-point/floating-point.spec.ts index 9bc76377dad..80db4138b5b 100644 --- a/core/src/utils/floating-point/floating-point.spec.ts +++ b/core/src/utils/floating-point/floating-point.spec.ts @@ -13,7 +13,7 @@ describe('floating point utils', () => { }); }); - describe('fixRoundingErrors', () => { + describe('roundToMaxDecimalPlaces', () => { it('should round to the highest number of places as references', async () => { const n = roundToMaxDecimalPlaces(5.12345, 1.12, 2.123); expect(n).toBe(5.123); From 4c61735cb5b93318d311719ff52192412f3d3dea Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Mon, 8 May 2023 12:43:08 -0500 Subject: [PATCH 7/8] add comments about precision limitations --- core/src/utils/floating-point/index.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/src/utils/floating-point/index.ts b/core/src/utils/floating-point/index.ts index de103a2ae9b..64c0ca3764c 100644 --- a/core/src/utils/floating-point/index.ts +++ b/core/src/utils/floating-point/index.ts @@ -21,6 +21,15 @@ export function getDecimalPlaces(n: number) { * desired. For example, 1 / 3 = 0.33333(...), but * roundToMaxDecimalPlaces((1 / 3), 1, 3) will return 0, since both * 1 and 3 are whole numbers. + * + * Note that extremely precise reference numbers may lead to rounding + * errors not being trimmed, due to the error result having the same or + * fewer decimal places as the reference(s). This is acceptable as we + * would not be able to tell the difference between a rounding error + * and correct value in this case, but it does mean there is an implicit + * precision limit. If precision that high is needed, it is recommended + * to use a third party data type designed to handle floating point + * errors instead. * * @param n The number to round. * @param references Number(s) used to calculate n, or that should otherwise From d295ee405f8cee1bc947ff133fc6ebf3aedd8eef Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Mon, 8 May 2023 12:52:47 -0500 Subject: [PATCH 8/8] lint --- core/src/utils/floating-point/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/utils/floating-point/index.ts b/core/src/utils/floating-point/index.ts index 64c0ca3764c..d8ed8570741 100644 --- a/core/src/utils/floating-point/index.ts +++ b/core/src/utils/floating-point/index.ts @@ -21,7 +21,7 @@ export function getDecimalPlaces(n: number) { * desired. For example, 1 / 3 = 0.33333(...), but * roundToMaxDecimalPlaces((1 / 3), 1, 3) will return 0, since both * 1 and 3 are whole numbers. - * + * * Note that extremely precise reference numbers may lead to rounding * errors not being trimmed, due to the error result having the same or * fewer decimal places as the reference(s). This is acceptable as we