Théo Chevalier [:tchevalier] Reporter
	Description • 10 years ago

STR:

1. Switch to French
2. Go to homescreen
3. Long press on an empty area
4. "Cancel" button is not localized

Not sure if it's the same issue as on https://bugzilla.mozilla.org/show_bug.cgi?id=1022445 (There is also another unlocalized "Cancel" button there)
CC'ing Stas just in case


Tested with latest Flame build:
Gaia      f42ebc93554979501d3ac52bcf9e69cb4b310a4f
Gecko     https://hg.mozilla.org/mozilla-central/rev/9dc0ffca10f4
BuildID   20140610040208
Version   33.0a1
ro.build.version.incremental=eng.cltbld.20140610.071214
ro.build.date=Tue Jun 10 07:12:22 EDT 2014

	Jason Smith [:jsmith]
	Comment 1 • 10 years ago

Can we get a screenshot?

	Théo Chevalier [:tchevalier] Reporter
	Comment 2 • 10 years ago

Sure.
I also tried in Italian, same result.

	Jason Smith [:jsmith]
	Comment 3 • 10 years ago

Technically this is a system prompt, so I'd expect this to be reproducing on either homescreen. So we should be able to get a window here for 2.0.

We should confirm this doesn't reproduce on 1.4, although I'm pretty sure that it won't reproduce there.

	Théo Chevalier [:tchevalier] Reporter
	Comment 4 • 10 years ago

Just checked, and can't reproduce on Horizontal homescreen

	Jason Smith [:jsmith]
	Comment 5 • 10 years ago

Ok - guess we can call this a user-facing regression from switching homescreens then.

	Cristian Rodriguez (:crdlc)
	Comment 6 • 10 years ago

Hi Kevin, do you know if this is a problem related to web components?

https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_menu/script.js#L43

	Kevin Grandon :kgrandon Assignee
	Comment 7 • 10 years ago

Yeah, seems like we need manual localization of web components for now (hopefully we can fix this in the future). I will take this for now.

	Kevin Grandon :kgrandon Assignee
	Comment 8 • 10 years ago

Review anyone?

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 9 • 10 years ago

Stas: can you look at this?

Kevin: this may also be fixed by bug 992473 (or a followup that will extend MO to reach to ShadowDOM)

	Staś Małolepszy :stas
	Comment 10 • 10 years ago

(In reply to Zibi Braniecki [:gandalf] from comment #9)
> Stas: can you look at this?

Sure, I'll have a look while you're on PTO, no worries.  Leaving the needinfo request for now.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 11 • 10 years ago

Kevin: for the future reference. Please, f? me or stas on bugs like that. We want to take part in designing the interaction between web components and l10n.

	Kevin Grandon :kgrandon Assignee
	Comment 12 • 10 years ago

(In reply to Zibi Braniecki [:gandalf] from comment #11)
> Kevin: for the future reference. Please, f? me or stas on bugs like that. We
> want to take part in designing the interaction between web components and
> l10n.

Good to know. I'm definitely looking forward to a nice interaction between the two, I'm just not sure what makes sense to get into 2.0. Do you have any thoughts/concerns about this patch? I'm more than happy to mark you as F? or R? on this one, but I consider this more of a temporary workaround/hack for the long term solution.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 13 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #12)
> Good to know. I'm definitely looking forward to a nice interaction between
> the two, I'm just not sure what makes sense to get into 2.0. Do you have any
> thoughts/concerns about this patch? I'm more than happy to mark you as F? or
> R? on this one, but I consider this more of a temporary workaround/hack for
> the long term solution.

roger that. we'll want to design sth long term. I'm going on pto and stas is designing our next revision of L10n API, so I feel he's better suited to look into this. :)

	James Lal [:lightsofapollo]
	Comment 14 • 10 years ago

So this is in theory late l10n ? We need to make sure these strings get copied over in the gecko-l10n thing.

	James Lal [:lightsofapollo]
	Comment 15 • 10 years ago

Comment on attachment 8438657 [details]
Github pull request

I am going to r+ this since I think it's probably the safest thing for us to do to get 2.0 out the door but this is essentially a hack around the lack of good l10n for webcomponents (which I think we can fix without too much effort). Can we file bugs for this and reference them where we do the shadowDom hacks so its clear we need to fix this?

	Francesco Lodolo [:flod]
	Comment 16 • 10 years ago

We're not adding new strings, right? We already have that ID, also on 2.0
https://github.com/mozilla-b2g/gaia/blob/v2.0/apps/verticalhome/locales/homescreen.en-US.properties#L8

If that's the case, the bug is not late-l10n (feel free to add it back if I misunderstood the code in this PR).

	James Lal [:lightsofapollo]
	Comment 17 • 10 years ago

Its not a _new_ string but one we need to copy for all the locales into the verticalhome we don't reference the one in homescreen in this code. Not sure how to signal this other then late-l10n.

	Kevin Grandon :kgrandon Assignee
	Comment 18 • 10 years ago

Comment on attachment 8438657 [details]
Github pull request

Thanks for the review. I'll land for now, but will follow-up on bug 992473 to see if it fixes shadow dom localization or not.

	Théo Chevalier [:tchevalier] Reporter
	Comment 19 • 10 years ago

(In reply to James Lal [:lightsofapollo] from comment #17)
> Its not a _new_ string but one we need to copy for all the locales into the
> verticalhome we don't reference the one in homescreen in this code. Not sure
> how to signal this other then late-l10n.

If you don't add anything to en-US.properties file, then it's not late-l10n.

	Francesco Lodolo [:flod]
	Comment 20 • 10 years ago

(In reply to James Lal [:lightsofapollo] from comment #17)
> Its not a _new_ string but one we need to copy for all the locales into the
> verticalhome we don't reference the one in homescreen in this code. Not sure
> how to signal this other then late-l10n.

There's no need to signal the presence of new strings: tools will be them up and display them as untranslated.

About late-l10n
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Keywords.3A_l12y.2C_late-l10n

	Staś Małolepszy :stas
	Comment 21 • 10 years ago

Kevin, I have a few comments about the patch:

 - please don't require shared/js/l10n.js in tests.  Especially after bug 992473 lands, having real mozL10n set up its mutation observers could lead to weird race conditions in the tests.  Let's use MockL10n for that.  I'm close to landing bug 1004973 if you'd like to use that.

 - we shouldn't test this against the French localization,  as we're going to remove it soon: bug 1011519.  But, as I don't have a good alternative to suggest right now, I think landing this is fine.  I'll find a way to deal with this in bug 1011519.  (Pseudolocales added in bug 900182 could help here, but they're currently created dynamically.)

	Staś Małolepszy :stas
	Comment 22 • 10 years ago

BTW using navigator.mozL10n.translate(template); should be OK for now (2.0 scope).  In the future, we're considering creating L10n contexts for each component with a separate set of resources.  Another idea was to expose the localizable string required by the web component in the projected content, so that the developer becomes responsible for providing localized values.  See bug 1011807 comment 22.

	Kevin Grandon :kgrandon Assignee
	Comment 23 • 10 years ago

Thanks or the comments. Feel free to file a follow-up bug if you'd like if anything is terrible.

I also saw that we use real l10n in many tests, and is like to understand why that's not goij to be good. We should be able to use real libraries in tests if we like, it can be easier/cleaner than using mocks IMO.

	Kevin Grandon :kgrandon Assignee
	Comment 24 • 10 years ago

Going to land for now, but will certainly revisit this as the l10n/wc landscape changes.

https://github.com/mozilla-b2g/gaia/commit/90777363ed0a4e6d32612074a12fb2c73c353a25

	Staś Małolepszy :stas
	Comment 25 • 10 years ago

(In reply to Kevin Grandon :kgrandon from comment #23)
> Thanks or the comments. Feel free to file a follow-up bug if you'd like if
> anything is terrible.
> 
> I also saw that we use real l10n in many tests, and is like to understand
> why that's not goij to be good. We should be able to use real libraries in
> tests if we like, it can be easier/cleaner than using mocks IMO.

Not that many, actually :)  See bug 992473 comment 60: there are only 6 other unit test files where we require shared/js/l10n.js in tests, and I already have a patch under review to fix those in the SMS app.

The startup logic of l10n.js is pretty intricate becuase of how performance-sensitive it is.  Including the real library in tests which don't link to locales.ini or don't have <link type="application/l10n"> in <head> may lead to unexpected results in tests, like "L10nError: Context not ready" errors.  Moreover, with bug 992473, the real lib will start registering a Mutation Observer on document which will kick in whenever a test will modify the localizable DOM.  Even if you use MockL10n, the mutation observer will continue chaning textContent of elements that you testing.

I'll file a follow-up to remove the require('/shared/js/l10n.js') call and also I'll try to come up witha solution for testing things when 'fr' is no longer in the repository.

Thanks!

	Kevin Grandon :kgrandon Assignee
	Comment 26 • 10 years ago

Comment on attachment 8438657 [details]
Github pull request

This is required for the vertical homescreen. We've done our best effort at testing and believe the tree will remain green with uplift.

	Johan Lorenzo [:jlorenzo] - On PTO, back on September 11th
	Comment 27 • 10 years ago

"Annuler" is now back on master.

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

v2.0: https://github.com/mozilla-b2g/gaia/commit/57fd8754cc5c9360d184237773589a7c6c914126