robisim74 / angular-l10n

Angular library to translate texts, dates and numbers
MIT License
380 stars 59 forks source link

localeValidation.parseNumber return NaN for hu locale on latest chrome - Version 76.0.3809.87 (Official Build) (64-bit) #261

Closed moromete closed 4 years ago

moromete commented 5 years ago

I'm submitting a...

[x ] bug report [ ] feature request [ ] support request


**Expected behavior**
this.localeValidation.parseNumber('1,123', '1.0-6', 'hu');
must return 1.123
decimalSeparator should be ','

**Actual behavior**
this.localeValidation.parseNumber('1,123', '1.0-6', 'hu');
return NaN
decimalSeparator is detected as '9'

**Steps to reproduce the behavior**
chrome - Version 76.0.3809.87 (Official Build) (64-bit) - on other browsers is detecting OK so the issue is just on chrome. From what I found until now because 
new Intl.NumberFormat('hu').format(-1000.9) return -1000,9 instead of -1 000,9 as until now on chrome older versions or other browsers.
so function  getDecimalCode is reading the decimal character from the wrong position because the string is one char shorter.

StackBlitz Template: 
https://stackblitz.com/edit/angular-l10n-3yjp1x

**Specifications: Angular version, library version, environment, browser**
angular-l10n 8.1.0
chrome Version 76.0.3809.87 (Official Build) (64-bit)
robisim74 commented 5 years ago

@moromete Thanks for investigating and reporting. The point is that for hu the correct thousand separator shoud be an empty space (like for fr), isn't right? So this seems a Chrome bug.

We could add a check into the method to detect if the thousand separator is present (by the number of positions), just to solve the error (NaN).

moromete commented 5 years ago

Yes you are correct thousand separator is empty space and indeed is a chrome bug. As a hot fix I just added the empty space if the length of the string is shorter after formatDecimal

var localeValue = this.locale.formatDecimal(value, '1.1-1', defaultLocale);

if(localeValue.length == 7) { //fix for google chrome hu language localeValue = [localeValue.slice(0, 2), ' ', localeValue.slice(2)].join(''); }

but I don't know how this will affect or if will affect the other formats like right to left numbers

robisim74 commented 5 years ago

I prefer a generic fix for all languages that should return the value without thousand separator if it missing. Let you know.

Just a question: did you already open an issue on Chrome?

moromete commented 5 years ago

Nope. I was not sure if we should adapt to chrome or chrome should fix it :)

robisim74 commented 5 years ago

I think we should report an issue on Chrome and add a fix to avoid NaN in case of browser error... just to no break our apps.

robisim74 commented 5 years ago

I published a beta version with the fix:

npm install --save angular-l10n@next

It fixes the lack of the separator in each language, and resolves the use of space as a separator (in your case, hu, space as thousand separator actually should work in all browsers, but Chrome, where it should work without separator).

Please, try it and give me a feedback.

moromete commented 5 years ago

In my ap is working OK now. Thank you for support!

robisim74 commented 5 years ago

Thanks for collaboration! Released v8.1.1

Greetings

moromete commented 5 years ago

I found out also a strange problem that affect languages with space as thousand separator. It is also related to Intl.NumberFormat and this give false negative from validateNumber. https://stackblitz.com/edit/angular-l10n-ymh4xl The problem is that new Intl.NumberFormat('hu').format(1000); does not put a real space in thousand separator. If I replace the \s with ' ' is working OK. This is valid in Firefox. On Chrome we get reversed result meaning the typed number is invalidated and the one obtained by Intl.NumberFormat is not

robisim74 commented 5 years ago

I'm not sure I understood the problem here. The directive uses Intl.NumberFormat behind the scenes to validate the number: it does not accept a number formatted with the Intl API as an argument: then the API does not use space, but the character NO-BREAK SPACE, and that is why you must do the replace.

moromete commented 5 years ago

In Firefox Intl.NumberFormat use something different than regular space. And the regular expression used inside LocaleValidation.validateNumber to validate return that is not valid. Because the regular expression is build with space as grouping. In Chrome the result are somehow reversed because missing thousand separator I think.

robisim74 commented 5 years ago

They are correct.

moromete commented 5 years ago

Firefox first case give correct result for nr typed by hand but wrong one for the one obtained by Intl.NumberFormat - "nr2" Chrome the results are reversed and I don't quite understand it fully if is just because of the Intl.NumberFormat.

Both numbers typed or formatted by Intl.NumberFormat should be valid in the same time.

robisim74 commented 5 years ago

I repeat: you cannot pass as argument to the directive a number already transformed with Intl.NumberFormat as your nr2.

moromete commented 5 years ago

OK Then maybe I use it wrongly but what is the proper way to create a form control with a default value ("one already stored for example in a database") and to show it already formatted inside of an input. My logic say is normal to save 1000 in database send it to client -> formatNumber with angular (which I suspect uses Intl.NumberFormat internaly ) -> display the formatted number in user input -> and if user does not modify or type a new number -> the number to be valid.

As for Chrome is it not possible to create a case to accept numbers with space as grouping even if the grouping is missing in Intl.NumberFormat ?

robisim74 commented 5 years ago

Ok, I see. I'll try to find a fix, but for the different behavior of Chrome I can't do anything. Let you know.

robisim74 commented 5 years ago

Released v8.1.2 Let me know.

moromete commented 5 years ago

Working. Thanks for quick reaction ! I put again an issue to chrome. https://stackblitz.com/edit/js-qgj83f You can report it too to have more luck on fixing ;)

robisim74 commented 4 years ago

Closed via https://github.com/robisim74/angular-l10n/releases/tag/v9.0.0