1032502 - [B2G][2.0][Lockscreen] The phone label of an incoming call on the lockscreen is not translated.

Status: RESOLVED FIXED

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

Description

[Yeojin Chung [:YeojinC]]

Attachment: PhoneLableNotTranslated.png

Description:
When the user receives an incoming call on the lockscreen, the phone label is NOT translated in ALL language. 

Pre-requisite: Have an existing contact with name and phone number with the phone label as "Home".

Repro Steps:
1) Update a Flame to BuildID: 20140630000201.
2) Set the device to any language other than English.
3) Have the existing contact with 'Home' phone label call the testing device on a lockscreen.
4) Verify the phone label is translated properly.


Actual:
The string "Home" is not translated properly, and appears in English.

Expected:
The string "Home" is translated properly to the test language.

Note: 
All phone labels such as "Home", "Mobile", or "work" are NOT translated to ANY language. (Tested: Swahili, French, Italian, Korean, Japanese)

Environmental Variables:
Device: Flame 2.0
BuildID: 20140630000201
Gaia: c0c8ad187c0466285f2580531e09f8322996f561
Gecko: d4dc609bcc8a
Version: 32.0a2 (2.0) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Repro Frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/11960/
https://moztrap.mozilla.org/manage/case/11974/
See attached: screenshot, logcat

Comment 1

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20140630_NotTranslated.txt

This issue DOES reproduce on Flame 2.1 and Buri 2.0:

Flame 2.1

Environmental Variables:
Device: Flame Master
BuildID: 20140630040201
Gaia: de14e61098b742251b34f856e48649db8bed552c
Gecko: b6408c32a170
Version: 33.0a1 (Master) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Buri 2.0

Environmental Variables:
Device: Buri 2.0
Build ID: 20140630000201
Gaia: c0c8ad187c0466285f2580531e09f8322996f561
Gecko: d4dc609bcc8a
Version: 32.0a2 (2.0)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

The string "Home" is not translated properly, and appears in English.
----------------------------------------------

This issue does NOT reproduce on Flame 1.4:

Flame 1.4

Environmental Variables:
Device: Flame 1.4
BuildID: 20140630000201
Gaia: aa896d5db1b4929f3bf31a0f4bb7de50530222a8
Gecko: 8cba60bc12ef
Version: 30.0 (1.4) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

The string "Home" is translated properly to the test language.

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Nominating this 2.0? since this is a regression from 1.4

Phone labels were translated properly on 1.4 and now appear to be untranslated across multiple languages on 2.0.

Comment 3

[Doug Sherk (:drs) (inactive)]

This regressed when bug 990003 landed originally. Also, woot for blindly guessing that without any bisecting (I did verify this though).

Comment 4

[Doug Sherk (:drs) (inactive)]

Attachment: Fix phone type label not being localized on callscreen.

PR: https://github.com/mozilla-b2g/gaia/pull/21248

This is a pretty cookie-cutter refactor of l10n strings. Francisco, feel free to redirect to someone else. Thanks!

Comment 5

[Jason Smith [:jsmith]]

(In reply to Doug Sherk (:drs) from comment #4)
> Created attachment 8449079 [details] [diff] [review]
> Fix phone type label not being localized on callscreen.
> 
> PR: https://github.com/mozilla-b2g/gaia/pull/21248
> 
> This is a pretty cookie-cutter refactor of l10n strings. Francisco, feel
> free to redirect to someone else. Thanks!

Do we need this on 1.4 as well after the regressing patch gets uplifted? If so, then we switch this to a 1.4? nom.

Also - are the string changes seen in this patch implying this is a late-l10n bug or not?

Comment 6

[Doug Sherk (:drs) (inactive)]

(In reply to Jason Smith [:jsmith] from comment #5)
> Do we need this on 1.4 as well after the regressing patch gets uplifted? If
> so, then we switch this to a 1.4? nom.

Yes. That's a good point. The dependency tree is weird now since this bug both blocks and depends on bug 990003, so I'll leave it as-is for now.

> Also - are the string changes seen in this patch implying this is a
> late-l10n bug or not?

No, sorry, I should have clarified. Since it's a refactor, it's just moving around existing strings, so it shouldn't be late-l10n.

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 8449079 [details] [diff] [review]
Fix phone type label not being localized on callscreen.

Review of attachment 8449079 [details] [diff] [review]:
-----------------------------------------------------------------

Francisco's review will be enough for this one.
But I think "moving around existing strings" to a new file might qualify as late-l10n.

Comment 8

[Doug Sherk (:drs) (inactive)]

(In reply to Etienne Segonzac (:etienne) from comment #7)
> But I think "moving around existing strings" to a new file might qualify as
> late-l10n.

Jason, do you know if this is true or not?

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8449079 [details] [diff] [review]
Fix phone type label not being localized on callscreen.

Review of attachment 8449079 [details] [diff] [review]:
-----------------------------------------------------------------

Tested on the phone and working perfectly.

Thanks Dough!

Comment 10

[Doug Sherk (:drs) (inactive)]

Attachment: Fix phone

Comment 11

[Doug Sherk (:drs) (inactive)]

Attachment: Fix phone type label not being localized on callscreen.

OK, I got some more information on the l10n process from Francesco. See bug 1030546 comment 10. I've restructured this patch slightly based on his comments and I'm carrying r+. I also updated the PR.

Comment 12

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/2f2cc0a82d3b80c37efe4e89fb4e95055b598843

Comment 13

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8449597 [details] [diff] [review]
Fix phone type label not being localized on callscreen.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 990003
[User impact] if declined: The phone type label (e.g. "mobile", "home") won't be localized on the callscreen.
[Testing completed]: Francisco and I both tried this. I used a Flame, not sure how he tested it.
[Risk to taking this patch] (and alternatives if risky): Low, other than needing to deal with the strings being moved.
[String changes made]: Added phone_types.ini file which is a manifest for a refactor of some strings from the contacts app. This file is shared between the contacts, dialer, and emergency-call apps. Unfortunately, there's no way to structure this patch that I can think of that doesn't, at the very least, involve moving around strings.

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8449597 [details] [diff] [review]
Fix phone type label not being localized on callscreen.

See comment 12 as everything is identical. Francesco (:flod) tells me that the window for making string changes to v2.0 has already passed, so I'm not sure what the process would be for getting this landed on v1.4. There's no way to structure this such that it doesn't involve string changes.

Note that this will only need to be uplifted to v1.4 once bug 990003 is. It's not needed right now.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Doug Sherk (:drs) from comment #13)
> [String changes made]: Added phone_types.ini file which is a manifest for a
> refactor of some strings from the contacts app. This file is shared between
> the contacts, dialer, and emergency-call apps. Unfortunately, there's no way
> to structure this patch that I can think of that doesn't, at the very least,
> involve moving around strings.

Given that this patch touches strings (if only for a refactor), I'd like someone from l10n to comment on the impact of this change to their work before approving. 

flod/pike: This is a regression visible on the lockscreen (see screenshot in comment 0). Given that drs states that he doesn't see a way to fix this without moving strings around but has not otherwise changed the strings, can we have someone refactor the l10n repo or otherwise respond to this change without impacting all of the localizers? Is there another route that we can take to fix this in 2.0?

drs: I see that bug 990003 landed on 1.3T. Do we need this fix there as well?

Comment 16

[Francesco Lodolo [:flod]]

One clarification about moving around strings: a string moved from one file to another is just a new string. Tools won't move it, people need to translate the string in the new place, if they don't, it will show up in English as fallback.

So, moving a string is still breaking string freeze.

Landing this on 1.4 or 2.0 won't fix the callscreen, it will just break both callscreen and contacts for a lot of locales.

Is there any way to just load contacts.properties in callscreen on 2.0?

Comment 17

[Axel Hecht [:Pike]]

What flod said, I only see doing a build-time hack as a way out here.

Also, I still think this is a bug in the contacts API, it should at least optionally expose localized field labels. Didn't have much luck arguing with sicking last time, though. Can't find the bug right now.

Comment 18

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #17)
> Also, I still think this is a bug in the contacts API, it should at least
> optionally expose localized field labels. Didn't have much luck arguing with
> sicking last time, though. Can't find the bug right now.

https://bugzilla.mozilla.org/show_bug.cgi?id=1005805#c5

Comment 19

[Doug Sherk (:drs) (inactive)]

(In reply to Lawrence Mandel [:lmandel] from comment #15)
> drs: I see that bug 990003 landed on 1.3T. Do we need this fix there as well?

Yes, good point.

(In reply to Axel Hecht [:Pike] from comment #17)
> What flod said, I only see doing a build-time hack as a way out here.

I have no idea how to do that. Do you have any ideas or a rough outline on that? Also adding Yuren as he might know. This process will be needed for bug 1030546 as well.

Also, note that the contacts.properties file contains a lot of key with relatively generic names ("cancel", "ok", etc.), and I'm not sure what the effect would be of double-loading these keys from two sources.

Comment 20

[Axel Hecht [:Pike]]

My rough outline would be staring at yuren, with that "don't hurt the web" look in my eyes. In particular because we've probably rewritten significant parts of that at various stages between 1.3 and today.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Francesco Lodolo [:flod] from comment #16)
> One clarification about moving around strings: a string moved from one file
> to another is just a new string. Tools won't move it, people need to
> translate the string in the new place, if they don't, it will show up in
> English as fallback.

Trying to talk through this (always a bad idea in Bugzilla) so that I can understand better. I get your point about the string showing up to localizers (in the tool) as a new string. I have never seen an l10n tool with refactor/mapping magic. However, we have already translated the content. Is there a reason why this content can't be used? Can we assign the manual task of copying strings in the l10n tool to someone?

Comment 22

[Doug Sherk (:drs) (inactive)]

(In reply to Lawrence Mandel [:lmandel] from comment #21)
> Trying to talk through this (always a bad idea in Bugzilla) so that I can
> understand better. I get your point about the string showing up to
> localizers (in the tool) as a new string. I have never seen an l10n tool
> with refactor/mapping magic. However, we have already translated the
> content. Is there a reason why this content can't be used? Can we assign the
> manual task of copying strings in the l10n tool to someone?

Yeah, I was trying to understand this too. I think the reason that it's considered different is because they may actually be different when used in the new context. Since we're using them in different places than we were before, they might actually be translated differently.

Though I'm all but completely sure that's not the case here -- not only because this is a relatively recent regression -- but because the keys are mostly single words that I don't think ever change depending on context.

Based on what I know, I don't think there's any reasons that we couldn't just move the strings around, but I might just not be aware of them.

Comment 23

[Axel Hecht [:Pike]]

There's no such thing as "the l10n tool". It's like telling coders that you have a way to do in emacs. You'll just not get anywhere. Or closer, you can do it on bitbucket, but that leaves all the folks on github and sourceforge in the wilderness.

The reason why we can't use them at runtime is the gaia security model, which has different domains for different apps, and thus one app can't read the strings of another.

To give this another twist:

I have no idea how to do this in:

- mozilla.locamotion.org (pootle instance)
- transifex
- mozilla translator (some mozilla-specific tool written in java)
- (possibly even that old version of narro that zh-TW is using)

Each of those need their own way of dealing with it, and then we still have the few for which we could possibly script a source-based version.

I don't have resources to solve even one of those problems, let alone all.

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

pike, drs, and I spoke about options on irc.
- moving strings is out as there is no canonical source for string changes (each locale uses their own system and exports to Hg)
- manually copying the required files is out as the l10n pootle system will overwrite the changes
- looks like our best option is a build hack to grab the needed files (or strings) and package them in the lockscreen - drs already has a ni on yuren about this in comment 19

Comment 25

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: 0001-WIP.patch

(In reply to Francesco Lodolo [:flod] from comment #16)
> Is there any way to just load contacts.properties in callscreen on 2.0?

I still don't understand what kind of hack should we do for callscreen app, but tried to have a converation with code. This patch will insert |contacts/locales/locales.ini| to callscreen/index.html

is that what we want? or we need extract some translation from contacts.properties to phone_types.properties for ALL LANGUAGES? (sounds horrible)

Comment 26

[Théo Chevalier]

(In reply to Yuren [:yurenju] from comment #25)
> Created attachment 8450738 [details] [diff] [review]
> 0001-WIP.patch
> 
> (In reply to Francesco Lodolo [:flod] from comment #16)
> > Is there any way to just load contacts.properties in callscreen on 2.0?
> 
> I still don't understand what kind of hack should we do for callscreen app,
> but tried to have a converation with code. This patch will insert
> |contacts/locales/locales.ini| to callscreen/index.html
> 
> is that what we want? or we need extract some translation from
> contacts.properties to phone_types.properties for ALL LANGUAGES? (sounds
> horrible)

If it works and there's no conflicts between the keys, your patch is what we want, yes.
As Lawrence and Axel said, extracting translations is not an option.

Comment 27

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

noted, this patch wasn't tested well and I think the path of .ini is wrong, so please test it before request review.

Comment 28

[Francesco Lodolo [:flod]]

(In reply to Théo Chevalier [:tchevalier] from comment #26)
> If it works and there's no conflicts between the keys, your patch is what we
> want, yes.

call-screen doesn't have locales at all on 2.0, so we can't have conflicts
https://github.com/mozilla-b2g/gaia/tree/v2.0/apps/callscreen/locales

Comment 29

[Axel Hecht [:Pike]]

Attachment: use contacts.properties next all the stuff we do

Yuren's patch confused the hell out of me, 'cause I would have sworn that we can't load strings from other apps.

Just the callscreen merrily does already.

I've not even remotely tried to test this, and we should check that the files don't conflict.

But this could Just Work (TM).

PS: we should get the previous patch off the 1.4 radar, there's not apps/callscreen/index.html, and I only see 1.4 mentioned in this bug as "it worked there, but doesn't on 2.0"

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8449597 [details] [diff] [review]
Fix phone type label not being localized on callscreen.

2.0- and 1.4-

As discussed in this bug, we're pursuing a build hack that will allow for a fix without breaking string freeze. This patch will need to be modified to remove the string changes and work with the build hack.

Comment 31

[Doug Sherk (:drs) (inactive)]

(In reply to Yuren [:yurenju] from comment #25)
> I still don't understand what kind of hack should we do for callscreen app,
> but tried to have a converation with code. This patch will insert
> |contacts/locales/locales.ini| to callscreen/index.html
> 
> is that what we want? or we need extract some translation from
> contacts.properties to phone_types.properties for ALL LANGUAGES? (sounds
> horrible)

Yuren, I think there was a misunderstanding here. We actually do need to import all of the contacts keys and values from contacts.properties to the callscreen app (whether it's into phone_types.properties or elsewhere). I agree, it's horrible, but we don't really have a choice. I tried this patch and I don't think it works because it's importing only the locales.ini file, which contains paths relative to the ./apps/communications/contacts/ folder.

Comment 32

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8450822 [details] [diff] [review]
use contacts.properties next all the stuff we do

This doesn't seem that bad. I'd rather just do this.

Comment 33

[Doug Sherk (:drs) (inactive)]

(In reply to Doug Sherk (:drs) from comment #32)
> Comment on attachment 8450822 [details] [diff] [review]
> use contacts.properties next all the stuff we do
> 
> This doesn't seem that bad. I'd rather just do this.

Oh, and I tested this and it worked.

Comment 34

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

okay got it.

Comment 35

[Doug Sherk (:drs) (inactive)]

v2.0: https://github.com/mozilla-b2g/gaia/commit/6a2be886df047682d897d1c9c560009021c01951

This only ever had blocking 2.0+ status, so I'm asking for triaging to land on v1.4 and v1.3T.

Comment 36

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8450822 [details] [diff] [review]
use contacts.properties next all the stuff we do

drs - I think you want to request uplift for 1.3T and 1.4 on this patch. (Please correct the noms if this is wrong.) I'd like confirmation that the fix is good on 2.0 before approving for uplift to the other branches.

Comment 37

[Doug Sherk (:drs) (inactive)]

(In reply to Lawrence Mandel [:lmandel] from comment #36)
> drs - I think you want to request uplift for 1.3T and 1.4 on this patch.
> (Please correct the noms if this is wrong.) I'd like confirmation that the
> fix is good on 2.0 before approving for uplift to the other branches.

I just wanted it to be triaged with 1.3T+ status, so that when it stuck on 2.0 the sheriffs would uplift it.

Comment 38

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8450822 [details] [diff] [review]
use contacts.properties next all the stuff we do

The approval flags aren't necessary. There are no string changes in this patch, so it'll get automatic landing approval for both branches if given 1.3T+ status.

Comment 39

[Doug Sherk (:drs) (inactive)]

Also, I'll do the uplift myself when given permission, since this thread is really confusing for onlookers.

Comment 40

[Francesco Lodolo [:flod]]

(In reply to Doug Sherk (:drs) from comment #38)
> The approval flags aren't necessary. There are no string changes in this
> patch, so it'll get automatic landing approval for both branches if given
> 1.3T+ status.

FYI, those approval flags have absolutely nothing to do with l10n or strings.

Comment 41

[Doug Sherk (:drs) (inactive)]

(In reply to Francesco Lodolo [:flod] from comment #40)
> FYI, those approval flags have absolutely nothing to do with l10n or strings.

My understanding is that a bug with blocking status has automatic landing approval on that branch unless it has string changes or something else weird.

Comment 42

[Doug Sherk (:drs) (inactive)]

(In reply to Francesco Lodolo [:flod] from comment #40)
> FYI, those approval flags have absolutely nothing to do with l10n or strings.

My understanding is that a bug with blocking status has automatic landing approval on that branch unless it has string changes or something else weird. Also, the regressing bug that this is fixing has 1.3T+ status, so it makes more sense to follow suit with that.

Comment 43

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8450822 [details] [diff] [review]
use contacts.properties next all the stuff we do

Thanks to Lawrence for explaining this process to me on IRC. A 1.3T+ bug doesn't get permission to be landed on any other branches, but any other x+ bug gets permission to land on (x+i) (i.e. 1.4+ can land on 2.0+ automatically). This means that I have to ask for v1.4 approval.

Comment 44

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8450822 [details] [diff] [review]
use contacts.properties next all the stuff we do

As discussed in this bug, this fix is required to fix an l10n regression introduced in bug 990003. 1.4+

Comment 45

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8450822 [details] [diff] [review]
use contacts.properties next all the stuff we do

This already landed on v2.0, so this is basically a formality (it had 2.0+ status at one point so I took that as approval).

But as for 1.3T, I'm still waiting for triaging, so I'm requesting approval instead. Lawrence says he'll investigate this and get back to me (thanks!)

Comment 46

[Doug Sherk (:drs) (inactive)]

v1.4: https://github.com/mozilla-b2g/gaia/commit/393d72937727ad20e82b2ff7b13e3d7ff077a9f0

Comment 47

[bhavana bajaj [:bajaj]]

Comment on attachment 8450822 [details] [diff] [review]
use contacts.properties next all the stuff we do

Clearing the stale approval requests here. Discussed offline with drs and we confirm the problem still exists on 1.3T, but I  we decided elsewhere not to deal with it given the timeline.

Comment 48

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to bhavana bajaj [:bajaj] from comment #47)
> Comment on attachment 8450822 [details] [diff] [review]
> use contacts.properties next all the stuff we do
> 
> Clearing the stale approval requests here. Discussed offline with drs and we
> confirm the problem still exists on 1.3T, but I  we decided elsewhere not to
> deal with it given the timeline.

According to this I'm removing the nomination.