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

VERIFIED FIXED

Status

Firefox OS
Gaia::First Time Experience
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mbarone, Assigned: rudyl)

Tracking

(Blocks: 1 bug)

unspecified
x86
Mac OS X

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: testrun 5.1)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 707531 [details]
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
Duplicate of this bug: 835745
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.
blocking-b2g: tef? → -
tracking-b2g18: ? → -
This occurs w/ or w/o a SIM.  Odd... I thought this was already reported?  :|
Summary: [FTE] The Date&Time window does't show the Coutry - Device without SIM → [FTE] The Date&Time window doesn't show the Country
Created attachment 710766 [details]
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.
Assignee: nobody → fernando.campo
Status: NEW → ASSIGNED
Attachment #710766 - Flags: review?(timdream)
Attachment #710766 - Flags: review?(kaze)
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.
Attachment #710766 - Flags: review?(timdream) → review-
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.
Attachment #710766 - Flags: review?(kaze) → review-
Duplicate of this bug: 837800
Noming this bug due to comment 8 in bug 8357144

https://bugzilla.mozilla.org/show_bug.cgi?id=835744c8
blocking-b2g: - → tef?

Comment 9

5 years ago
Not a blocker, but we'll consider for tracking.
blocking-b2g: tef? → -
tracking-b2g18: - → ?
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.
blocking-b2g: - → tef?

Updated

5 years ago
Summary: [FTE] The Date&Time window doesn't show the Country → [FTE] The Date&Time window doesn't show the Country (with Movistar SIM Card or without it)
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.
Attachment #710766 - Flags: review?
Attachment #710766 - Flags: review-
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.
Attachment #710766 - Flags: review?(timdream)
Attachment #710766 - Flags: review?(kaze)
Attachment #710766 - Flags: review?
(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.
blocking-b2g: tef? → tef+
tracking-b2g18: ? → ---
Attachment #710766 - Flags: review?(21)
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?
Attachment #710766 - Flags: review?(timdream)
Attachment #710766 - Flags: review?(kaze)
Attachment #710766 - Flags: review?(21)
Attachment #710766 - Flags: review-
: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?
(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.
Created attachment 716047 [details]
link to PR https://github.com/mozilla-b2g/gaia/pull/8215
Attachment #710766 - Attachment is obsolete: true
Attachment #716047 - Flags: review?(timdream)
Attachment #716047 - Flags: review?(21)
Comment on attachment 716047 [details]
link to PR https://github.com/mozilla-b2g/gaia/pull/8215

Tim's review will be enough.
Attachment #716047 - Flags: review?(21)
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?
Attachment #716047 - Flags: review?(timdream) → review-
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.
I am going to steal this one to have a look.
Assignee: fernando.campo → rlu
Created attachment 716530 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8239

Pointer to Github pull-request
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.
Attachment #716530 - Flags: review?(timdream)
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
Blocks: 843817
Attachment #716530 - Flags: review?(timdream)
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.
Attachment #716530 - Flags: review?(timdream)
Comment on attachment 716530 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8239

thanks!
Attachment #716530 - Flags: review?(timdream) → review+
Merged on Gaia mater,
https://github.com/mozilla-b2g/gaia/commit/e2d687965dfb99e03378c28b56a546de496e504a

Dear Tim, :fcampo,

Thanks for the review.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
v1-train@a5ae703
v1.0.1@2f97f29
status-b2g18: --- → fixed
status-b2g18-v1.0.1: --- → fixed

Comment 29

5 years ago
Testrun 5.1. Test case  #2368
Whiteboard: testrun 5.1

Comment 30

5 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.