Refactor System to use modern L10n API

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

That mainly means we want to remove all `mozL10n.get` uses.
Depends on: 1224065
Assignee: nobody → gandalf
Comment on attachment 8694109 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part1 > mozilla-b2g:master

Fernardo, can you review my patch?
Attachment #8694109 - Flags: review?(ferjmoreno)
Comment on attachment 8694109 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part1 > mozilla-b2g:master

Erienne, can you review this patch as well pls?
Attachment #8694109 - Flags: review?(etienne)
Comment on attachment 8694109 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part1 > mozilla-b2g:master

r=me on the identity, payments and mobile ID parts.

Thanks!
Attachment #8694109 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8694109 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part1 > mozilla-b2g:master

Thanks! It was indeed a really manageable patch :)

r=me for everything except the migrator part which I know _nothing_ about, hoping that Sam will.
Attachment #8694109 - Flags: review?(sfoster)
Attachment #8694109 - Flags: review?(etienne)
Attachment #8694109 - Flags: review+
Comment on attachment 8694109 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part1 > mozilla-b2g:master

I dont know much about settings_migrator.js, but I do know we are using the Intl api for 12/14 hour concerns now so removing these tests seems like the right thing to do, so I'll add my r+ FWIW.
Attachment #8694109 - Flags: review?(sfoster) → review+
Thanks!

Commit of part1: https://github.com/mozilla-b2g/gaia/commit/89349eb99f91ba91973432f7a21058d8d2251075

Part2 is a bit more complex since I'm moving ValuePicker and ValueSelector and it requires a bit more serious refactor to make it work using Intl and L20n.
Comment on attachment 8698221 [details] [review]
[gaia] KevinGrandon:test_backout > mozilla-b2g:master

Oops - just testing, ignore this for now. Seeing some weirdness with the tests, and wanted to see if this caused it.
Attachment #8698221 - Attachment is obsolete: true
Ok, it seems that there already was an intermittent bug 1192843 and this patch made it more permanent.

The way I'm fixing it is two-fold -

a) I'm making sure that elements in marionette tests are visible to screen reader
b) I'm artificially keeping 'OK' string in alert dialog

The reason for b) is that it seems that in marionette context l10n.js' MutationObserver is not running and setting data-l10n-id on okButton does not translate it. Without manually set string in the template, marionette test fails because it cannot reach to the element without value.

In real world l10n.js works and the element is properly localized.

This should also fix bug 1192843.
Flags: needinfo?(gandalf)
Attachment #8694109 - Attachment is obsolete: true
Comment on attachment 8699412 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part1 > mozilla-b2g:master

Ok, this is not going to be fun.

Tim, the second patch is probably the most entangled one. I'm refactoring pieces of very ancient code here - most of it dates back to 2012.

The scope of this patch:

 - Make ValuePicker work with L10n IDs and raw strings
 - Remove the use of properietary toLocaleFormat
 - Remove the use of mozL10n.get
 - Remove the reliance on values being western arabic digits (`getSelectedDisplayedText` and so on)
 - Conform the InputParser to input type=time|date norms (HH:MM, not h:MM)
 - Moves the hardcoded "0" padding to Intl.NumberFormat

Fred: there's a small piece of the patch that touches Settings. Hopefully nothing controversial :)

Testing completed:

 - I tested Settings, Calendar event add/modify, Clock alarm set/modify, Contacts add/modify.
 - I updated tests
 - I tested against en-US, ar, fr, de, pl, it, and ru
 - I tested on two devices

It *seems* to me that this patch works well with all tested apps without any changes to the API use so I'm hopeful it will be a smooth landing, but since it's a very old code very deep in the System I'd like to land it early in the cycle.
Attachment #8699412 - Flags: review?(timdream)
Attachment #8699412 - Flags: review?(gasolin)
Comment on attachment 8699412 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part1 > mozilla-b2g:master

I assume you want me to review part 2? Part 1 has already merged.

I did not work on value selector directly so my review will not be very helpful, still I tried to spot a few stuff and leave some comment there. Otherwise this looks good.
Attachment #8699412 - Flags: review?(timdream) → review+
Comment on attachment 8697576 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part2 > mozilla-b2g:master

You should probably land this on under another bug #?
Attachment #8697576 - Flags: review+
Comment on attachment 8699412 [details] [review]
[gaia] zbraniecki:1224063-migrate-system-away-from-mozl10n-get-part1 > mozilla-b2g:master

Looks good, thanks!
Attachment #8699412 - Flags: review?(gasolin) → review+
Depends on: 1235975
Depends on: 1237496
Depends on: 1244629
And this is complete now!
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.