sync-1 Reporter
	Description • 11 years ago

AU_LINUX_GECKO_ICS_STRAWBERRY.01.01.00.019.171
 Firefox os  v1.1
 Mozilla build ID:20130722070207
 
 
 Created an attachment (id=471106)
 KO3
 
 DEFECT DESCRIPTION:
  When you change the languages,Contacts can't displayed correctly.
 
  REPRODUCING PROCEDURES:
  1.Set language as English,Launch Contacts
  2.Add contact screen->slide down notification->from the notification bar, tap Settings icon->Set language as Francais->Press OK
  3.Long press Home Key,there are 3 choices
    (1)Contacts was disappeared---->KO1
    (2)Contacts can't be opened---->KO2
    (3)Select Contacts,it's create contact screen,The title was not translate corrently---->KO3
 
  EXPECTED BEHAVIOUR:
  Change the languages,it should open Contacts normally,and title can be translated correctly.
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
  Mid
  REPRODUCING RATE:
  5/5
  For FT PR, Please list reference mobile's behavior:

	sync-1 Reporter
	Comment 1 • 11 years ago

Clone from brother

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 3 • 11 years ago

I've reproduced KO3, but have not seen the other behavior.  I'm out of time for today, but can investigate further tomorrow.

	buri.blff
	Comment 4 • 11 years ago

Long press Home Key,there are 3 choices
    (1)Contacts app was disappeared---->KO1
    (2)Contacts app can't be opened---->KO2
    (3)Select Contacts,it's create contact screen,The title was not translate corrently---->KO3

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 5 • 11 years ago

It appears that other fields in contacts app also fail to update when the language is changed.  For example, see the Theo's comments in bug 898241.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 6 • 11 years ago

Here is a pull request that fixes the language update issue, but at the expense of losing any unsaved data in the form.  It works by simply re-rendering the contact detail and form panels when the 'localized' event fires.

Right now it looks like much more work would be needed to update the translated fields in the form without disturbing unsaved, edited content.  The logic to do all of the tel/email/etc translation is in insertField() which seems somewhat complicated.  Perhaps we could use a selector to get all the translated fields, but we would then need to map them back to the original values in order to do the translation.  In addition, if a new field has been entered then we may need to translate a non-original field as well.

My impression was that this would mean some more significant refactoring to the contact_form.js file.  I thought I would get some feedback for you before proceeding.

(We could also land the contact_detail.js fix separately from contact_form.js since it does not have this problem.)

What do you guys think?

Thanks!

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 7 • 11 years ago

After thinking about this some more, it seems like this is something the l10n.js library should be doing.  I thought since we set the l10n-id attribute it would automatically re-translate the fields.  Perhaps we are doing something that is overriding this behavior.  I'll take a look tomorrow morning.

	Julien Wajsberg [:julienw]
	Comment 8 • 11 years ago

need info Kaze on this as I think he has some light to shed on this.

	Fabien Cazenave [:kaze]
	Comment 9 • 11 years ago

(In reply to Ben Kelly [:bkelly] from comment #7)
> After thinking about this some more, it seems like this is something the
> l10n.js library should be doing.  I thought since we set the l10n-id
> attribute it would automatically re-translate the fields.  

Correct. And that’s what the mozL10n.localize() method is about.
Having a look at this bug right now…

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 10 • 11 years ago

Comment on attachment 783406 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/11256

Kaze is looking at the code now, but we can at least improve upon this pull request by simply calling mozL10n.translate(form) from the 'localized' event.  Removing feedback requests until I can update for this solution or alternative recommended by Kaze.

	bhavana bajaj [:bajaj]
	Comment 11 • 11 years ago

IS this a 1.1 regression?

	Natalya Kot [:nkot]
	Comment 12 • 11 years ago

(In reply to bhavana bajaj [:bajaj] from comment #11)
> IS this a 1.1 regression?

I've tested on Unagi 7/29 v1.0.1 build, have reproduced KO3 ( tested with "Deutsch")
 - same behavior, the title is not translated correctly.
Not a regression then.

Build ID: 20130729070220
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
Gaia: 054cdc27404e2daca91d3065d9783681032b2151
Platform Version: 18.0

	Jose Manuel Cantera
	Comment 13 • 11 years ago

oh great Ben. Please ping us when you have an updated solution.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 14 • 11 years ago

Comment on attachment 783406 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/11256

I believe I have identified the issue.  When the language observer fires and calls loadLocale(), the code first attempts to parse the inlined dictionary and then translate.  If this is successful, then translation is disabled for the later resource load.

This causes the problem because these fields are not present in the static index.html which in turn means they are not in the inlined JSON dictionary.  The build system only includes fields there which are used in the index.html.

It seems that in order to correctly automatically translate dynamic content we must retranslate after the full resource load occurs.

I've updated the pull request to do this.  Since this is in l10n.js I am requesting review from kaze.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 15 • 11 years ago

One possible enhancement to this fix would be to modify translateFragment() and translateElement() to return a value indicating whether a node failed to translate.  We could then determine if the secondary translation is necessary.  Its unclear to me if this optimization is really worth it, though.

	Fabien Cazenave [:kaze]
	Comment 16 • 11 years ago

Comment on attachment 783406 [details]
Pull request at https://github.com/mozilla-b2g/gaia/pull/11256

Good catch!

(In reply to Ben Kelly [:bkelly] from comment #15)
> One possible enhancement to this fix would be to modify translateFragment()
> and translateElement() to return a value indicating whether a node failed to
> translate.  We could then determine if the secondary translation is
> necessary.  Its unclear to me if this optimization is really worth it,
> though.

I think it’s worth optimizing it a bit more… Let me propose a patch.

	Fabien Cazenave [:kaze]
	Comment 17 • 11 years ago

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

Ben, this is an optimized version of your patch: when the inline directory is not enough to translate the current document, this patch waits for the external l10n resources to be loaded and translates /only/ the elements that couldn’t be translated with the inline directory (= the content created dynamically, logically).

As a bonus, l10n warnings about missing entities are now grouped properly — and switching the locale won’t cause l10n warnings for the dynamic content any more.

	Julien Wajsberg [:julienw]
	Comment 18 • 11 years ago

Comment on attachment 784711 [details]
link to pull request

several changes needed, essentially to make it clearer and have a smaller diff.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 19 • 11 years ago

This is too much churn/risk to take so late in the cycle and should ride the trains on v1.2 - Aside from testing we don't expect many users change languages on their devices in the middle of doing a task and this isn't a regression from v1.0.1

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 20 • 11 years ago

Here is a unit test case the exposes the issue.  I have validated it fails with the current code and passes with my one-line quick fix.  It should also pass with the kaze's pull request.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 21 • 11 years ago

Comment on attachment 784711 [details]
link to pull request

I think the patch looks good.  You've already addressed the few nits I saw thanks to Julien's thorough review.

My only other concern was the lack of a unit test.  I think this needs a test case since the failure mode is non-obvious and may appear like bugs in other apps.  I took a shot at writing a test in attachment 785132 [details] [diff] [review].

I suggest that you incorporate the test into your PR, assuming it looks ok, and then mark julien for overall review of the updated PR.

Sound reasonable?  Thanks!

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 22 • 11 years ago

The previous patch had a stupid copy/paste mistake.  It didn't effect the result of the test, but here is a corrected patch.

	Fabien Cazenave [:kaze]
	Comment 23 • 11 years ago

(In reply to Ben Kelly [:bkelly] from comment #21)
> I suggest that you incorporate the test into your PR, assuming it looks ok,
> and then mark julien for overall review of the updated PR.
> 
> Sound reasonable?

Deal. Thanks for the tests!

	Fabien Cazenave [:kaze]
	Comment 24 • 11 years ago

Comment on attachment 784711 [details]
link to pull request

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

Julien, I’ve addressed all your comments and added Ben’s unit tests to the PR.

	Julien Wajsberg [:julienw]
	Comment 25 • 11 years ago

Comment on attachment 784711 [details]
link to pull request

r=me with nits and passing tests

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 26 • 11 years ago

Here is a patch to address Julien's review comments on the tests.

I'm sure there is an easier way to do this with github, but I thought posting a patch would be easiest for now.

	Fabien Cazenave [:kaze]
	Comment 27 • 11 years ago

Merged on master:
https://github.com/mozilla-b2g/gaia/commit/09f70c7de1a4e918766e7757a660a0d9732dcba7

	buri.blff
	Comment 28 • 11 years ago

(In reply to Fabien Cazenave [:kaze] from comment #27)
> Merged on master:
> https://github.com/mozilla-b2g/gaia/commit/
> 09f70c7de1a4e918766e7757a660a0d9732dcba7

is the patch merge to v1.1?

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 29 • 11 years ago

(In reply to buri.blff from comment #28)
> (In reply to Fabien Cazenave [:kaze] from comment #27)
> > Merged on master:
> > https://github.com/mozilla-b2g/gaia/commit/
> > 09f70c7de1a4e918766e7757a660a0d9732dcba7
> 
> is the patch merge to v1.1?

No, because this bug was marked leo- in comment 19.  Please re-nominate for leo? if you think this was in error.

	Jack Liu
	Comment 30 • 11 years ago

I mark it leo? again since it impact user experience. Please uplift the patch to v1.1.
Thanks.

	Julien Wajsberg [:julienw]
	Comment 31 • 11 years ago

I really think this should not be uplifted:

* this addresses only the contacts app, other apps (notably the Messages app) needs more work to do this (that work has already started in bug 886715). So uplifting this won't make the whole OS better after all. We should rather do some QA work to identify the applications that need to be fixed in 1.2, and block on this if this is important.

* this is quite invasive, and we're so close to the end. If we want to uplift this we'll need a massive QA in all apps.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 32 • 11 years ago

Too invasive for final rounds of IOT on v1.1 - we'll have to take this in v1.2 and later.

	buri.blff
	Comment 33 • 11 years ago

(In reply to lsblakk@mozilla.com [:lsblakk] from comment #32)
> Too invasive for final rounds of IOT on v1.1 - we'll have to take this in
> v1.2 and later.

the patch dosen't resolve this pr. 
we can reproduce the KO3 with the patch in commit 27?

	buri.blff
	Comment 34 • 11 years ago

the patch dosen't resolve this pr. 
we can reproduce the KO3 with the patch in commit 27?

	buri.blff
	Comment 35 • 11 years ago

if we don't back out the contact app, the transelate will not be refreshed.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 36 • 11 years ago

(In reply to sync-1 from comment #0)
>   3.Long press Home Key,there are 3 choices
>     (1)Contacts was disappeared---->KO1
>     (2)Contacts can't be opened---->KO2
>     (3)Select Contacts,it's create contact screen,The title was not
> translate corrently---->KO3
>  
>   EXPECTED BEHAVIOUR:
>   Change the languages,it should open Contacts normally,and title can be
> translated correctly.

I originally took this to mean that when opening the contact app the title should be translated.  I just rechecked on master (only place the patch has landed) and the title is now updated properly.

However, the "long press home key" method of selecting the open contacts app still shows the old translation during selection.  I believe this is because that selection screen is just showing screenshots of the various open apps.  Is it the translation in the selection process you are asking about?

If the answer is yes, then I think it would be helpful to open a new bug.

Otherwise, can you post the gaia revision hash you are retesting with?

	buri.blff
	Comment 37 • 11 years ago

(In reply to Ben Kelly [:bkelly] from comment #36)
> (In reply to sync-1 from comment #0)
> >   3.Long press Home Key,there are 3 choices
> >     (1)Contacts was disappeared---->KO1
> >     (2)Contacts can't be opened---->KO2
> >     (3)Select Contacts,it's create contact screen,The title was not
> > translate corrently---->KO3
> >  
> >   EXPECTED BEHAVIOUR:
> >   Change the languages,it should open Contacts normally,and title can be
> > translated correctly.
> 
> I originally took this to mean that when opening the contact app the title
> should be translated.  I just rechecked on master (only place the patch has
> landed) and the title is now updated properly.
> 
> However, the "long press home key" method of selecting the open contacts app
> still shows the old translation during selection.  I believe this is because
> that selection screen is just showing screenshots of the various open apps. 
> Is it the translation in the selection process you are asking about?
> 
> If the answer is yes, then I think it would be helpful to open a new bug.
> 
> Otherwise, can you post the gaia revision hash you are retesting with?

the ko3 means that when we select the background contact app by click the screenshots after long press home, the title shoudle be translated. it does't mean to open contacta app by click it's icon on homescreen.
so we don't need to open an new bug;

	buri.blff
	Comment 38 • 11 years ago

So the patch in commit 27 is for another pr not current.

we test the patch by add the patch to version Mozilla build ID:20130806071254

	buri.blff
	Comment 39 • 11 years ago

(In reply to buri.blff from comment #37)
> (In reply to Ben Kelly [:bkelly] from comment #36)
> > (In reply to sync-1 from comment #0)
> > >   3.Long press Home Key,there are 3 choices
> > >     (1)Contacts was disappeared---->KO1
> > >     (2)Contacts can't be opened---->KO2
> > >     (3)Select Contacts,it's create contact screen,The title was not
> > > translate corrently---->KO3
> > >  
> > >   EXPECTED BEHAVIOUR:
> > >   Change the languages,it should open Contacts normally,and title can be
> > > translated correctly.
> > 
> > I originally took this to mean that when opening the contact app the title
> > should be translated.  I just rechecked on master (only place the patch has
> > landed) and the title is now updated properly.
> > 
> > However, the "long press home key" method of selecting the open contacts app
> > still shows the old translation during selection.  I believe this is because
> > that selection screen is just showing screenshots of the various open apps. 
> > Is it the translation in the selection process you are asking about?
> > 
> > If the answer is yes, then I think it would be helpful to open a new bug.
> > 
> > Otherwise, can you post the gaia revision hash you are retesting with?
> 
> the ko3 means that when we select the background contact app by click the
> screenshots after long press home, the title shoudle be translated. it
> does't mean to open contacta app by click it's icon on homescreen.
> so we don't need to open an new bug;

the ko3 means that when we launch the background contact app by click the
screenshots after long press home, the title shoudle be translated.

	Julien Wajsberg [:julienw]
	Comment 40 • 11 years ago

(In reply to buri.blff from comment #37)
> 
> the ko3 means that when we select the background contact app by click the
> screenshots after long press home, the title shoudle be translated. it
> does't mean to open contacta app by click it's icon on homescreen.
> so we don't need to open an new bug;

Let me rephrase the STR so that we understand ourselves clearly:

* open the contacts app
* press home
* open the settings app
* change the language
* long-press home
* select the contacts app
=> the contacts app is displayed
=> KO3 (?)

I _think_ this is fixed. Ben, can you confirm this ?

Buri, please correct if this STR (Steps To Reproduce) is not what you're doing.

Thanks !