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

Comma decimal separator fails validation in amount inputs #2954

Open
brymut opened this issue Mar 4, 2020 · 7 comments · May be fixed by opencollective/opencollective-frontend#11002
Open

Comments

@brymut
Copy link

brymut commented Mar 4, 2020

Describe the bug
Using a comma as a decimal separator in currency fields doesn't work properly. Depending on browser typing a comma into a currency field either removes the whole value being typed or outright doesn't print the comma into the input.

To Reproduce

  1. Go to the Collective page, click on the gear icon and switch to the Tiers section.
  2. Add a new Tier or edit existing Tier.
  3. Click on any field on with a currency symbol and try inputting an amount with a decimal using a comma as a decimal separator.
  4. See error behaviour like in the screenshots below.
  • The value being edited completely removed from the form

Peek 02-03-2020 16-54

  • Comma not being printed into the form
    comma not recognised

Expected behaviour
Comma accepted/saved as decimal separator

Additional context
In many regions, commas are used as decimal separators in money values.

@brymut
Copy link
Author

brymut commented Mar 4, 2020

spotted by @Betree while reviewing opencollective/opencollective-frontend#3613

@brymut
Copy link
Author

brymut commented Mar 7, 2020

Initially, I had a look around and I came across this blog post on how <input type="number"> validation is implemented differently across browser vendors and also based on the locale.
For example, on my default browser (macOS, Chrome 80), I can't seem to be able to get a comma into the field.
comma not recognised

However, I was able to reproduce the error behaviour you pointed out in Firefox 73 and Safari 13.

And when I added a lang attribute (Norwegian for example) to the input element in components/InputField.js lang=nb commas were accepted in the input in Firefox 73 as well as periods.
comma recognised

But the error behaviour still persisted in the other browsers I had tried previously.

We could try adding lang attributes that would allow commas&periods to the input fields based on the currency set, however, behaviour across browsers may not be consistent.

@Betree
Copy link
Member

Betree commented Mar 9, 2020

@znarf also shared something on this topic the other day: https://technology.blog.gov.uk/2020/02/24/why-the-gov-uk-design-system-team-changed-the-input-type-for-numbers/.

There are two things here:

  • The question about localization, using , vs .. I've made some changes in our StyledInputAmount, when used as a controlled component with parseNumbers I believe the behavior is pretty comprehensive (but I haven't tested it on a lot of browsers):

Peek 09-03-2020 11-25

It probably won't be perfect, but even just relying on the native input and warning if the number is not valid is, in my opinion, an acceptable solution.

  • Another problem, that is to me the really problematic part here, is that all the options after the invalid one gets removed. Let's say you have [ 10, 20, 30, 40, 50 ] and you try to add a comma or a dot on 30 then it should never remove the options after (40 and 50).

So for me the two things that we need to do here are:

  • Have a clear error message if a number is not formatted properly
  • If a number is invalid, don't remove the entries after it

@brymut
Copy link
Author

brymut commented Mar 9, 2020

I'll get on this and submit a PR asap.

  • Another problem, that is to me the really problematic part here, is that all the options after the invalid one gets removed. Let's say you have [ 10, 20, 30, 40, 50 ] and you try to add a comma or a dot on 30 then it should never remove the options after (40 and 50).

However, for this, I haven't been able to recreate this behaviour unless I'm missing something/misunderstanding you. Like in the example you shared originally it only appears to remove the invalid value being edited and doesn't affect the values after it. (like in your example, out of [ 5, 25, 66] when you edit the 25 that makes it invalid, the 66 seems to remain intact.)
Peek 02-03-2020 16-54

@Betree
Copy link
Member

Betree commented Mar 9, 2020

@brymut Yes you're right, I was not remembering the behavior correctly. So to edit my second point:

  • If a number is invalid, don't remove it but eventually show a warning/an error message

Should ideally be resolved with #3438

@RichardLitt
Copy link
Member

This also happens when submitting an expense.

@Betree Betree changed the title Tiers: Comma decimal separator fails validation in currency fields. Comma decimal separator fails validation in amount inputs Feb 4, 2025
@Betree
Copy link
Member

Betree commented Feb 4, 2025

This is still happening with expenses. We need to have a look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants