936386 - [Contacts] consider adding "phonetic-name-field" for Japanese locale

Status: RESOLVED WONTFIX

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

Description

[Teiichiro Fukuda]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Steps to reproduce:

The Contacts app(iOS and Android) that is used in Japan, there is a text field for entering the "phonetic-name-field", but it does not have a "phonetic-name-field" in the Contacts app of FirefoxOS.
Since there is more than one pronunciation in Japanese kanji, pronunciation name is required.
Please add the text field to input "Phonetic-name".

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Need info to the UX team.  Should we include a "phonetic-name" field as described in comment 0 for the Japanese locale to match the feature provided by other mobile platforms?

Comment 2

[Stephany Wilkes]

I am flagging Joe Cheng, who is lead on Comms, to see if this is something they've discussed for Contacts. Also flagging Jacqueline to advise on UX. 

Please note that UX may be slow to respond as this is not a 1.2 blocker and we are clearing 1.2 and 1.3 blockers first, in that order. Thanks for your understanding!

Comment 3

[Teiichiro Fukuda]

Attachment: screenshot-contacts app edit screen(iOS and Android).pdf

I will attach a edit screen of the Contacts app(iOS and Android).

Comment 4

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

This will require either:

- A change in the API.
- Move Contacts to use DataStore so we can extend the data stored right now in API.

As well that the changes needed in the UX.

Cheers,
F.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Francisco Jordano [:arcturus] from comment #4)
> - A change in the API.
> - Move Contacts to use DataStore so we can extend the data stored right now
> in API.

I think we would want to change the API so that all third party apps could access the field as well.  I know Datastore allows cross-app access, but this is a core data element need, not an optimized index or something.

Need info to Tantek about adding a phonetic-name field to the contacts API.

Comment 6

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

(In reply to Ben Kelly [:bkelly] from comment #5)
 
> I think we would want to change the API so that all third party apps could
> access the field as well.  I know Datastore allows cross-app access, but
> this is a core data element need, not an optimized index or something.
> 
> Need info to Tantek about adding a phonetic-name field to the contacts API.

Right, was suggesting the DataStore solution as we have been pointed out to it each time that we want to change the contacts API. (Adding JulienW to the bug)

IMHO, I would put it on the API as well, but it's not my call, also I can see this field being totally compatible to make the API standard :)

Cheers!
F.

Comment 7

[Teiichiro Fukuda]

"phonetic-name-field" of Contacts API is registered in Bugzilla[1].

[1]：https://bugzilla.mozilla.org/show_bug.cgi?id=909224

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

flaging 1.4? to be discussed in comms meeting

Comment 9

[Teiichiro Fukuda]

Attachment: phonetic-name-field UX image.png

Because comment was not updated, I made UX image.
I am glad when I have you refer to it.

Comment 10

[Joe Cheng [:jcheng] (please needinfo)]

Wayne, is this needed for Madai? part of partner contribution? thanks

Comment 11

[Wayne Chang [:wchang]]

Joe, yes to both questions. :)

Comment 12

[Wayne Chang [:wchang]]

Joe, yes to both questions. :)

Comment 13

[Teiichiro Fukuda]

Attachment: Bug 936386 UI.pdf

Please do a review of the user interface.

Comment 14

[Wayne Chang [:wchang]]

Comment on attachment 8390318 [details]
Bug 936386 UI.pdf

tag carrie, you're it!

Comment 15

[Carrie Wang [New account: [e-mail]]]

Comment on attachment 8390318 [details]
Bug 936386 UI.pdf

Hi, 
the spec looks fine to me, but if we add this phonetic name in Contact, will it be displayed on incoming call pages or Message APP? Thanks!

Comment 16

[Teiichiro Fukuda]

Hi Carrie,

The phonetic name is not displayed in incoming call page and message APP.
In Japan, a phonetic name is generally displayed only by Contacts APP.

Thanks.

Comment 17

[Teiichiro Fukuda]

Attachment: bug936386.patch

Please review a patch.

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8406041 [details] [diff] [review]
bug936386.patch

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

Redirecting to Francisco for review.

Comment 19

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

Comment on attachment 8406041 [details] [diff] [review]
bug936386.patch

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

Hi,

Excellent job here!

Made some comments to the files, basically the overall comment will be:

- Separate the phonetic checks and helpers in a different module to be reused from details and form.
- Try to create something more generic, not just for the JA locale.
- Avoid as much as possible doing ops when this functionality is not needed

Thanks a lot for the hard work, happy to review the next iteration :)

::: apps/communications/contacts/js/contacts.js
@@ +35,4 @@
>      navigation.go('view-contact-form', 'popup');
>    };
>  
> +  const PHONETIC_LANG = 'ja';

This looks to me a bit specific, would like a patch that open the doors to any other language that could use this phonetic feature :), even we could offer it as a setting (in the future)

@@ +40,2 @@
>    var contactTag,
> +      phoneticGivenName,

unused variable

@@ +40,3 @@
>    var contactTag,
> +      phoneticGivenName,
> +      phoneticFamilyName,

same here

@@ +867,5 @@
> +    if (document.documentElement.lang === navigator.mozL10n.language.code)
> +      return;
> +
> +    if (isJapaneseLang()) {
> +      contacts.Form.showPhoneticInputArea();

With this method in the |contacts.js| file we are coupling this class to the details and form one.

Right now we are working on a patch to separate contacts in different documents, with this code we are adding extra dependencies (now we have them, but we are trying to remove).

Instead of |contacts.js| being in charge of telling the details and form to show or hide, that should be coded in a separate helper and used independently in the places needed.

Take into account that with web activities we will be opening just a file called details.html that will include details.js and not contacts.js

@@ +978,5 @@
>      appTitleElement.textContent = _('SelectedTxt', {n: count});
>    };
>  
> +  var isJapaneseLang = function c_isJapaneseLang() {
> +    return navigator.mozL10n.language.code === PHONETIC_LANG;

Too specific for being here. A helper to detect specific needs will be useful for me.

@@ +1010,4 @@
>      'view': loadView,
>      'utility': loadUtility,
>      'updateSelectCountTitle': updateSelectCountTitle,
> +    'isJapaneseLang': isJapaneseLang,

If we move this to a helper we won't need to expose this and have the dependency.

::: apps/communications/contacts/js/views/details.js
@@ +197,5 @@
>  
>      if (hasName(contact)) {
> +      var givenName = (contact.givenName && contact.givenName[0]) || '';
> +      var familyName = (contact.familyName && contact.familyName[0]) || '';
> +      var cjk = contacts.List.isCJK(String(familyName));

Again we are adding an extra dependency with the |list.js| file.

Also we should encapsulate this special case in a common helper that later we could modify, independently, to add any extra possible cases by locale.

As well, we are doing extra operations (calculating givenName and familyName) always when in some cases we won't need it.

@@ +199,5 @@
> +      var givenName = (contact.givenName && contact.givenName[0]) || '';
> +      var familyName = (contact.familyName && contact.familyName[0]) || '';
> +      var cjk = contacts.List.isCJK(String(familyName));
> +      if (cjk) {
> +        name = familyName + ' ' + givenName;

Even in this case, is better to not to concatenate this, and use mozL10n, to find a format to put names together.

@@ +235,4 @@
>      contactDetails.classList.remove('up');
>      utils.dom.removeChildNodes(listContainer);
>  
> +    if (Contacts.isJapaneseLang()) {

As commented before, instead of having in contacts something like |isJapaneseLang| we could have a helper that gives us something like |shouldShowPhoneticInfo| (or something like that ;)), and internally keep a list of locales that should display the phonetic stuff.

(again, and in the future add extra functionality to do this by locale, setting, building options, whatever, but changing a dedicated and smaller shared file will be better)

@@ +264,5 @@
> +  var renderPhoneticName = function cd_renderPhoneticName(contact) {
> +    var phoneticNameText = '';
> +
> +    if (contact.phoneticFamilyName && contact.phoneticFamilyName.length > 0 &&
> +        contact.phoneticFamilyName[0] != '') {

perhaps is good idea to perform a trim, and avoid values like '         '

@@ +269,5 @@
> +      phoneticNameText = contact.phoneticFamilyName[0] + ' ';
> +    }
> +    if (contact.phoneticGivenName && contact.phoneticGivenName.length > 0 &&
> +        contact.phoneticGivenName[0] != '') {
> +      phoneticNameText += contact.phoneticGivenName[0];

Instead of concatenation with a specific format, above, using mozL10n with a format that could change by locale will be better.

@@ +772,4 @@
>      'render': render,
>      'onLineChanged': checkOnline,
>      'reMark': reMark,
> +    'showPhoneticArea': showPhoneticArea,

We won't need to expose this methods if we have the magic helper :)

::: apps/communications/contacts/js/views/form.js
@@ +1346,5 @@
>      'render': render,
>      'insertField': insertField,
>      'saveContact': saveContact,
>      'onNewFieldClicked': onNewFieldClicked,
> +    'showPhoneticInputArea': showPhoneticInputArea,

As commented before, we would like to remove this exposure to minimise coupling

::: apps/communications/contacts/js/views/list.js
@@ +550,4 @@
>    var getSearchString = function getSearchString(contact, display) {
>      display = display || contact;
>      var searchInfo = [];
> +    var searchable = [];

The search string calculation is a bit of a pain in the ass right now.

It does reduce the performance of the contacts list, since we need to calculate it for each field.

Any extra calculation done is painful for us and will affect the performance.

With that said, we will still need the change, so let's see what we can do.

@@ +552,5 @@
>      var searchInfo = [];
> +    var searchable = [];
> +    var familyName = (display.familyName && display.familyName[0]) || '';
> +    var cjk = isCJK(String(familyName));
> +    if (cjk) {

Perhaps we could leave the code as it was and in a special case add the extra elements to the |searchable| array.

Also the |isCJK| function is not related to having phonetics available or not, is just related to string, this will affect searches of people that are not using those fields. For example, my Japanese friends from Facebook, will have characters like that, but in my current locale I don't have to display them or take into account those fields.

@@ +596,5 @@
>        return fs;
>      }
>  
> +    var cjk = isCJK(String(familyName));
> +    if (orderByLastName && cjk) {

Again, pretty specific for the Japanese locale, we should try to have a mechanism that is more generic.

@@ +613,4 @@
>      return ele;
>    }
>  
> +  function isCJK(name) {

To specific to be here

@@ +626,5 @@
> +    return true;
> +  }
> +
> +  function isKanji(c) {
> +    var unicode = c.charCodeAt(0);

Based on string search instead of locale detection won't help.

We will trigger a lot of code that in a locale different than needed will be executed.

@@ +639,5 @@
> +    return false;
> +  }
> +
> +  function isKana(c) {
> +    var unicode = c.charCodeAt(0);

Same comment that above.

@@ +1895,4 @@
>      'getHighlightedName': getHighlightedName,
>      'selectFromList': selectFromList,
>      'exitSelectMode': exitSelectMode,
> +    'isCJK': isCJK,

We shouldn't offer this as a possible dependency.

Comment 20

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

Comment on attachment 8406041 [details] [diff] [review]
bug936386.patch

Asking Ben for feedback, since we are adding extra operations to the search field and that could impact list performance.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8406041 [details] [diff] [review]
bug936386.patch

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

Thanks for working on this.  I think its a good start, but as Francisco notes, still needs a bit more work.

So, I'd like to see some load time measurements before this ultimately lands, but for now I am not too concerned with adding a couple more fields to the search string array.  As I mention below, I think most of the pain we pay for generating the search strings is in the normalization step.

I'm giving f-, though, because this does not update the |node.dataset.order| string to match the potentially different phonetic sort order.  If you don't update this then adding a new contact to an already loaded list will result in incorrect sorting.

Also, have we thought about how the app should work if we switch the users lang to and from japanese while the contacts app is loaded?  Do we suddenly want to sort differently or display differently?  While this is not a frequent use case for real users, I think its something QA likes to do during validation.

Thanks again.

::: apps/communications/contacts/js/views/list.js
@@ +550,4 @@
>    var getSearchString = function getSearchString(contact, display) {
>      display = display || contact;
>      var searchInfo = [];
> +    var searchable = [];

Based on my past profiling I think most of the cost of |getSearchString| comes in the |Normalizer.toAscii()| and |Normalizer.escapeHTML()| calls at the end.  I would not expect pushing a few more contact fields to have that much impact; especially if they are normally empty.

Ultimately we need to measure to know for sure.

@@ +552,5 @@
>      var searchInfo = [];
> +    var searchable = [];
> +    var familyName = (display.familyName && display.familyName[0]) || '';
> +    var cjk = isCJK(String(familyName));
> +    if (cjk) {

I think we will pay a bigger price from calling |isCJK()| vs. just adding the the phonetic fields to the array for all contacts.  This will just result in a few more spaces being joined into the final string to be normalized.

In comparison, when actually using phonetic names you will have to inspect every character of every name for each contact.  That seems non-scalable to me.

Alternatively, maybe |isCJK()| should just look at the first character in the name.

Comment 22

[Teiichiro Fukuda]

Hi, Ben. Sorry for my late reply.

Can I discuss the following two topics on the other Bug?

A) possible use of phonetic feature on any other language
B) separate helper to show or hide the phonetic fields

If you allow to me, I will register the Bug for further discussion later.
Please stand by.
And, I will consider corresponds for V2.1.

Regarding inspecting only with the first character of a name.
Inspecting every character of every name for each contact is not a process specifically prepared for Japanese.
It is used on English, and quite possibly for any other language as well, I suppose.
I see no reason to customize inspection process specifically for Japanese language.
I recommend no change and want to leave the existing implementations as they are.

Thanks in advance,
Teiichiro

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I'm not sure what you're asking me.  We can continue to look at this in another bug if you prefer.  What is that bug number?  Or are you saying you want to land the patch in attachment 8406041 [details] [diff] [review]?

Sorry for my confusion.

Comment 24

[Teiichiro Fukuda]

Hi, Ben. Thank's for your reply.
Sorry to trouble you but please forget Comment 22 and proceed with the followings.

There are four indications for the patch in attachment 8406041 [details] [diff] [review].

A) separate the phonetic checks and helpers in a different module to be reused from details and form.
B) possible use of phonetic feature on any other language
C) separate helper to "show or hide" the phonetic fields
D) inspectig only with the first character of a name

For (A), see Bug 1020773
We are making changes as we speak. Expect to be notified within a few days.
For (B), We are going to modify to V2.1 or later for this issue.
For (C), We are going to modify to V2.1 or later for this issue.
For (D), as I mentioned above, I see no reason to customize inspection process specifically for Japanese language.

Regarding inspecting only with the first character of a name.
Inspecting every character of every name for each contact is not a process specifically prepared for Japanese.
It is used on English, and quite possibly for any other language as well, I suppose.
I see no reason to customize inspection process specifically for Japanese language.
I recommend no change and want to leave the existing implementations as they are.

Comment 25

[BMO Automation]

Firefox OS is not being worked on