943104 - [B2G][Clock][Alarm]App lists Monday as first day of week when choosing days for alarm to go off.

Status: VERIFIED FIXED

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

Description

[Ross Kuhlman ([e-mail])]

Attachment: Alarm Clock App starts with Monday

When the user is setting an alarm using the Clock app, they may choose which days of the week to trigger the alarm. The list of days starts with Monday and ends with Sunday. This is inconsistent with other apps, which list the days starting with Sunday and ending with Saturday.

Repro Steps:
1) Updated Buri to Build ID: 20131125004001
2) Launch Clock App.
3) Tap the bell icon in the upper right corner to create a new alarm.
4) Tap on the 'Repeat' droplist.
5) Observe droplist.

Actual:
First day listed is Monday.

Expected:
First day listed is Sunday. (This is consistent with other apps)

Environmental Variables
Device: Buri v1.2 Moz RIL
Build ID: 20131125004001
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/368ea26d2136
Gaia: c2dea53b36bb9d4331a94976344515f60dc5a3d4
Platform Version: 26.0
RIL Version: 01.02.00.019.102 
Firmware Version: V1.2_20131115

Notes:
Repro frequency: 100%
See attached screenshot.

Comment 1

[Ross Kuhlman ([e-mail])]

Attachment: this is how other Apps display the week.

Comment 2

[Ross Kuhlman ([e-mail])]

Issue also occurs in 1.1

Comment 3

[Vishnu Dangi]

i am working in this bug.

Comment 4

[Marcus Cavanaugh [:mcav]]

Hi Vishnu, I'm doing some bug cleanup -- are you still working on this bug?

Comment 5

[Vishnu Dangi]

i finished it but you can cleanup.

Comment 6

[Marcus Cavanaugh [:mcav]]

Cool, could you post a link to your patch or a pull request?

Comment 7

[Vishnu Dangi]

Attachment: Pointer to Pull Request.html

Hi,
I have made a pull request at
https://github.com/mozilla-b2g/gaia/pull/15584
Please review it.

Thank you.

Comment 8

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8363522 [details]
Pointer to Pull Request.html

Thanks for the patch! Unfortunately, we actually need to go a little further than just a constant change to the weekday start. There's an i18n variable called 'weekStartsOnMonday' (a string '0' or '1') which might change depending on the locale. See <https://github.com/mozilla-b2g/gaia/pull/7620/files> for a somewhat old PR that did that.

So we'll need to do something that checks the 'weekStartsOnMonday' i18n variable, and change the display based on that. We should be able to just change the order in the DOM and leave the other calculating logic the same.

Does that make sense?

Comment 9

[Vishnu Dangi]

if we not change in Index.html then Monday is display first and Sunday is last.so for displaying Monday is first i think it is necessary.

Comment 10

[Marcus Cavanaugh [:mcav]]

Attachment: Gaia PR

Comment 11

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8364176 [details]
Gaia PR

https://github.com/mozilla-b2g/gaia/pull/15632#issuecomment-33100657 Comments on GH!

Comment 12

[Mike Pennisi [:jugglinmike]]

I've added a few comments to the pull request, as well.

Comment 13

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8364176 [details]
Gaia PR

Hey Gareth and Mike,

I talked to Gareth about this a bit during our workweek last week.
Long story short, right now I'm assigned to three very similar bugs
dealing with localization, so I felt it would be less error-prone to
just fix it all in one go. I think it's a reasonable change to
coalesce.

Part of that involved slightly refactoring and simplifying the l10n
shims we use, because I found that when testing I had a hard time
following which shim was used for what purpose.

Here's what I've changed, and if you disagree with this we can talk
about it:

- I've changed all references of a RequireJS-loaded 'l10n' library
  with direct references to `navigator.mozL10n`. My rationale is that
  because it's hoisted on `navigator`, it makes sense to treat it as a
  browser-supplied library for the purposes of dealing with
  localization. This simplifies code needing to use localization, in
  that it can always assume `navigator.mozL10n` will be present.

- I've removed the 'l10n' and 'navigator_l10n' library fake shims from
  test/unit/, replacing them with a brief static shim in `setup.js`
  for unit tests. Previously, it was confusing as to which l10n shim
  was being used during testing; in line with my rationale above, it
  seems simpler to just assume that we have `mozL10n` available
  always, and it's shimmed appropriately for unit tests.

- Note for Gareth: Disregard my comments from the workweek about using
  the real `navigator.mozL10n`; that's no longer applicable because I
  ended up using the static "fake" responses we previously used for
  returning strings from mozL10n.get(). In other words, we never look
  at the real translated strings in tests, so we don't have to worry
  about translators' changes messing anything up.

- Also note for Gareth: I added unit tests for this bug's case; see
  `alarm_edit_test.js` line 233.

- Mike has refactoring out in bug 922128, which should land after the
  integration tests in bug 907177 land pending Gareth's review. I'm OK
  with waiting to land this until after the integration tests land and
  then adding integration tests for this before finally landing this.

Comment 14

[Marcus Cavanaugh [:mcav]]

Also, potentially relevant: The other bugs I mentioned, which this
patch also fixes: bug 913409 and bug 949082.

Comment 15

[Mike Pennisi [:jugglinmike]]

Hey Marcus,

I understand your rationale for using a global reference instead of an explicit dependency, but I don't understand your motivation. Using the global definitely simplifies the code (since it has one less call to `require`), but the same argument could be made for any module. (One of the biggest benefits of AMD is the explicit management of dependencies, after all.) You also mention confusion about the l10n stub in the unit test environment. Can you elaborate on that a bit? Ideally, I would like to find some solution that simplifies the test setup but does not introduce a new global variable.

Comment 16

[Marcus Cavanaugh [:mcav]]

The shared l10n library automatically hoists itself onto navigator.mozL10n (whether we like it or not), so it's not creating another global, it's just reusing one that was already there.

The confusion with the test setup was mainly because there were a couple of different l10n references -- those to navigator.mozL10n, which was shimmed, and also to the l10n library itself, which was also shimmed. It seems unnecessary to have both, when we could just reuse the existing one that the shared library provides.

Comment 17

[Marcus Cavanaugh [:mcav]]

To elaborate slightly more, I see the `navigator.mozL10n` interface as something system-wide, system-provided; the infrastructure supporting the localization was really designed to be something the entire system can take advantage of. To me, the fact that they hoisted mozL10n on navigator implies the same -- just as with any other API like mozAlarms, mozPower, etc. If, at some point, mozL10n becomes fully baked in (as the other navigator APIs are), having a completely separate loaded module would just be duplication.

If they didn't hoist mozL10n on navigator and returned it as a normal module, obviously it wouldn't make sense to add another global just for this. But because the l10n library creates that reference no matter what, I see no reason to not take advantage of it. It eliminates the confusion of "why are there references to navigator.mozL10n when there's this shared l10n library too?"; similarly, shimming both is redundant.

From my perspective, if "l10n" is going to provide `navigator.mozL10n` no matter what, it complicates things to _not_ use it. The fact that it's in shared is just an implementation detail -- that it isn't built in to B2G by default.

If they decided to change their minds and treat l10n as a library in B2G apps, then I'd probably advocate for the other way around -- but also getting rid of any references to `navigator.mozL10n` in unit tests as well.

Comment 18

[Mike Pennisi [:jugglinmike]]

I don't think it's accurate to call the `mozL10n` API system-provided because the application has to explicitly include a JavaScript file before it can be accessed. This requirement is why I believe we should continue to treat mozL10n as a JavaScript library.

While the global exists either way, using it would undermine the AMD pattern--the dependency would become implicit, and you would have to read through a module to know if it requires the library in order to function.

When the platform natively exposes `mozL10n.get` and `mozL10n.DateTimeFormat` on the `navigator` object, this dependency can be taken for granted. But until then, module that rely on the API will be incomplete without requiring the script.

Another solution comes to mind: we could update the `l10n.js` and `l10n_date.js` libraries in `shared/` to export their API using the UMD pattern--then we would not have to shim the dependencies in Clock. I think that e-mail would also benefit from this change. I'd be happy to do the leg-work for this--just let me know if it sounds good to you.

Comment 19

[Marcus Cavanaugh [:mcav]]

Okay, you convinced me. I'll update the PR to revert most of the changes referring to navigator, and further I'll try to make sure we don't have any unnecessary references to navigator.mozL10n.

Your idea about AMDifying the shared/ module seems good, though I don't particularly want to tackle that right now :-) if you want to.

My main beef is that it should be one way or the other -- and you've convinced me to go all-AMD for this and try to ignore the exported global unless there comes a day when it's actually built-in without extra JS requirements.

Comment 20

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8364176 [details]
Gaia PR

Canceling :gaye's review, will probably pass it to Mike when I've reverted some of the l10n changes.

Comment 21

[Mike Pennisi [:jugglinmike]]

(In reply to Marcus Cavanaugh [:mcav] <[e-mail]> from comment #19)
> Your idea about AMDifying the shared/ module seems good, though I don't
> particularly want to tackle that right now :-) if you want to.


On it! See bug 968443

Comment 22

[Marcus Cavanaugh [:mcav]]

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

Okay, here's the cleaned-up patch, much easier to digest. :-)

The only thing I changed with mozL10n is to remove the "navigator.mozL10n" shim from tests; it appears it was unused anyway. Unit tests pass.

Comment 26

[Gareth Aye [:gaye] (back from PTO)]

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

Reflag when ready for re-re-review :)... comments on GH

Comment 27

[Mike Pennisi [:jugglinmike]]

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

Hey Marcus,

Looks like we have some integration test failures. Let me know if you'd like help debugging them!

Comment 28

[Marcus Cavanaugh [:mcav]]

Yeah, I don't think I accounted for your new integration tests when I wrote the latest changes. I'll update and re-flag both you and Gareth when these are ready.

Comment 29

[Julien Wajsberg [:julienw]]

(thanks for using the l10n value and not blindly changed to Sunday ;) )

Comment 30

[Marcus Cavanaugh [:mcav]]

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

Okay Gareth and Mike, both the Marionette and unit tests (and Travis) pass. Could you take another look?

One small note -- Mike, in the wait() we have for hitting the Lap button, my computer still needed a nudge extra time. Maybe there's a touch of latency somewhere along the chain that causes the lap to not register after we issue a .wait(11) call; I changed it to wait for 20ms and it works properly for me now. I think that should be fine, but wanted to clarify why I changed that value. I'm running it on a Mac Pro which could explain the speed discrepancy.

Comment 31

[Mike Pennisi [:jugglinmike]]

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

Looks good to me!

(In reply to Marcus Cavanaugh [:mcav] <[e-mail]> from comment #30)
> Comment on attachment 8371102 [details]
> Link to Github pull-request:
> https://github.com/mozilla-b2g/gaia/pull/15632/files
> 
> One small note -- Mike, in the wait() we have for hitting the Lap button, my
> computer still needed a nudge extra time. Maybe there's a touch of latency
> somewhere along the chain that causes the lap to not register after we issue
> a .wait(11) call; I changed it to wait for 20ms and it works properly for me
> now. I think that should be fine, but wanted to clarify why I changed that
> value. I'm running it on a Mac Pro which could explain the speed discrepancy.

This really should be hardware independent: `client.helper.wait` is implemented via a call to `setTimeout` in the context of the application. I'm thinking this may actually be a bug in the implementation of `start`. I'll follow up with you on a separate bug because I won't be able to verify my fix.

Comment 32

[Marcus Cavanaugh [:mcav]]

Landed in master:

https://github.com/mozilla-b2g/gaia/commit/dfc094b8485f526a6a2a58ef90c172ea11b7fd88

Comment 33

[Edward Chen[:edchen]]

[Verified info]
Gaia      6e71ab4da1b08586ea0c758edb7aa199ee34cd2f
Gecko     https://hg.mozilla.org/mozilla-central/rev/bb030d47c946
BuildID   20140219160202
Version   30.0a1

Bug has fixed, I mark it to VERIFIED.