Closed Bug 1000599 Opened 5 years ago Closed 5 years ago

Use the new & fixed mozL10n.ready in Settings

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(2 files)

The Settings app currently uses a clunky combination of mozL10n.once and window.addEventListener because of the broken mozL10n.ready method.

https://github.com/mozilla-b2g/gaia/blob/1ec50063098805adff9bdc3347d8c26421e0bc34/apps/settings/js/settings.js#L331

I cleaned it up a little bit in bug 993189, but when mozL10n.ready is fixed in bug 993188, we will be able to simply use that instead.
https://github.com/mozilla-b2g/gaia/pull/18933

This is a pretty simple change.  We can get rid of startupLocale in favor of mozL10n.ready.  Settings initialization logic is run in a window.onload handler, so the DOM is guaranteed to be available when mozL10n.ready is called.
Assignee: nobody → stas
Status: NEW → ASSIGNED
Attachment #8417295 - Flags: feedback?(gandalf)
The Settings app uses the localized event in a few more places.  Here's a quick grep.  Most of them look legitimate to me.  Gandalf, should we leave them as they are right now?
Flags: needinfo?(gandalf)
Attachment #8417295 - Flags: feedback?(gandalf) → feedback+
(In reply to Staś Małolepszy :stas from comment #2)
> Created attachment 8417296 [details]
> Other uses of the localized event
> 
> The Settings app uses the localized event in a few more places.  Here's a
> quick grep.  Most of them look legitimate to me.  Gandalf, should we leave
> them as they are right now?

Yes, I believe we will want to do another round of reviews against events when we'll get to bug 1000806
Flags: needinfo?(gandalf)
Comment on attachment 8417295 [details] [diff] [review]
Use mozL10n.ready

Evelyn -- this is the follow-up to bug 993189, which now uses the correct behavior of mozL10n.ready (it guarantees to register a listener for future language changes).  Would you mind taking a look?
Attachment #8417295 - Flags: review?(ehung)
Comment on attachment 8417295 [details] [diff] [review]
Use mozL10n.ready

Stas, thanks for the patch. I'd like to redirect to Arthur since he knows this code segment more than me.

@Arthur, thank you.
Attachment #8417295 - Flags: review?(ehung) → review?(arthur.chen)
Comment on attachment 8417295 [details] [diff] [review]
Use mozL10n.ready

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

Thanks for the patch! Could you also create a PR so that travis can run the tests?

::: apps/settings/js/settings.js
@@ -87,5 @@
> -      var lang = navigator.mozL10n.language.code;
> -
> -      // set the 'lang' and 'dir' attributes to <html>
> -      // when the page is translated
> -      document.documentElement.lang = lang;

This seems to be used by the "inlineLocalization" function in l10n.js, not sure if we could remove this line.

@@ -88,5 @@
> -
> -      // set the 'lang' and 'dir' attributes to <html>
> -      // when the page is translated
> -      document.documentElement.lang = lang;
> -      document.documentElement.dir = navigator.mozL10n.language.direction;

We need to keep this because the styles use it to support rtl layouts.
Attachment #8417295 - Flags: review?(arthur.chen)
Comment on attachment 8417295 [details] [diff] [review]
Use mozL10n.ready

Hey Arthur, thanks for your comments.  The refactored l10n.js now takes care of setting the lang and dir of the <html> element, so apps don't have to worry about anymore.

https://github.com/mozilla-b2g/gaia/blob/eb6ee83804ea79e65b716385a46a86e256bbda4c/shared/js/l10n.js#L1444-L1445

Re-requesting review.

Pull request: https://github.com/mozilla-b2g/gaia/pull/18933
Travis run: https://travis-ci.org/mozilla-b2g/gaia/builds/24438231 (green)
Attachment #8417295 - Flags: review?(arthur.chen)
Comment on attachment 8417295 [details] [diff] [review]
Use mozL10n.ready

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

Cool! r=me, thanks!
Attachment #8417295 - Flags: review?(arthur.chen) → review+
You need to log in before you can comment on or make changes to this bug.