1113023 - [l10n] String identifiers displayed for plural forms instead of strings (zh-TW and other locales with plural rule #0)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect)

Description

[Shing Lyu [:shinglyu]]

Attachment: screenshot

*** Description
Notification panel title and update notification titles are wrong in Traditional Chinese locale

*** Steps to Reproduce
1. Switch to Traditional Chinese locale (正體中文(繁體))
2. Pull down the notification panel
3. Observe the title and the update notification (if any update is available)

*** Expected Results
Both the title and the update notification is in Chinese and they makes sense.

*** Actual Results
The title is "statusbarNotifications". The update notification is "updateAvailableInfo.innerHTML" (see the screenshot)

*** Other Notes

*** Reproduction Frequency: 
Always

*** Build
Gaia-Rev        d22dfece04fc00457e8369c660c11f945b088d2f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/cb8ad2251c09
Build-ID        20141217160203
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  40
FW-Date         Tue Oct 21 15:59:42 CST 2014
Bootloader      L1TC10011880

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

-> Mozilla Localization/zh-TW

Comment 2

[Peter Pin-Guang Chen [:petercpg] (MozTW.org)]

Confirmed, but I can't figure why texts were not displayed but entities. 
Spotted another string apps/system/system.properties:numberOfUpdates not displaying translations, too. 

Bug 1113026 is probably with same problem. Any ideas, Flod?

Comment 4

[Francesco Lodolo [:flod]]

I'm using the Calendar bug steps from bug 1113026 because they're easier to test: I can definitely reproduce with zh-TW, while it works just fine with 'it' (tried also 'pl', 'ta', 'xh' to be sure).

It also reproduces with zh-CN and ja, I wonder if it's related to the time format somehow.

I checked zh-TW.json on the device, compared "minutes-before" record with the one available in it.json, but they look absolutely similar.

Let's move this to system.

Comment 5

[Francesco Lodolo [:flod]]

Gandalf, are you aware of any change in l10n.js that could have caused this?
I wonder if we should actually move this back to Gaia::L10n

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

This seems serious. I can reproduce it. It seems that for some reason compiler is unable to compile that entity in zh-TW and fails to return a string.

Easiest way to reproduce:

1) Build gaia with zh-TW
2) Launch WebIDE
3) Select Calendar
4) Type: navigator.mozL10n.get('minutes-before', {'value': "5"});

I'd say it should block 2.2

Stas, any idea what may be going on in resolver?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, so it's a regression from bug 940772. Basically we expect [0] that for locales that don't use plural forms, we will not pass arguments.

[0] https://github.com/mozilla-b2g/gaia/blob/bdedbaf9f18a43c091ede770407d68d38582fe29/shared/js/l10n.js#L919-L924

If I understand correctly, the regression landed only after 2.1 branched, so we only have to fix it on master.

The regression affects multiple languages and makes all entities that use plural forms not work in those locales.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

A separate question is why this class of bugs is silenced. It would be easier to debug them if they showed up in the log.

Comment 9

[Staś Małolepszy :stas]

I think that the proper fix would be for zh-TW to change

  months-before={[ plural(value) ]}

to

  months-before={[ plural() ]}

in

  https://hg.mozilla.org/gaia-l10n/zh-TW/file/0d6aeb485310/apps/calendar/calendar.properties#l141

That's not very realistic nor practical, I'm afraid.

A possible work-around could be to add 'n' to the definition of the 0th plural rule in

  https://github.com/mozilla-b2g/gaia/blob/bdedbaf9f18a43c091ede770407d68d38582fe29/shared/js/l10n.js#L1528-L1535

But that's hacky and not really solving any problems.

Zibi, should we remove that argument length check entirely?  At least for the time being while the plural macros are defined in l10n.js and not under the localizer's control.


(In reply to Zibi Braniecki [:gandalf] from comment #8)
> A separate question is why this class of bugs is silenced. It would be
> easier to debug them if they showed up in the log.

We forgot to add a case for 'resolveerror' here:

  https://github.com/mozilla-b2g/gaia/blob/bdedbaf9f18a43c091ede770407d68d38582fe29/shared/js/l10n.js#L1528-L1535

I'll add it in this bug.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas (vacation until Dec 22) from comment #9)
> Zibi, should we remove that argument length check entirely?  At least for
> the time being while the plural macros are defined in l10n.js and not under
> the localizer's control.

I would be in favor of that.

Comment 11

[Staś Małolepszy :stas]

Attachment: Pull request: remove the arity check for macros

Comment 12

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas (vacation until Dec 22) from comment #9)

> We forgot to add a case for 'resolveerror' here:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> bdedbaf9f18a43c091ede770407d68d38582fe29/shared/js/l10n.js#L1528-L1535
> 
> I'll add it in this bug.

I added it in the pull request but I discovered in the process that our runtime DEBUG builds don't currently flash: bug 1116162.

Comment 13

[Staś Małolepszy :stas]

Master: https://github.com/mozilla-b2g/gaia/commit/0ff2ea32f2952b81de27d8e050144f7551431879
L20n.js: https://github.com/l20n/l20n.js/commit/9dee8caaae713128ca71320268053ff246d4c131

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Staś Małolepszy :stas from comment #13)
> Master:
> https://github.com/mozilla-b2g/gaia/commit/
> 0ff2ea32f2952b81de27d8e050144f7551431879
> L20n.js:
> https://github.com/l20n/l20n.js/commit/
> 9dee8caaae713128ca71320268053ff246d4c131

Can we uplift this to 2.2 if ready?

Comment 15

[Francesco Lodolo [:flod]]

(In reply to bhavana bajaj [:bajaj] from comment #14)
> Can we uplift this to 2.2 if ready?

This landed at the end of 2014 (before branching), so it's already on 2.2

I guess the flag was set because we were in the middle of the Holidays, close to the branching date, and before finding the root issue.