fix: NumberParser useGrouping false returned wrong value

[snowystinger]

Closes #9086

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

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the documentation
• View the S2 docs

[AnnaYuS]

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();

[snowystinger]

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?

[yakobe]

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();

[snowystinger]

(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!

[snowystinger]

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

[snowystinger]

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

[snowystinger]

closing in favour of #8592