835744 - [FTE] The Date&Time window doesn't show the Country (with Movistar SIM Card or without it)

Status: VERIFIED FIXED

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

Description

[mbarone]

Attachment: error_screenshot

Pre-Condition
Device without SIM

STR
1. Run the FTE and click Next until the Date&Time window

ACTUAL RESULT
The Default Country is not displayed

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

This is polish and not a blocker, a low risk fix nominated for uplift would be considered if there was time to land it in v1.0 safely.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This occurs w/ or w/o a SIM.  Odd... I thought this was already reported?  :|

Comment 4

[Fernando Campo (:fcampo)]

Attachment: link to PR: https://github.com/mozilla-b2g/gaia/pull/7992

I undid the latest changes that Tim did, because that made the field update not to be called on init, as the listener is set after the moztimechange is done.

My tests didn't gave any error on the timing, also.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 710766 [details]
link to PR: https://github.com/mozilla-b2g/gaia/pull/7992

racing issue is always tricky. Failed to reproduce bug 826286 locally does not justify removing the code.

I agree that code should have a better place to live to workaround the init() issue. I don't agree a patch this simple.

Kaze can disagree me, though.

Comment 6

[Fabien Cazenave [:kaze]]

Comment on attachment 710766 [details]
link to PR: https://github.com/mozilla-b2g/gaia/pull/7992

We do need this `moztimechange' event listener. I remember Tim has added it to solve a race condition, and the fix has proven to be efficient.

I expected to keep both this event listener *and* call `onchange(tz)' on first startup.

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Noming this bug due to comment 8 in bug 8357144

https://bugzilla.mozilla.org/show_bug.cgi?id=835744c8

Comment 9

[Alex Keybl [:akeybl]]

Not a blocker, but we'll consider for tracking.

Comment 10

[Daniel Coloma:dcoloma]

With a Movistar SIM Card (Target Market) or with no SIM Card inserted I get the same effect:

https://bug835744.bugzilla.mozilla.org/attachment.cgi?id=707531

The continent label is not shown and I have a selector field that I do not know what its purpose is. 

This is the First Time Experience, the First Impression a customer has of the device,  having such a visible bug in the 4th screen every user sees is not acceptable. Nominating again.

Comment 11

[Fernando Campo (:fcampo)]

Comment on attachment 710766 [details]
link to PR: https://github.com/mozilla-b2g/gaia/pull/7992

Updated code, hence asking for review again.

After :timdream comments I made some more tests and finally reproduced that race condition you guys were talking about. But the problem we have is basically:

- If we only update the fields from the listener (original), nothing happens on init, so user will see empty fields when no SIM or no NITZ service available.
- If we only do it explicitly (1st patch), we ignore the moztimechange events, so even when SIM and NITZ working, we won't change them automatically.
- Now, this second version of the patch use both ways to update the labels, which works fine on init and on moztimechange events, but in case of user interaction (change timezone with button clicks) then we update the fields twice.

As this has no effect on user experience, and is only a matter of duplicate actions, I think is the less-evil scenario right now.

Please comment if you think I'm missing something.

Comment 12

[Fernando Campo (:fcampo)]

Comment on attachment 710766 [details]
link to PR: https://github.com/mozilla-b2g/gaia/pull/7992

Updated code, hence asking for review again.

After :timdream comments I made some more tests and finally reproduced that race condition you guys were talking about. But the problem we have is basically:

- If we only update the fields from the listener (original), nothing happens on init, so user will see empty fields when no SIM or no NITZ service available.
- If we only do it explicitly (1st patch), we ignore the moztimechange events, so even when SIM and NITZ working, we won't change them automatically.
- Now, this second version of the patch use both ways to update the labels, which works fine on init and on moztimechange events, but in case of user interaction (change timezone with button clicks) then we update the fields twice.

As this has no effect on user experience, and is only a matter of duplicate actions, I think is the less-evil scenario right now.

Please comment if you think I'm missing something.

Comment 13

[Alex Keybl [:akeybl]]

(In reply to Daniel Coloma:dcoloma from comment #10)
> With a Movistar SIM Card (Target Market) or with no SIM Card inserted I get
> the same effect:
> 
> https://bug835744.bugzilla.mozilla.org/attachment.cgi?id=707531
> 
> The continent label is not shown and I have a selector field that I do not
> know what its purpose is. 
> 
> This is the First Time Experience, the First Impression a customer has of
> the device,  having such a visible bug in the 4th screen every user sees is
> not acceptable. Nominating again.

TEF considers this a critical blocker, we'll look for new reviewers and try to land this week.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 710766 [details]
link to PR: https://github.com/mozilla-b2g/gaia/pull/7992

Calling the callback twice sounds bad. I think you want to find the root cause of the issue. Could it be that the default timezone is already set to the same value so there is no timezone change event?

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

:fcampo, I re-read the code again: isn't the init process rely on setTimezoneDescription() call at L122 to fill the default value?

https://github.com/fcampo/gaia/blob/38e33aa/js/tz_select.js#L122

Why does anything in the updateTZ() function have anything to do with init process?

Do we have another race condition where webL10n is not loaded but updateTZ() is called? Why would updateTZ() be called without user interaction on TZSelector?

Comment 16

[Fernando Campo (:fcampo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #15)
> :fcampo, I re-read the code again: isn't the init process rely on
> setTimezoneDescription() call at L122 to fill the default value?
Yes, it changes the region and city description, but not the labels (that is change on the callback onchange()).

After making some traces I see that :vingtetun was totally right, timezone change is not triggered cause we are setting the value to the same value that was saved in there (even after a reset-gaia, the setting is kept from last run), and that doesn't trigger a timezonechange event. Not a bug, probably, as it's specific code on gecko (http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/TimeZoneSettingObserver.cpp#139) [thanks :gwagner].

One fix would be to null the setting time.timezone on the startup of FTE, so then the default value (Pago_Pago) will be set and the UI updated properly. For this I have to change some code from :etienne to reset the timezone on initial call for tzSelect
I don't think that using default value on first run init would be problematic (as in case NITZ is working this should be changed after), but if anyone have any issues about it just comment in here.

Another posible fix would be to listen to the mozsettingchange that is triggered when setting time.timezone, and update UI then, but would take us back to the situation where we update twice in case of user interaction with the selectors.

So right now, as I think first option is a good fix, I'm updating my patch with that code and asking for review again.

Comment 17

[Fernando Campo (:fcampo)]

Attachment: link to PR https://github.com/mozilla-b2g/gaia/pull/8215

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 716047 [details]
link to PR https://github.com/mozilla-b2g/gaia/pull/8215

Tim's review will be enough.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 716047 [details]
link to PR https://github.com/mozilla-b2g/gaia/pull/8215

With this fix, FTU will overwrite timezone setting set during the build process. Also, you are breaking both the landing rule (bug # on comment) and unit tests.

Can you properly fix the tz_select.js so that it will update the label w/o trigger the callback during init?

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 716047 [details]
link to PR https://github.com/mozilla-b2g/gaia/pull/8215

Right, unit test breakage is unrelated to this bug.

Comment 21

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I am going to steal this one to have a look.

Comment 22

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8239

Pointer to Github pull-request

Comment 23

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 716530 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8239

Patch V.1

1. Now it will have 2 paths for the FTU to update the country labels,
   1.a. init - onload callback will be invoked for the client code (FTU) to update the country labels when it invoke tzSelect().
      onload callback is provided separately for more flexibility.
 
   1.b. timechange - as the original path.

2. (Another issue) Modify FTU part to listen to region <select> change, so that it will upate the label as well.

Note: The only thing I am not sure is in the init path, whether we need to change time zone settings for the first. originally, it would, but after aeb356fa352378c12774817feaf2672e07ed45bc change, it will never do this.

Thanks.

Comment 24

[Fernando Campo (:fcampo)]

Comment on attachment 716530 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8239

Thanks for the help :rudyl, I was getting blocked on this.

And even when I like the onload callback approach, this patch raises a new issue, which is not changing the city (and in consequence, not changing timezone and map overlay) when a new region is selected.

I made some comments on github

Comment 25

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 716530 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8239

Pull request updated to address the issues:
  1. As :fcampo mentioned above, now if you change the region, we will change the city to the first one in that region and the UI will be updated.
  2. Also check settings > Date & Time to sync up with FTU part.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 716530 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8239

thanks!

Comment 27

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Merged on Gaia mater,
https://github.com/mozilla-b2g/gaia/commit/e2d687965dfb99e03378c28b56a546de496e504a

Dear Tim, :fcampo,

Thanks for the review.

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train@a5ae703
v1.0.1@2f97f29

Comment 29

[isabelrios]

Testrun 5.1. Test case  #2368

Comment 30

[mdaniloff]

Verified fixed on Unagi:
Unagi Build ID: 20130322070203
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4931ec89ebbe
Gaia: 85fd164691bb852f1cfaf82405df4380629ced6e