1005805 - [10n] [Dialer] Phone tags not localized

Status: VERIFIED FIXED

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

Description

[Juan Perez-Bedmar [:juanpbf]]

Attachment: Screenshot from the dialer

When opening the dialer and typing a number, the dialer offers you possible matches within your phonebook. The tags that shows the "type" of your contact's phone matching what you're typing is not localized (at least in Spanish language).

STR:
0) Preload several contacts in your phonebook
1) Set the handset's general language to Spanish
2) Open the dialer, and start typing a phone number
3) when the dialer offers possible matches, check the tag describing the phone "type)

CURRENT BEHAVIOUR:
The tags are not localized


EXPECTED BEHAVIOUR:
Those tags must be localized


ADDITIONAL INFO:
Some pictures added for clarification sake

Reproduced on latest build for Open II:

RIL commit:
40328af5e4c6ace39d56a6749f2fe34bf80272f8
Author: Linux Build Service Account <lnxbuild@localhost>
Date:   Mon Mar 31 20:00:59 2014 -0700
Merge "Initialize nai in struct Item to 0"
QC RIL TAG version: AU_LINUX_GECKO_B2G_JB_3.2.01.03.00.112.289

gaia commit:
26fcc03f01a3376c41c41dcaef787115d4ba372c
Author: Bob Silverberg <[e-mail]>
Date:   Fri Apr 4 12:42:26 2014 -0400
Bug 991659 - [v1.3] Backport Music tests to v1.3

gecko commit:
9962a52eb9b1e7e9dec0ca4468c3acc664ca7c7a
Author: B2G Bumper Bot <[e-mail]>
Date:   Fri Apr 4 10:00:52 2014 -0700
Bumping manifests a=b2g-bump

Comment 1

[Francesco Lodolo [:flod]]

Not :l10n, moving to dialer. I can confirm the issue.

Comment 2

[Francesco Lodolo [:flod]]

First problem is that we don't have these strings in dialer.properties (but we're not even trying to localize them).
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/suggestion_bar.js#L239

If we add them, if would be the third replication after contacts.properties and sms.properties

Comment 3

[isabelrios]

This is happening in v1.3 buri build (05/04):
Gecko-72bc7d7
Gaia-667539f

Nominating for 1.3

Comment 4

[Francesco Lodolo [:flod]]

This would require new strings, 1.3 and 1.4 have been string frozen for a while.

Comment 5

[Axel Hecht [:Pike]]

I actually think it's a design issue in the contacts API more than anything else. IMHO, the contacts API should return localized contact entries, and not require every consumer of the API to do that.

Jonas, what'd be your take? Also, do we have other APIs suffering from the same problem?

Comment 6

[nulti-korisnik]

I want to confirm this on v1.3 on keon using Serbian language.

Gecko: 72bc7d7
Gaia: 667539f

Comment 7

[Jason Smith [:jsmith]]

Note - since we're string frozen, we'll probably need to remove the phone number type in the UX here for 1.3 & 1.4 & provide the real fix for 2.0 to include the type.

Vance - Can you find out if this is a cert blocker?

Comment 8

[isabelrios]

Since the call log has access those strings correctly and, when you call to the suggested number in the attention screen the tab is correctly translated, it seems that those strings are already localized and it is not necessary to create new ones. Please see attached screenshots.
In theory it would only be necessary that the code related to the suggestion bar could use them.

Ni German who worked a lot on the dialer app and may have a clue about how to solve this issue.

Comment 9

[isabelrios]

Attachment: Attention screen

Comment 10

[isabelrios]

Attachment: Call log

Comment 11

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 1005805.patch

Hi guys! Isabel is completely right. No locale files have to be updated to solve the issue. In fact, please find attached the patch which solves it.

BTW, the issue also reproduces in master and any other version (it was never implemented until now). Do you want me to prepare a pull request for master including the patch? The uplifting to any other version (1.4 and 1.3, I guess) would be straight forward.

Comment 12

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Per discussion with TEF, this is not a cert blocker.

Comment 13

[Francesco Lodolo [:flod]]

(In reply to Germán Toro del Valle from comment #11)
> Hi guys! Isabel is completely right. No locale files have to be updated to
> solve the issue. In fact, please find attached the patch which solves it.

And from which .properties file are you reading those strings?

Comment 14

[Jason Smith [:jsmith]]

Backlog per comment 12.

Comment 15

[Beatriz Rodríguez [:brg]]

(In reply to Germán Toro del Valle from comment #11)
> Do you want me to prepare a pull request for master
> including the patch? 
Yes please. Lets have a fix for the bug in master so future OS releases will avoid cert issues.

I guess Axel wanted to ni Jonas in commnet 5 to know his feedback as well. I add ni to him, please remove it if I am wrong.

Comment 16

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to Francesco Lodolo [:flod] from comment #13)
> (In reply to Germán Toro del Valle from comment #11)
> > Hi guys! Isabel is completely right. No locale files have to be updated to
> > solve the issue. In fact, please find attached the patch which solves it.
> 
> And from which .properties file are you reading those strings?

Although already chatted about it via IRC, The Communications app is kind of an special app which includes, so to call them, 3 sub-apps: the Call Log, the Contacts and the Dialer. And regarding locales, they share them, as far as I know. In this case, the Call Log is reusing the Contacts app locale.

Comment 17

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #13)
> (In reply to Germán Toro del Valle from comment #11)
> > Hi guys! Isabel is completely right. No locale files have to be updated to
> > solve the issue. In fact, please find attached the patch which solves it.
> 
> And from which .properties file are you reading those strings?

After talking with Germán, I tried the code and it works. I had no idea that we add apps sharing strings like this.

At this point it's probably worth renominating at least for 1.4, since it doesn't involve new strings.

Comment 18

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 18993.html

Comment 19

[Etienne Segonzac (:etienne)]

Comment on attachment 8418114 [details]
18993.html

The change looks good but we're going to need 2/3 small tests to make sure it works properly :)

* no type
* type with translation available
* type without any translation

Comment 20

[Germán Toro del Valle (:gtorodelvalle)]

Sure ;-) Including the proposed unit tests ;-)

Comment 21

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8418114 [details]
18993.html

Hi Etienne! As you'll see in the review, I have included unit tests for the custom and pre-defined phone types. The no type case is not posible right now. This means that we could avoid the checking of the existence of the type in the patch. I updated it accordingly ;-)

Comment 22

[Etienne Segonzac (:etienne)]

Comment on attachment 8418114 [details]
18993.html

I think we need a test where we stub MockMozL10n.get to return undefined and check that we're inserting the type directly (the "|| type" in your code)

Comment 23

[Germán Toro del Valle (:gtorodelvalle)]

Hi Etienne, in fact that's reason why I changed the type of a phone number from a predefined one to "my-custom-type" in https://github.com/mozilla-b2g/gaia/pull/18993/files#diff-daf6737947ffaf7d0a62bf6d704d89ddR57
It is checked afterwards in https://github.com/mozilla-b2g/gaia/pull/18993/files#diff-daf6737947ffaf7d0a62bf6d704d89ddR171 and https://github.com/mozilla-b2g/gaia/pull/18993/files#diff-daf6737947ffaf7d0a62bf6d704d89ddR238
This is the case where there exists a type (in this case a custom type) but that type cannot be internationalized. In fact, _('my-custom-type') is undefined which is the test case I think you want to be checked in your previous comment. Am I right? :)

Comment 24

[Germán Toro del Valle (:gtorodelvalle)]

Need-infoing Etienne regarding comment 23 ;)

Comment 25

[Germán Toro del Valle (:gtorodelvalle)]

Let's see if at the third time I managed to get it done :-S

Comment 26

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8418114 [details]
18993.html

Hi Etienne, re-asking for review after including the change you suggested in the PR? Please, let me know if I understood your request in comment 22 as explained in comment 23 ;-) Thanks!

Comment 27

[Etienne Segonzac (:etienne)]

(In reply to Germán Toro del Valle from comment #23)
> Hi Etienne, in fact that's reason why I changed the type of a phone number
> from a predefined one to "my-custom-type" in
> https://github.com/mozilla-b2g/gaia/pull/18993/files#diff-
> daf6737947ffaf7d0a62bf6d704d89ddR57
> It is checked afterwards in
> https://github.com/mozilla-b2g/gaia/pull/18993/files#diff-
> daf6737947ffaf7d0a62bf6d704d89ddR171 and
> https://github.com/mozilla-b2g/gaia/pull/18993/files#diff-
> daf6737947ffaf7d0a62bf6d704d89ddR238
> This is the case where there exists a type (in this case a custom type) but
> that type cannot be internationalized. In fact, _('my-custom-type') is
> undefined which is the test case I think you want to be checked in your
> previous comment. Am I right? :)

I think the mock l10n lib always send you the string back.
Otherwise your assert will be that telType.textContent is equal to undefined, since your assert calls |MockMozL10n.get('my-custom-type')|.

Like I said on github the assert should be that telType.textContent equals the 'my-custom-type' string directly.

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 8418114 [details]
18993.html

comments on github, sorry for the misunderstanding.

Comment 29

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8418114 [details]
18993.html

Hi! As you will see in the updated PR, I opted for a little more complex version of your proposed stub since we still need to return the localization property in some cases, these are: the predefined phone type tags ('mobile', 'home', etc.). Now for the 'my-custom-type' case we return undefined as you suggest ;-)

Comment 30

[Etienne Segonzac (:etienne)]

Comment on attachment 8418114 [details]
18993.html

r=me with nits on github

Comment 31

[Germán Toro del Valle (:gtorodelvalle)]

Merged in master: https://github.com/mozilla-b2g/gaia/commit/37fc8503029d50cf7b7c36bcf292f52ed2ad3c77

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Axel Hecht [:Pike] from comment #5)
> I actually think it's a design issue in the contacts API more than anything
> else. IMHO, the contacts API should return localized contact entries, and
> not require every consumer of the API to do that.

It'll add significantly more complexity in the API if localization happens in the API itself. Both as far as API surface goes and as far as what you'd do to use it. For example it would mean that in order to find a person's work address, you'd have to write locale-dependent code. Even if you never actually display the string "work address" on the screen.

So I think the API should stay as it is. Asking developers to localize the tag when they put it on the screen doesn't seem terribly different from asking them to localize any other part of the UI.

Comment 33

[nulti-korisnik]

Will this fix be used in "Messages" app because tags are not localized there too (view any message and check for the tags in header of the app). Should i open another bug report for it?

Comment 34

[Francesco Lodolo [:flod]]

(In reply to djura-san from comment #33)
> Will this fix be used in "Messages" app because tags are not localized there
> too (view any message and check for the tags in header of the app). Should i
> open another bug report for it?

Which version? Because I'm aware of bug 931119, at that's fixed on master.

Comment 35

[isabelrios]

Checked on master with today's (05/12) buri build:
Gecko-849b0f7
Gaia-faf0245

The tags are correctly localized both for the dialer and the sms apps for Spanish now.

Comment 36

[Noemí Freire (:noemi)]

Per Comment 35 marking this bug as VERIFIED

Comment 37

[nulti-korisnik]

(In reply to Francesco Lodolo [:flod] from comment #34)
> Which version? Because I'm aware of bug 931119, at that's fixed on master.

Yes, this is the one. I will wait for new build from geeksphone to see if it will change things. So far, the bug is still there.