Closed Bug 1000599 Opened 11 years ago Closed 11 years ago

Use the new & fixed mozL10n.ready in Settings

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Settings
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(2 files)

Use mozL10n.ready
3.94 KB, patch
arthurcc
: review+
zbraniecki
: feedback+
Details | Diff | Splinter Review
Other uses of the localized event
4.05 KB, text/plain
Details
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.

Attachment

General

Creator:
Created:
Updated:
Size: