975637 - [B2G][l10n][FTE] Continent names display in English in the timezone title text

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Ross Kuhlman ([e-mail])]

Attachment: ContinentInconsistency.png

During the FTE, the user can choose what location they are in. While selecting a location, the device will update to inform the user what timezone and region they are in. The timezone title contains the name of the continent and city that the user has selected. The continent always displays in English.

Repro Steps:
1) Updated Buri to BuildID: 20140221004002
2) Begin FTE.
3) Proceed through FTE until prompted to specify Location, Date and Time.
4) Observe the name of he continent displayed in the Timezone text.
5) Compare results from step 4 to the name of the continent in the continent selection menu.

Actual:
The Continent displayed in the timezone is displayed in English.

Expected:
The Continent displayed in the timezone is displayed in Serbian-Cyrillic.

Environmental Variables:
Device: Buri v1.3 Moz RIL
BuildID: 20140221004002
Gaia: 8039a5cb7519adfa81677df577f494c6a4de6140
Gecko: e5f25becc0e7
Version: 28.0
Firmware Version: v1.2-device.cfg

Notes:

Repro frequency: 100%
See attached: screenshot.

Comment 1

[Delphine Lebédel [:delphine - Use needinfo]]

Isn't this a bug that exists on all locales? I remember seeing this somewhere.
Unless this has been corrected for 1.3?

Comment 2

[Delphine Lebédel [:delphine - Use needinfo]]

My wrong, I think that was for cities: https://bugzilla.mozilla.org/show_bug.cgi?id=837800

Comment 3

[Vanja Tumbas [:civas]]

Attachment: 2014-02-25-19-00-50.png

I checked few more languages.
This bug exists on all locales. Adding image for Italian locale.

Francesco can you help with this bug?

Comment 4

[Francesco Lodolo [:flod]]

I can confirm it happens on my locale on 1.4. Moving to FTU since it's definitely not a Serbian problem.

Comment 5

[Francesco Lodolo [:flod]]

Time zone names are defined in shared/locales/tz/tz.properties, but while a function is used to generate the select there's no such thing for the title
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/ftu/js/ui.js#L429

Comment 6

[Delphine Lebédel [:delphine - Use needinfo]]

Proposing to nominate this as a must have for 1.4

Comment 7

[Francesco Lodolo [:flod]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16687/files

Asking also Pike's opinion: is this approach overkill?

We can solve the problem reported in this bug just by replacing timezone.id with timezone.region, but I think it makes more sense to have a string and let locales change order or separators.

Comment 8

[Francesco Lodolo [:flod]]

Attachment: Screenshot of Keon with patch applied

Comment 9

[Francesco Lodolo [:flod]]

Following :fcampo suggestion on Github, moved "UTC" to the string and fixed another concatenation.

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 8382802 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16687/files

Looks good to me, short of

+# Example of a resulting string for English: "UTC +01:00 Europe/Paris"

+timezoneTitle = UTC {{utcOffset}} {{region}}/{{city}}

adding a space now after UTC, where there was none. And I think it reads better without, as it's more like 'utc+1', 'Europe/Paris' than 'utc', '+1', 'europe/paris'.

Comment 11

[Francesco Lodolo [:flod]]

(In reply to Axel Hecht [:Pike] from comment #10)
> adding a space now after UTC, where there was none. 

Absolutely right, I didn't want to introduce this change. Updated PR to fix it.

Comment 12

[Fernando Campo (:fcampo)]

Comment on attachment 8382802 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16687/files

Looks good, thanks for fixing this

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This has real-looking Travis failures.

Comment 14

[Francesco Lodolo [:flod]]

Tried a new push to run Travis again.
Unit test is failing on Calendar, marionette in Gallery, so they seem unrelated.

Comment 15

[Francesco Lodolo [:flod]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #13)
> This has real-looking Travis failures.

They were unrelated failures, this time it passed (it did pass locally before)
https://travis-ci.org/mozilla-b2g/gaia/builds/19742478

Comment 16

[Anthony Ricaud (:rik)]

This patch is lacking new tests. It can't land as is.

Comment 17

[Francesco Lodolo [:flod]]

As far as I can see there's no test coverage at all for the timezone section of FTU, absolutely no idea if I'm able to add tests for that (so far I'm just fixing broken tests).

Comment 18

[Francesco Lodolo [:flod]]

I checked the existing tests. I could add a section in mock_navigation_index.html with a title with id="time-zone-title", and then add an assertion in 'date&time screen >', but I'm still failing to understand how useful such a test would be since we're not using the actual l10n.js but a mockL10n, returning a predetermined value.

I'm sure that's my limit, not being a developer, but could someone explain to me why we need such tests in this case and what they should cover?

Comment 19

[Francesco Lodolo [:flod]]

Based on feedback received by :kaze in bug 976663, I'm going to rewrite this PR using mozL10n.localize
Also discussed about the lack of tests, and this should be just covered by l10n.js tests.

Comment 20

[Fabien Cazenave [:kaze]]

(In reply to Anthony Ricaud (:rik) from comment #16)
> This patch is lacking new tests. It can't land as is.

This is an l10n fix, what should we test? We already have unit tests for l10n.js (in the Gallery app), I don’t really see what kind of significant tests we could add for this patch.

Comment 21

[Francesco Lodolo [:flod]]

Clearing NI, going to provide a new patch and go through r/f again.

Comment 22

[Francesco Lodolo [:flod]]

Attachment: Updated PR

As Kaze suggested, using mozL10n.localize, going again through review and feedback for clarity.

Travis: https://travis-ci.org/mozilla-b2g/gaia/builds/19790607

Tested locally, switching between Italian (with a weird value set for timezoneTitle) and English in FTU, everything looks good.

Comment 23

[Preeti Raghunath(:Preeti)]

Is Italian a supported language in 1.4?

Comment 24

[Maria Angeles Oteo (:oteo)]

(In reply to Preeti Raghunath(:Preeti) from comment #23)
> Is Italian a supported language in 1.4?

Telecom Italia launched v1.1 version, so Italian should be supported for 1.4 too

Comment 25

[Francesco Lodolo [:flod]]

Preeti: besides Italian support in 1.4, this bug affects all locales, Italian was used just to do tests (bug was reported for Serbian initially).

Comment 26

[Anthony Ricaud (:rik)]

(In reply to Fabien Cazenave [:kaze] from comment #20)
> (In reply to Anthony Ricaud (:rik) from comment #16)
> > This patch is lacking new tests. It can't land as is.
> 
> This is an l10n fix, what should we test?
You should test that the code calls the l10n library appropriately.

Comment 27

[Fernando Campo (:fcampo)]

Hi Francesco, sorry I didn't see your NI on time. Maybe I can help with the tests, would you rather to give me access to your github so I send a PR to your branch, send you the code on email, or we can chat a bit on IRC and try to explain how-to.

As Rik suggests on comment 26, creating a spy on l10n.localize would be enough for the test, but sadly as you saw, there's no time&date coverage, so we'll need to create the whole new js.

Comment 28

[Anthony Ricaud (:rik)]

(In reply to Fernando Campo (:fcampo) from comment #27)
> As Rik suggests on comment 26, creating a spy on l10n.localize would be
> enough for the test, but sadly as you saw, there's no time&date coverage, so
> we'll need to create the whole new js.
Not sadly :) It will improve code coverage and make it easier for others to add tests. So yeah ! :)

Comment 29

[Fernando Campo (:fcampo)]

Comment on attachment 8383541 [details] [review]
Updated PR

Code works fine, but I recommend to add some other reviewer, as I just wrote the tests and don't feel ok with reviewing myself :D

Comment 30

[Francesco Lodolo [:flod]]

Comment on attachment 8383541 [details] [review]
Updated PR

Etienne, could you take a second look at the code?

I wrote the app change, while fcampo was nice enough to fix the lack of tests.

Comment 31

[Etienne Segonzac (:etienne)]

Comment on attachment 8383541 [details] [review]
Updated PR

r=me (with nits on github) for the tests.

Comment 32

[Francesco Lodolo [:flod]]

Thanks Etienne, comments addressed, now waiting for Travis to complete on the last push.

Comment 33

[Fabien Cazenave [:kaze]]

Comment on attachment 8383541 [details] [review]
Updated PR

LGTM

Comment 34

[Jason Smith [:jsmith]]

Is this problem present in 1.1?

Comment 35

[Brogan Zumwalt [Inactive]]

Issue occurs on 1.1, multiple languages tested.

Result:
The Continent displayed in the timezone is displayed in English.

Environmental Variables:
Device: Buri v1.1 Mozilla RIL
BuildID: 20140304041201
Gaia: c5cb252e5d1aa45d14f3a2ea182e73e2271e4496
Gecko: 1421a6b7fc51
Version: 18.0
Firmware Version: v1.2-device.cfg

Comment 36

[Jason Smith [:jsmith]]

In that case, this isn't a blocker then - we've shipped with this bug in a past partner release.

Comment 37

[Francesco Lodolo [:flod]]

Finally green Travis
https://travis-ci.org/mozilla-b2g/gaia/builds/20113094

Setting checkin-needed

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Master: 48e101917a73e228b27537d30254d276a4721dae

Comment 39

[Francesco Lodolo [:flod]]

Verified on Keon with master build 20140307, setting phone to Serbian Cyrillic.

Comment 40

[Jason Smith [:jsmith]]

This probably should be backed out - this patch caused a blocking regression.

Comment 41

[Francesco Lodolo [:flod]]

I've submitted a patch in bug 980456 to fix the problem, unfortunately I don't have control over the timing (need a reviewer and someone to merge it).