914942 - [b2g][Contacts] Entries in Contacts App all appear under the "#" category for locales with non-Latin alphabets (and shouldn't)

Status: RESOLVED FIXED

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

Description

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

Issue confirmed on latest Leo v1.1 Moz RIL build
Gaia   946623575b3fbf6138fff5f77ddead3cadffacce
BuildID 20130910041240
Version 18.1

Repros on all v1.1 non-Latin alphabet shipping locales

Repro: 
* Switch to a non-Latin alphabet locale (Greek, Russian, Serbian Cyrillic,)
* Enter the Contacts App
* Enter a contact (just a name with number will be enough) and save it

Expected:
Contact should appear under the appropriate first letter of the name

Actual: 
All contacts appear under the # category

Seems to be related to bug 914558, since for non-latin alphabet locales, the vertical sorting list in Contacts App stays in Latin letters

Comment 1

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

Nominating for leo since this repros in v1.1 shipping locales

Comment 2

[Fabien Cazenave [:kaze]]

Attachment: link to pull request

https://github.com/mozilla-b2g/gaia/pull/12502

Comment 3

[Jose Manuel Cantera]

From a functional perspective I'm ok with the patch but from a perf perspective I would like Ben Kelly to check the patch, as we could be introducing perf regressions, particularly because we will be creating more letter groups.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Shouldn't the following line guarantee we are looking at an ascii character?

  https://github.com/fabi1cazenave/gaia/blob/d52f32def1758398852a6bab017dd45263a76c16/apps/communications/contacts/js/views/list.js#L1040

Why are we seeing non-ascii values out of Normalizer.toAscii()?  I guess my impression was that this would coerce characters to their closest ascii equivalent.  For example, I thought umlauts should up in group U.

Also, I don't think the scrollbar targets will work with these new groups, will they?  It seems we'll be able to jump to the old ascii groups, but not the new groups.

I'm not sure yet what the performance impact might be.  It would have been nice to make the groups static like the scrollbar.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

From IRC conversation with Kaze, it sounds like Normalizer.toAscii() only works for Roman scripts.  It seems to me the correct solution is to make it work with these other scripts as well.  Having extra groups for some scripts, but not others would seem confusing UX to me.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

From a performance perspective I think the main concern I have is with when the group DOM is getting created, not its size.  It always happens at load with the same content, but its done dynamically which causes an additional reflow.  So soon I am going to look at either making it static index.html or perhaps creating the group just-in-time when a contact needs to be added to it.

I don't think the number of groups matters too much from a perf perspective if the majority are empty.  There will be some penalty, but I expect it to be modest.  We would really have to measure to know for certain, though.

As I said in comment 5, though, I think using the Normalizer makes more sense from a functional perspective.

Comment 7

[Fabien Cazenave [:kaze]]

(In reply to Ben Kelly [:bkelly] from comment #6)
> From a performance perspective I think the main concern I have is with when
> the group DOM is getting created, not its size.  It always happens at load
> with the same content, but its done dynamically which causes an additional
> reflow.  So soon I am going to look at either making it static index.html or
> perhaps creating the group just-in-time when a contact needs to be added to
> it.

Creating the group just-in-time would make more sense, imho. I’m afraid making it static will cause a significant regression for the startup time (first paint).

Are you saying that you’ll write this part?

> I don't think the number of groups matters too much from a perf perspective
> if the majority are empty.  There will be some penalty, but I expect it to
> be modest.  We would really have to measure to know for certain, though.

FWIW, for 1.2 we won’t have to deal with more than these three scripts (Roman, Greek, Cyrillic).

> As I said in comment 5, though, I think using the Normalizer makes more
> sense from a functional perspective.

The Normalizer is used to deal with diacritics in Roman letters: that’s how we can sort “Étienne” under “E” rather than in “#”. But it can’t be used to transform a Greek 'π' or a Cyrillic 'П' into a Roman 'P' — and we sure don’t want this Normalizer to do such a thing.

We probably should extend this Normalizer to support diacritics in Greek [1] and Cyrillic [2] scripts; but we’ll still need a list of “index” letters for the three supported scripts in 1.2.

I can open a follow-up to extend the Normalizer. Note that in the long term I think we should get rid of the Normalizer anyway, see bug 914558 comment 16.

[1] http://en.wikipedia.org/wiki/Greek_diacritics
[2] http://en.wikipedia.org/wiki/Cyrillic_script_in_Unicode

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Fabien Cazenave [:kaze] from comment #7)
> (In reply to Ben Kelly [:bkelly] from comment #6)
> > From a performance perspective I think the main concern I have is with when
> > the group DOM is getting created, not its size.  It always happens at load
> > with the same content, but its done dynamically which causes an additional
> > reflow.  So soon I am going to look at either making it static index.html or
> > perhaps creating the group just-in-time when a contact needs to be added to
> > it.
> 
> Creating the group just-in-time would make more sense, imho. I’m afraid
> making it static will cause a significant regression for the startup time
> (first paint).
> 
> Are you saying that you’ll write this part?

Well, its something I'm actively investigating for bug 914913.  Moving from dynamic to static can be faster if the content is always going to be present during the load anyway since we can avoid needless reflows.

I think you're right, though, and in this case doing it just-in-time is probably better.  There is a good chance that at least some groups, like Q, will remain unused.  So we can have a smaller DOM.

I wrote bug 921683 to track this work.

> > As I said in comment 5, though, I think using the Normalizer makes more
> > sense from a functional perspective.
> 
> The Normalizer is used to deal with diacritics in Roman letters: that’s how
> we can sort “Étienne” under “E” rather than in “#”. But it can’t be used to
> transform a Greek 'π' or a Cyrillic 'П' into a Roman 'P' — and we sure don’t
> want this Normalizer to do such a thing.

I'm sold.

Just to clarify, we are planning to leave the scrollbar targets as only Roman letters for 1.2?

Comment 9

[Fabien Cazenave [:kaze]]

(In reply to Ben Kelly [:bkelly] from comment #8)
> Just to clarify, we are planning to leave the scrollbar targets as only
> Roman letters for 1.2?

That’s what I propose to do with this bug, yes. Besides, I think that’s what Android 4.0 does (at least on my SGS2), see bug 914558 comment 15.

I propose to handle scrollbar targets for non-Roman *alphabets* in bug 914558 comment 16, along with a sharper (locale-specific) alphabetic sort. I rather see that as a 1.3+ feature but bug 914558 is leo+ at the moment…

Handling scrollbar targets for non-alphabetic scripts (e.g. Chinese, which is a target locale for 1.2…) is another story. Bug 914589 is specific to Chinese, I guess we’ll need similar bugs for other non-alphabetic languages.

Comment 10

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

(In reply to Fabien Cazenave [:kaze] from comment #7)
> FWIW, for 1.2 we won’t have to deal with more than these three scripts
> (Roman, Greek, Cyrillic).

Actually, there are also Simplified and Traditional Chinese that have been asked for 1.2: see Tracking Bug for all required v1.2 locales in Bug 905292
(FWIW I have not heard back any news concerning the integration of Traditional Chinese though)

Comment 11

[Fabien Cazenave [:kaze]]

Correct, my bad. :-/

As this bug is leo+, my proposed patch brings a short-term solution to the issue with Greek/Cyrillic.
Next step = try to get non-Roman scrollbar indexes too, see bug 914558. This might happen for 1.1, and we should try to make it happen for 1.2.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Is this going to land on master?  I see that its r+, but I don't see it in the repo.

I ask because it will likely conflict with the patch I am working on for bug 921683:

  https://github.com/wanderview/gaia/compare/contacts-dynamic-group?expand=1

This creates the groups only when needed.  Note, I made the ordering of the groups somewhat arbitrary thinking it would make it easier to incorporate these additional scripts.

Thoughts?

Comment 13

[Fabien Cazenave [:kaze]]

Ben, I’ve just updated my pull request to benefit from your patch. As it’s a bit different from the first patch I submitted, asking for review again.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 811072 [details]
link to pull request

Redirecting to our new contact peer :)

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 811072 [details]
link to pull request

Looks good!  Just a few nits on github.  I restarted travis since it errored out previously on npm.

I was able to run the tests locally and verified existing behavior still works on the phone.  I haven't figured out how to type these new scripts, though, so I didn't test the new behavior on device.

Thanks!  r=me

Comment 16

[Fabien Cazenave [:kaze]]

Merged on master: https://github.com/mozilla-b2g/gaia/commit/6e8d90408efa9064ccfa877cb798aadbd2dbc021

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

This bug was partially uplifted.

Uplifted 6e8d90408efa9064ccfa877cb798aadbd2dbc021 to:
v1.2: 930ee0b4d6576fa61969af799f01268794de24b0

Commit 6e8d90408efa9064ccfa877cb798aadbd2dbc021 didn't uplift to branch v1-train

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 6e8d90408efa9064ccfa877cb798aadbd2dbc021
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 19

[Fabien Cazenave [:kaze]]

See comment 12, this bug now depends on bug 921683 — which has been uplifted to v1.2 but not to v1-train yet.

Comment 20

[Fabien Cazenave [:kaze]]

Turns out we also need (at least) bug 879299 to uplift this patch…

If we don’t want to uplift those other patches (which contain some very significant changes), I’ll have to write a completely different patch that is specific to the leo branch.

Comment 21

[Fabien Cazenave [:kaze]]

Attachment: link to pull request (leo branch)

https://github.com/mozilla-b2g/gaia/pull/13402

Comment 22

[Thanos Lefteris [:alefteris]]

Not sure if this has landed in 1.2 branch or if this is the correct place to report it, but I tested with latest 1.2 Gaia and Greek names and noticed that if a name starts with accent character ex. Έκτορας or Ήρα it's listed under # and not under "Ε" or "Η".

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

I take it from comment 7 that Greek letters with diacritics are going to be handled in a followup. Is that correct Fabien?

Aside: there is a GreekUpperCase function defined in Gecko that could be used for this, but I'm not sure if it makes sense to expose it to script instead of just rewriting it in JS.

Comment 24

[Jose Manuel Cantera]

Comment on attachment 827659 [details]
link to pull request (leo branch)

thanks for the work

Comment 25

[Ryan VanderMeulen [:RyanVM]]

v1.1: https://github.com/mozilla-b2g/gaia/commit/dadecdea09e5f9310f10c6689411ecc742244223

Comment 26

[Fabien Cazenave [:kaze]]

(In reply to Panos Astithas [:past] from comment #23)
> I take it from comment 7 that Greek letters with diacritics are going to be
> handled in a followup. Is that correct Fabien?
> 

Correct.

To be sharp though, I think the current approach is wrong: we should rely on the built-in .localeCompare() method to sort contact names properly, without having to rely on a hard-coded list of diacritics.

I have started to work on this. This will require a significant code rewrite in the Contacts app and I still have to make sure this won’t be a performance breaker, so it’s out of the scope of the leo branch.

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: dadecdea09e5f9310f10c6689411ecc742244223
v1.1.0hd: faa8a0c2961afdf1ff4e40a104f672920a40d069

Comment 28

[Fabien Cazenave [:kaze]]

I’m afraid we have to backout the commits to the 1.1 branch:
 • dadecdea09e5f9310f10c6689411ecc742244223
 • faa8a0c2961afdf1ff4e40a104f672920a40d069

Working on a fix right now. :-/

Comment 29

[Fabien Cazenave [:kaze]]

I’ve just backed-out these two commits.
 • v1-train: https://github.com/mozilla-b2g/gaia/commit/4fa6e6362b6a35fd18c7a631dfdaca748cc22c18
 • v1.1.0hd: https://github.com/mozilla-b2g/gaia/commit/77fbd88897006b0a3af6955dd0fb68a4eb476e8e

Comment 30

[Fabien Cazenave [:kaze]]

Attachment: uplift to v1-train

Second try. Let’s make sure this one doesn’t break anything.

Comment 31

[Julien Wajsberg [:julienw]]

Do we really need it on leo anyway?

The leo+ flag was set 2 months ago, is it enough a blocker to land it on a leo branch that where nobody will take updates from anyway (except for v1.1.0hd maybe?), and as we saw, there is a risk in doing this...

I think you should ask on the release driver ML before remerging.

Comment 32

[Fabien Cazenave [:kaze]]

Preeti, what’s your opinion on this?

I tend to think it’s important for two locales that are officially supported by leo (Greek and Serbian) but if nobody’s pulling updates from this branch any more, it might not be worth the trouble.

Comment 33

[Jose Manuel Cantera]

Comment on attachment 832511 [details] [review]
uplift to v1-train

thanks for the uplift

Comment 34

[Jason Smith [:jsmith]]

(In reply to Fabien Cazenave [:kaze] from comment #32)
> Preeti, what’s your opinion on this?
> 
> I tend to think it’s important for two locales that are officially supported
> by leo (Greek and Serbian) but if nobody’s pulling updates from this branch
> any more, it might not be worth the trouble.

If these are target languages for 1.1, then this needs to be taken to 1.1. If we ever have a 1.1.1 happen, then we shouldn't let this regress. If there's concern about risk, then we can have someone do some exploratory testing around the patch to ensure it doesn't cause serious regressions.

Note though - risk doesn't have an effect on a blocking decision. It only effects an uplift decision for non-blockers.

Comment 35

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 19c9ff3

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 19c9ff3a46a4389e40253c97b359763243af4531
v1.1.0hd: 3648c6796d09217718c2c7d34885d702e31c32f9

Comment 37

[Preeti Raghunath(:Preeti)]

Its already in 1.1 hd and will take it for 1.1.1 if needed.