Skip to content

fix: NumberParser useGrouping false returned wrong value #9089

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+22 −1
Closed

fix: NumberParser useGrouping false returned wrong value #9089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3853ee7
fix: NumberParser useGrouping false returned wrong value
snowystinger Oct 23, 2025
97d82db
add typing and pasting test
snowystinger Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/@internationalized/number/src/NumberParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,8 @@ function getSymbols(locale: string, formatter: Intl.NumberFormat, intlOptions: I
maximumSignificantDigits: 21,
roundingIncrement: 1,
roundingPriority: 'auto',
roundingMode: 'halfExpand'
roundingMode: 'halfExpand',
useGrouping: true
});
// Note: some locale's don't add a group symbol until there is a ten thousands place
let allParts = symbolFormatter.formatToParts(-10000.111);
Expand Down
5 changes: 5 additions & 0 deletions packages/@internationalized/number/test/NumberParser.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ describe('NumberParser', function () {
expect(new NumberParser('en-US', {style: 'decimal'}).parse('1abc')).toBe(NaN);
});

it('should return NaN for invalid grouping', function () {
expect(new NumberParser('en-US', {useGrouping: false}).parse('1234,7')).toBe(12347);
expect(new NumberParser('de-DE', {useGrouping: false}).parse('1234.7')).toBe(12347);
Comment on lines +60 to +61
Copy link

Choose a reason for hiding this comment

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

Thanks for looking into it! @snowystinger
Could you clarify the fix a bit more

My expectation here would be more in this direction
expect(new NumberParser('en-US', {useGrouping: false}).parse('1234,7')).toBeNaN();
expect(new NumberParser('de-DE', {useGrouping: false}).parse('1234.7')).toBeNaN();

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, from the description

The grouping character should be treated as extraneous and be removed as it doesn't actually carry meaning, no matter where it was added.

Can you say more as to why returning NaN would be your expectation though?

Copy link

Choose a reason for hiding this comment

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

The grouping character should be treated as extraneous and be removed as it doesn't actually carry meaning, no matter where it was added.

This is not really correct and could have serious consequences. From the perspective of a German locale, 1234.7 is not a number. If a user enters it then it is incorrect (they probably entered using a different locale format, but we need to be sure). Excel etc would also not understand it as a number.

If it is transformed to 12347 then this could have very serious consequences for further calculations that depend on it. It would be safer to parse it as NaN so that the error can be caught and handled correctly before it propagates further.

I would expect:

    expect(new NumberParser('en-US', {useGrouping: false}).parse('12,347')).toBe(12347);
    expect(new NumberParser('de-DE', {useGrouping: false}).parse('12.347')).toBe(12347);
    
    expect(new NumberParser('en-US', {useGrouping: false}).parse('1234,7')).toBeNaN();
    expect(new NumberParser('de-DE', {useGrouping: false}).parse('1234.7')).toBeNaN();

Copy link
Member Author

Choose a reason for hiding this comment

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

(bit of a stream of conscious as I was working through it while writing)
That makes sense. As a counter though, we don't want to just block out everything.

new NumberParser('en-US', {style: 'decimal'}).parse('+10') // should parse to 10, even though sign display isn't "always"

I know this particular case is more ambiguous than my example.

But hang on, let's back up, how are you getting to a point where this is coming up? Are you running isValidPartialNumber before? that's how our NumberField stops invalid numbers/characters.

For instance, this behaves as I believe you expect.

    it('should return NaN for invalid grouping', function () {
      expect(new NumberParser('en-US', {useGrouping: false}).isValidPartialNumber('1234,7')).toBe(false);
      expect(new NumberParser('de-DE', {useGrouping: false}).isValidPartialNumber('1234.7')).toBe(false);
    });

Parse will do its absolute best to convert something into a number. If you want to be strict about it, I suggest running isValidPartialNumber first. Hopefully this can get you unblocked in the meantime.

I'm going to close this PR as I've handled it over in #8592 now because I wanted to make sure I wasn't breaking any assumptions there and it was related to some work I was doing there to handle ambiguous vs non-ambiguous cases.

We can probably continue any discussion over there. Thanks for the feedback!

Copy link
Member Author

@snowystinger snowystinger Oct 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sigh, had another thought, it's not necessarily obvious whether an input allows grouping symbols. So someone in english could be trying to paste a number from some other place which does include a grouping symbol into a field that doesn't allow it, this should still work I think.

For example, if I pasted '1,024' in an en-US NumberField with useGrouping: false, then I think it should be parsed to 1024

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, added comments over there, at least the locales match in terms of handling now

});

describe('currency', function () {
it('should parse without the currency symbol', function () {
expect(new NumberParser('en-US', {currency: 'USD', style: 'currency'}).parse('10.50')).toBe(10.5);
Expand Down
15 changes: 15 additions & 0 deletions packages/react-aria-components/test/NumberField.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,4 +189,19 @@ describe('NumberField', () => {
expect(input).not.toHaveAttribute('aria-describedby');
expect(numberfield).not.toHaveAttribute('data-invalid');
});

it('should not type the grouping characters when useGrouping is false', async () => {
let {getByRole} = render(<TestNumberField formatOptions={{useGrouping: false}} />);
let input = getByRole('textbox');

await user.keyboard('102,4');
expect(input).toHaveAttribute('value', '1024');

await user.clear(input);
expect(input).toHaveAttribute('value', '');

await user.paste('102,4');
await user.tab();
expect(input).toHaveAttribute('value', '1024');
});
});