976663 - [B2G][l10n][Contacts][All languages] Number Label name of Suggested Contacts to merge is not translated

Status: VERIFIED FIXED

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

Description

[Peter Bylenga [:PBylenga]]

Attachment: Contact_Number_Type_Untranslated.png

Description:
The number label name (ie. Home, Work, Office, etc) of suggested contacts to merge are globally not translated.  This screen is accessed by tapping to the right of one of numbers.  Tapping the number itself will toggle it's selection.

Prerequisites:
Have a Contact setup with a phone number of 1234567890
Change device language to a non-English supported language

Repro Steps:
1) Update a Buri to BuildID: 20140225004004
2) Launch Contacts app
3) Create a new contact with identical information to the one previously setup
4) Tap done and on the Merge Contacts screen tap to the right of the suggested contact

Actual:
The number label name of the suggested duplicate remains in English

Expected:
The number label name of the suggested duplicate is localized to device language

v1.3 Environmental Variables:
Device: Buri v1.3 MOZ
BuildID: 20140225004004
Gaia: 6e883bde818d4d53aef7b5b075b4b34267918360
Gecko: e597280f9300
Version: 28.0
Firmware Version: v1.2-device.cfg


Notes:

Repro frequency: 5/5, 100%
See attached: screenshot

Comment 1

[Jason Smith [:jsmith]]

Does this reproduce on 1.2?

Comment 2

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

This issue occurs on the latest Buri v 1.2.0 Mozilla RIL

Environmental Variables
Device: Buri v 1.2.0 Mozilla RIL
Build ID: 20140220004002
Gecko: https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/2ea6a65eea23
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Platform Version: 26.0
Firmware Version: v1.2-device.cfg

The user can see the "Mobile" label in English.

Comment 3

[Delphine Lebédel [:delphine - Use needinfo]]

Ok so this is too late to make it into 1.3

Does this happen only when you save a contact in English and then switch to another locale?
Or does it also appear untranslated when you've added a contact in another locale than English?

Comment 4

[Peter Bylenga [:PBylenga]]

I was adding the contacts while in Polish during my initial testing and the number type was showing up in English on that specific screen.

Comment 5

[Delphine Lebédel [:delphine - Use needinfo]]

ok thanks Peter

I think the string is in apps/communications/contacts/contacts.properties
I do see it localized for many locales in 1.3 though, so I don't know why it's not appearing as so on the phone

Flod, can you help out with this please?

Comment 6

[Peter Bylenga [:PBylenga]]

Just to be clear, it is localized in multiple sections throughout the phone, so far it's just this screen that I found that it isn't localized on.  For example, the proper localized string is shown when creating/editing a contact.

Comment 7

[Francesco Lodolo [:flod]]

No l10n support in that function, just concatenates type + ',' + value
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts_matching_ui.js#L358

Comment 8

[Francesco Lodolo [:flod]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16686/files

Tested on my Keon and it definitely works (also passes unit tests locally), not sure if I'm overlooking anything.

Comment 9

[Fabien Cazenave [:kaze]]

(In reply to Francesco Lodolo [:flod] from comment #7)
> just concatenates type + ',' + value

This makes me nervous. ;-) (keeping bug 968853 on my radar…)

I agree this is a very simple case, but are we 100% sure this will work for all locales? I thought that the comma itself (,) was different in some locales like Arabic, Chinese, Japanese…

I’ll trust your opinion on that. I just wanted to say that since this is not a 1.3+ blocker (not even a blocker at all), we can take the time to fix this properly and add a few l10n strings like:

  number-personal = personal, {{ number }}
  number-work     = work, {{ number }}

</my2¢>

Comment 10

[Francesco Lodolo [:flod]]

(In reply to Fabien Cazenave [:kaze] from comment #9)
> (In reply to Francesco Lodolo [:flod] from comment #7)
> > just concatenates type + ',' + value
> 
> This makes me nervous. ;-) (keeping bug 968853 on my radar…)

You beat me by minutes ;-)

After bug 975637 I realized I need to fix this too. Clearing review and doing a proper fix.

P.S. it doesn't work well with Italian either: I would definitely use ":", not ",", given a choice.

Comment 11

[Francesco Lodolo [:flod]]

Comment on attachment 8382771 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16686/files

Need a proper fix, clearing review for now.

Comment 12

[Fabien Cazenave [:kaze]]

Haha, thanks for CC’ing me in the first place. :-)

Another $.02 — if it’s just a matter of not adding new l10n strings, we could even use this trick:

  number-personal = {{ personal }}, {{ number }}
  number-work     = {{ work}}, {{ number }}

In both case, instead of:

  item.textContent = _(fieldValue.type) + ', ' + fieldValue.value;

we would use:

  navigator.mozL10n.localize(item, 'number-' + fieldValue.type, { number: fieldValue.value });

Comment 13

[Francesco Lodolo [:flod]]

Attachment: Updated PR

Kaze, what about this approach? Only one new string and no concatenation.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8382819 [details] [review]
Updated PR

Redirecting to a contact peer, but looking good :)

Comment 15

[Francesco Lodolo [:flod]]

Requesting 1.4?: no new strings added, improve localizability even if it's a pretty hidden UI (but QA managed to find it already).

Comment 16

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

Comment on attachment 8382819 [details] [review]
Updated PR

Tried on the phone and looking perfect.

Could you add a unit test before we land it?

Comment 17

[Francesco Lodolo [:flod]]

(In reply to Francisco Jordano [:arcturus] from comment #16)
> Could you add a unit test before we land it?

To be honest I wouldn't know where to start. So far I only managed to fix existing tests, and I don't see any for this part of the UI.

Comment 18

[Francesco Lodolo [:flod]]

Didn't noticed the failing tests, trying to look into them now.

Comment 19

[Francesco Lodolo [:flod]]

I fixed the existing tests and we should have some coverage now (hope that's enough)
https://travis-ci.org/mozilla-b2g/gaia/builds/19743458

Comment 20

[Fabien Cazenave [:kaze]]

Comment on attachment 8382819 [details] [review]
Updated PR

One string to fix them all. Nice! :-)

Nitpick: I’d prefer to use mozL10n.localize() rather than mozL10n.get on HTML elements — see my detailed comment on your PR.

Comment 21

[Noemí Freire (:noemi)]

Triage: localization issue so it should be fixed.

Comment 22

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

Comment on attachment 8382819 [details] [review]
Updated PR

Working like charm now :)

Thanks!

Comment 23

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

@Francesco, could you rebase the patch?

I tried it on the phone, but now I'm not able to merge cause there are conflicts.

Merge once the conflicts are resolved unless the changes are huge.

Cheers!

Comment 24

[Francesco Lodolo [:flod]]

Rebased, there was a conflict in the .properties file (new label added right in the same place of this one).

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Master: 8f37a3795a07cfd5399c24d40a30ab92763cde0f

Comment 26

[Francesco Lodolo [:flod]]

Verified on Keon with build 20140307, setting phone to it.