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

fix(number-field, slider): floating point roundoff precision bug #4263

Merged
merged 30 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
3f6fde4
fix(number-field): fixed precision bug
blunteshwar Apr 11, 2024
c8a49bc
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 12, 2024
15bf141
chore(number-field): some minor fix
blunteshwar Apr 12, 2024
85bcd8e
chore(slider, number-field): added some tests and story
blunteshwar Apr 12, 2024
87f073b
Apply suggestions from code review
blunteshwar Apr 15, 2024
763c8d1
Revert "Apply suggestions from code review"
blunteshwar Apr 15, 2024
831ebbb
chore(number-field): updated test name
blunteshwar Apr 15, 2024
5f4fc2e
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 17, 2024
dd6ee09
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 29, 2024
c0675ae
fix(number-field): number formatter to format decimal values
blunteshwar Apr 29, 2024
5b080da
Merge branch 'blunteshwar/slider-not-rounding-values' of github.com:a…
blunteshwar Apr 29, 2024
3d5f8af
chore(number-field): minor fix
blunteshwar Apr 29, 2024
81f20ca
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 29, 2024
947091b
chore(number-field): implemented formatter using getter
blunteshwar Apr 29, 2024
53e621a
Merge branch 'blunteshwar/slider-not-rounding-values' of github.com:a…
blunteshwar Apr 29, 2024
8b52e92
chore(number-field): implemented single number formatter
blunteshwar Apr 29, 2024
88a4a4d
chore(number-field): some minor fix
blunteshwar Apr 29, 2024
8ce6361
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 29, 2024
a21b404
chore(number-field): minor fixes
blunteshwar Apr 29, 2024
e077c20
Merge branch 'blunteshwar/slider-not-rounding-values' of github.com:a…
blunteshwar Apr 29, 2024
d0b8d53
chore(number-field): implemented an independent formatter
blunteshwar Apr 29, 2024
70b6b14
chore(number-field): minor change
blunteshwar Apr 29, 2024
395e004
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 29, 2024
14cbe9e
chore(number-field): cached digitsAfterDecimal
blunteshwar Apr 29, 2024
d4affc5
Merge branch 'blunteshwar/slider-not-rounding-values' of github.com:a…
blunteshwar Apr 29, 2024
207ec62
chore(number-field): some minor changes
blunteshwar Apr 29, 2024
f5ad973
chore(number-field): minor fix
blunteshwar Apr 29, 2024
05f8bf9
chore(number-field): minor ts fix
blunteshwar Apr 29, 2024
c8e2faf
chore(number-field): minor ts check fix
blunteshwar Apr 29, 2024
b218046
Merge branch 'main' into blunteshwar/slider-not-rounding-values
blunteshwar Apr 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 31 additions & 1 deletion packages/number-field/src/NumberField.ts
Expand Up @@ -495,7 +495,9 @@ export class NumberField extends TextfieldBase {
// Step shouldn't validate when 0...
if (this.step) {
const min = typeof this.min !== 'undefined' ? this.min : 0;
const moduloStep = (value - min) % this.step;
const moduloStep = parseFloat(
this.valueFormatter.format((value - min) % this.step)
);
const fallsOnStep = moduloStep === 0;
if (!fallsOnStep) {
const overUnder = Math.round(moduloStep / this.step);
Expand All @@ -510,6 +512,7 @@ export class NumberField extends TextfieldBase {
value -= this.step;
}
}
value = parseFloat(this.valueFormatter.format(value));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to only format at the end to reduce the chances of fidelity loss?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, We need this const moduloStep = parseFloat(this.valueFormatter.format((value - min) % this.step)); format because in some cases moduloStep is evaluated as 0.00000000000017 or very close to zero , but it should be evaluated to 0, hence this formatting is required.
And we need this value = parseFloat(this.valueFormatter.format(value)); because after modulostep is evaluated the value of step is added to value and hence we again have to finally format it.
Basically the first format is to correctly calculate the moduloStep and the second format is to correctly calculate the value upto required decimal places in accordance with step.

}
value *= signMultiplier;
return value;
Expand Down Expand Up @@ -560,8 +563,23 @@ export class NumberField extends TextfieldBase {
: this._numberFormatter;
}

protected get valueFormatter(): NumberFormatter {
if (this.step && !this._valueFormatter) {
const digitsAfterDecimal =
this.step != Math.floor(this.step)
? this.step.toString().split('.')[1].length
: 0;
this._valueFormatter = new NumberFormatter(
this.languageResolver.language,
{ maximumFractionDigits: digitsAfterDecimal }
);
}

return this._valueFormatter;
}
private _numberFormatter?: NumberFormatter;
private _numberFormatterFocused?: NumberFormatter;
private _valueFormatter!: NumberFormatter;

protected get numberParser(): NumberParser {
if (!this._numberParser || !this._numberParserFocused) {
Expand Down Expand Up @@ -685,6 +703,18 @@ export class NumberField extends TextfieldBase {
);
this.value = value;
}
if (changes.has('step')) {
if (this.step) {
const digitsAfterDecimal =
this.step != Math.floor(this.step)
? this.step.toString().split('.')[1].length
: 0;
this._valueFormatter = new NumberFormatter(
this.languageResolver.language,
{ maximumFractionDigits: digitsAfterDecimal }
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to populate the cache here, if we evict it, the use of this.valueFormatter later will automatically populate it on demand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

}
}
super.update(changes);
}

Expand Down
21 changes: 21 additions & 0 deletions packages/number-field/test/inputs.test.ts
Expand Up @@ -314,6 +314,27 @@ describe('NumberField - inputs', () => {
expect(el.value).to.equal(255);
});
});
describe('with floating point numbers', () => {
it('do not crash the Number Field', async () => {
const el = await getElFrom(minMax(minMax.args));
el.setAttribute('min', '0.1');
el.setAttribute('step', '0.01');
el.setAttribute('value', '0.5');
await elementUpdated(el);

el.focus();
await sendKeys({
type: '6',
Rajdeepc marked this conversation as resolved.
Show resolved Hide resolved
});
await elementUpdated(el);
expect(el.formattedValue).to.equal('0.56');
await sendKeys({
press: 'Enter',
});
await elementUpdated(el);
expect(el.value).to.equal(0.56);
});
});
describe('locale specific', () => {
it('can determine the group symbol', async () => {
const [languageContext] = createLanguageContext('es-ES');
Expand Down
26 changes: 26 additions & 0 deletions packages/slider/stories/slider.stories.ts
Expand Up @@ -450,6 +450,32 @@ editableWithDefaultValue.swc_vrt = {
skip: true,
};

export const editableWithFractionValue = (
args: StoryArgs = {}
): TemplateResult => {
return html`
<div style="width: 500px; margin: 12px 20px;">
<sp-slider
editable
max="255"
min="0.1"
value="0.5"
step="0.01"
default-value="18"
@input=${handleEvent(args)}
@change=${handleEvent(args)}
...=${spreadProps(args)}
>
Angle
</sp-slider>
</div>
`;
};

editableWithFractionValue.swc_vrt = {
skip: true,
Rajdeepc marked this conversation as resolved.
Show resolved Hide resolved
};

export const editableDisabled = (args: StoryArgs = {}): TemplateResult => {
return html`
<div style="width: 500px; margin: 12px 20px;">
Expand Down