adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.99k stars 1.13k forks source link

NumberField - Comma as the first character displays unreadable values #3862

Open mikeblatter opened 1 year ago

mikeblatter commented 1 year ago

Appears that the NumberField doesn't handle comma's as first character of the field and causes values to become unreadable.

Expected Behaviour

Sets value to last value

Actual Behaviour

Values displayed are unreadable

Reproduce Scenario (including but not limited to)

Entering , as the first character in the NumberField

Steps to Reproduce

Platform and Version

Any browser

Logs taken while reproducing problem

None

reidbarber commented 1 year ago

Interesting, looks like it is ١, or the Arabic number 1, so maybe something true is evaluating to 1 with the Arabic locale (being first in alphabetical order?).

mikeblatter commented 1 year ago

Looks similar to this Number-Field Eastern Arabic Number format. Didn't find it in the search till I looked for Arabic

reidbarber commented 1 year ago

Good find, it looks like the regex here:

https://github.com/adobe/react-spectrum/blob/79f62e8fbf64e8359fb603f3a1cc41a2808389e2/packages/%40internationalized/number/src/NumberParser.ts#L218-L220

should be updated to [\\p{White_Space},] to handle this case.

snowystinger commented 1 year ago

I don't think that the whitespace regex should be modified to include a comma. That would already be in either the group or the decimal value typically.

Typing "," followed by numbers is not a valid number, which is why it's preventing you from typing any numbers. So that part is working as expected. However, it's switching to some arabic number and displaying that on blur, which it shouldn't, it should just return to any empty field.

I noticed that it only does the Arabic number if I try to type numbers after the comma, if I only type that character and leave the field, it behaves as expected.

snowystinger commented 1 year ago

I dug into this today. It appears that our NumberParse will determine , to be a valid arabic numeral, because it's part of the allowed characters in a en-US-u-nu-arab number string. https://github.com/adobe/react-spectrum/blob/main/packages/%40internationalized/number/src/NumberParser.ts#L78

On its own, this isn't an issue, however, when there is a default value, and a nonsense value is entered into the input, such as a comma all on its own, we'll restore the original value because it doesn't seem like the user meant to delete the number. When we restore it, we use the new parser and formatted, which gives us arabic.

Here's a unit test for it:

  it('should maintain original parser and formatted when restoring a previous value', () => {
    let {textField} = renderNumberField({onChange: onChangeSpy, defaultValue: 10});
    expect(textField).toHaveAttribute('value', '10');

    userEvent.tab();
    userEvent.clear(textField);
    typeText(textField, ',123');
    act(() => {textField.blur();});
    expect(textField).toHaveAttribute('value', '');
    expect(onChangeSpy).not.toHaveBeenCalled();
  });

I haven't figured out how to fix it yet.