New issue
Advanced search Search tips

Issue 791318 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Regression



Sign in to add a comment

toLocaleString / Intl.NumberFormat outputs currency code instead of symbol in en-GB locale

Reported by phonicde...@gmail.com, Dec 3 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.70 Safari/537.36

Steps to reproduce the problem:
new Intl.NumberFormat(
  'en-GB', {
    style: 'currency',
    currencyDisplay: 'symbol',
    currency: 'GBP'
}).format(1234.56)

What is the expected behavior?
£1234.56

What went wrong?
GBP1234.56 returned instead.

Works as expected in:
Safari 11
Firefox 57 & 59

Did this work before? Yes 63

Chrome version: 64  Channel: dev
OS Version: OS X 10.12.6
Flash Version:  

Changing currencyDisplay to 'name' returns '1234.56 Great British pounds' as expected.
 
Components: -Blink Blink>JavaScript>Internationalization
Labels: Needs-Bisect
Labels: -Pri-2 -Needs-Bisect hasbisect-per-revision ReleaseBlock-Stable Triaged-ET M-64 Needs-Triage-M64 OS-Linux OS-Windows Pri-1
Owner: js...@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on Windows 10, mac 10.12.6 and Ubuntu 14.04 using latest dev #64.0.3278.0 and latest canary #65.0.3285.0.

Bisect Information:
=====================
Good build: 64.0.3262.0    Revision(496533)
Bad Build : 64.0.3263.0    Revision(497279)

Change Log URL: 
https://chromium.googlesource.com/chromium/src/+log/010853f1f45f201f8b4141422ab9b5f0362309b0..befb16634bb440cf5442979ad262832b4cebd43e

From the above change log suspecting below change
Change-Id: I951a59ac26963e9e81341b32c95d5ac8f2f0d84c
Reviewed-on: https://chromium-review.googlesource.com/732193

jshin@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.
Note: Adding label ReleaseBlock-Stable as it seems to be a recent regression.

Thanks...!!

Comment 4 by js...@chromium.org, Dec 5 2017

Status: Started (was: Assigned)
It's definitely caused by ICU 60 update. Looking. 

It's interesting that en-GB and en-AU are 'broken' while other en's are ok. 

Among other combinations, de-CH for Euro is unexpected, 

en £1,234.56
en $1,234.56
en €1,234.56
en-GB GBP1,234.56
en-GB USD1,234.56
en-GB EUR1,234.56
en-CA £1,234.56
en-CA US$1,234.56
en-CA €1,234.56
en-US £1,234.56
en-US $1,234.56
en-US €1,234.56
en-AU GBP1,234.56
en-AU USD1,234.56
en-AU EUR1,234.56
en-NZ £1,234.56
en-NZ US$1,234.56
en-NZ €1,234.56
fr-CH 1 234.56 £GB 
fr-CH 1 234.56 $US 
fr-CH 1 234.56 € 
de-CH £ 1’234.56
de-CH $ 1’234.56
de-CH EUR 1’234.56
fr 1 234,56 £GB
fr 1 234,56 $US
fr 1 234,56 €
de 1.234,56 £
de 1.234,56 $
de 1.234,56 €

Comment 5 by js...@chromium.org, Dec 5 2017

Labels: -Needs-Triage-M64
We need this in M64 for sure. 

Comment 6 by js...@chromium.org, Dec 5 2017

With the upstream copy of ICU 60.1, I got this:

en £1,234.56
en $1,234.56
en €1,234.56
en-GB GBP1,234.56
en-GB USD1,234.56
en-GB EUR1,234.56
en-CA £1,234.56
en-CA US$1,234.56
en-CA €1,234.56
en-US £1,234.56
en-US $1,234.56
en-US €1,234.56
en-AU GBP1,234.56
en-AU USD1,234.56
en-AU EUR1,234.56
en-NZ £1,234.56
en-NZ US$1,234.56
en-NZ €1,234.56
fr-CH 1 234.56 £GB 
fr-CH 1 234.56 $US 
fr-CH 1 234.56 € 
de-CH £ 1’234.56
de-CH $ 1’234.56
de-CH EUR 1’234.56
fr 1 234,56 £GB
fr 1 234,56 $US
fr 1 234,56 €
de 1.234,56 £
de 1.234,56 $
de 1.234,56 €

en-GB is fine, but en-AU seems to be wrong.  de-CH for Europe is the same. 

Comment 7 by js...@chromium.org, Dec 5 2017

> de-CH EUR 1’234.56

This is per CLDR data. In de-CH, the narrow currency display format is supposed to use 'EUR'.  

en-GB and en-AU still under investigation

Comment 8 by js...@chromium.org, Dec 6 2017

Summary: toLocaleString / Intl.NumberFormat outputs currency code instead of symbol in en-GB locale (was: toLocaleString / Intl.NumberFormat outputs currency code instead of symbol)
en-AU is fine. CLDR has 3-letter ISO codes instead of symbols in en-AU. The only exception is "$" for AUD. 

So, the only remaining (so far known to us) issue is en-GB as reported here. 

Comment 9 by js...@chromium.org, Dec 6 2017

Found the cause. When trimming the locale data, curr/en_GB.txt ends up having an empty currency list as shown below:

en_GB{
    %%Parent{"en_001"}
    Currencies{
    }
    CurrencyPlurals{
        MKD{
            other{"Macedonian denari"}
        }
        XOF{
            other{"West African CFA francs"}
        }
    }
    Version{"2.1.37.22"}
}

----------------------

ICU's locale fallback chain is not triggered for 'empty' list (no entry at all). If there's at least one entry in the list, the value for missing entries would be obtained by climbing up fallback ladder (en_GB => en_001 => root). 

With an empty list, all the values become 'last resort' value (for currency symbols, they'd be ISO 3-letter currency code). 

I stumbled upon this issue before, but forgot about it. Need to figure out a guard against this. 


In third_party/icu/source/data/curr :

Running the following find 54 locales affected (53 of them are locale variants added recently). Only major one is en-GB.

for l in *txt; do    grep -A 1 'Currencies{' $l | tail -1 | grep '}' && echo $l; done


Labels: OS-Android OS-Chrome OS-Fuchsia
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/deps/icu.git/+/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f

commit e3b480d3be4446ea17011c0cdc9c4cd380a5c58f
Author: Jungshik Shin <jshin@chromium.org>
Date: Fri Dec 08 18:18:17 2017

Fix trim_data.sh to delete empty currency blocks

After data trimming, data/curr/en_GB.txt has empty "Currencies" block.
This prevents a locale fallback chain from being triggered for currency
symbols (en-GB => en-001 => en), leading all the currency symbols in en-GB
to be the last resort value (ISO 3-letter currency code). To avoid that,
trim_data.sh is revised to drop empty "Currencies" block entirely ('Missing'
triggers a fallback chain, but 'empty' goes straight to the last resort.)

Additional currency data fixes:

1. Serbian: RON and ROL display names are swapped. RON is the current
currency and should be included with a shorter display name.

2. Add a few more en-* locale variants to the locale list for currency.

Moreover, make_data_all.sh is updated to work wherever the Chromium
source tree is.

Finally, update the list of resources to remove from cast's ICU data
and rebuild it with ICU 60.  Added entries to cast-removed-resources.txt
are data for locale-variants that are not useful in Cast.

Bug:  791318 
Test: See the bug
Change-Id: Ib09ee36e3447557636d60d8aeabc7ca047718236
Reviewed-on: https://chromium-review.googlesource.com/810099
Reviewed-by: Mark Mentovai <mark@chromium.org>

[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/README.chromium
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/android/icudtl.dat
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/cast/cast-removed-resources.txt
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/cast/icudtl.dat
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/common/icudtb.dat
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/common/icudtl.dat
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/ios/icudtl.dat
[add] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/patches/curr_sr.patch
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/scripts/make_data_all.sh
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/scripts/trim_data.sh
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/source/data/curr/reslocal.mk
[modify] https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/source/data/curr/sr.txt

Project Member

Comment 14 by bugdroid1@chromium.org, Dec 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3dee456f0872993c86975aaaef0139b75e5594c5

commit 3dee456f0872993c86975aaaef0139b75e5594c5
Author: Jungshik Shin <jshin@chromium.org>
Date: Fri Dec 08 20:14:00 2017

Roll ICU to e3b480d3

Only one commit is included:

 https://chromium.googlesource.com/chromium/deps/icu.git/+log/26f7d8..e3b480d

 Fix trim_data.sh to delete empty currency blocks

TBR=mark@chromium.org

Bug:  791318 
Test: See the bug
Change-Id: I47b81c4e66d71f807f32100e669bd1228ff7641b
Reviewed-on: https://chromium-review.googlesource.com/817522
Reviewed-by: Jungshik Shin <jungshik@google.com>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Jungshik Shin <jshin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522856}
[modify] https://crrev.com/3dee456f0872993c86975aaaef0139b75e5594c5/DEPS

Status: Fixed (was: Started)
Will ask for merge to M64 once a canary build is out. 

And, I'll also add a test to v8. 

ICU in V8 6.4 branch will be rolled too when rolling ICU in Chrome 64's DEPS. 


Comment 17 by js...@chromium.org, Dec 12 2017

Labels: Merge-Request-64
Status: Verified (was: Fixed)
Verified in 65.0.3292.0 canary build

Requesting merge-approval to 64 branch. 

Project Member

Comment 18 by sheriffbot@chromium.org, Dec 12 2017

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 19 by js...@chromium.org, Dec 12 2017

Merging will be done by rolling ICU to e3b480d3 to bring in the CL recorded in comment 13. 

Note that the actual change is only in https://crrev.com/e3b480d3be4446ea17011c0cdc9c4cd380a5c58f/scripts/trim_data.sh . Others are the result of that change (generated data files). 

Labels: -Merge-Review-64 Merge-Approved-64
Approving merge to M64 Chrome OS.
Please merge this issue to M64 branch 3282 if it has been verified in canary. The sooner the better. Thanks!
Does this bug include string translation?

Comment 23 by js...@chromium.org, Dec 16 2017

Sorry for missing it. This change does not include string changes. 
I'm merging it now. 
Project Member

Comment 24 by bugdroid1@chromium.org, Dec 19 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/buildspec/+/a12f92dfbace0f6f7f2903990c8fcf0fec6b5d80

commit a12f92dfbace0f6f7f2903990c8fcf0fec6b5d80
Author: Jungshik Shin <jungshik@google.com>
Date: Tue Dec 19 21:42:23 2017

Was CL merged to M64 branch?  Anything pending here?
Labels: -Merge-Approved-64 merge-merged-3282
Sorry that I forgot to add merge-merged-32828

Sign in to add a comment