1057977 - [Status Bar] Remove AM/PM indicator

Status: VERIFIED FIXED

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

Description

[Rob MacDonald [:robmac]]

As per the UX and visual specs, remove the AM/PM indicator in the status bar clock to allow for more space for indicators. 

Although few concerns have been raised so far, retain an option to allow partners or mozilla to easily restore AM/PM in developer settings in the event this needs to be restored.

Comment 1

[Guillaume Marty [:gmarty]]

Adding Bug 1043381 as a dependency because I couldn't decide whether that's a duplicate or not.

Comment 2

[Kevin Grandon :kgrandon]

(In reply to Rob MacDonald [:robmac] from comment #0)
> Although few concerns have been raised so far, retain an option to allow
> partners or mozilla to easily restore AM/PM in developer settings in the
> event this needs to be restored.

How are we defining icon priority for 2.1? It seems like all priorities should be customizable by the OEM, and that we should not be using settings. Perhaps some JSON config file?

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Kevin Grandon :kgrandon from comment #2)
> (In reply to Rob MacDonald [:robmac] from comment #0)
> > Although few concerns have been raised so far, retain an option to allow
> > partners or mozilla to easily restore AM/PM in developer settings in the
> > event this needs to be restored.
> 
> How are we defining icon priority for 2.1? It seems like all priorities
> should be customizable by the OEM, and that we should not be using settings.
> Perhaps some JSON config file?

The spec is here:
https://bug1042105.bugzilla.mozilla.org/attachment.cgi?id=8476853

And in code, the priorities are defined here:
https://github.com/mozilla-b2g/gaia/blob/d281d90c3679d513627e17c0f4dcf9e895c9b47d/apps/system/js/statusbar.js#L37

A json config file wouldn't be too difficult to add as an operator config option. We discussed this in Berlin and decided it was not necessary for 2.1, but may come in the future.

Comment 4

[Kevin Grandon :kgrandon]

(In reply to Michael Henretty [:mhenretty] from comment #3)
> The spec is here:
> https://bug1042105.bugzilla.mozilla.org/attachment.cgi?id=8476853
> 
> And in code, the priorities are defined here:
> https://github.com/mozilla-b2g/gaia/blob/
> d281d90c3679d513627e17c0f4dcf9e895c9b47d/apps/system/js/statusbar.js#L37
> 
> A json config file wouldn't be too difficult to add as an operator config
> option. We discussed this in Berlin and decided it was not necessary for
> 2.1, but may come in the future.

Right - it just seems like we should rip that content into a customizable json file first instead of creating a setting for the am/pm label if we want this content to be operator controlled.

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Kevin Grandon :kgrandon from comment #4)
> Right - it just seems like we should rip that content into a customizable
> json file first instead of creating a setting for the am/pm label if we want
> this content to be operator controlled.

I don't agree since it's not the same thing. Lowering the priority doesn't turn off AM/PM entirely, and in fact AM/PM will still display mostly in the expanded Rocketbar state since it is one of the icons, like battery and time, which is always displays if space is available. This will lead to all the icons shifting when opening and closing the rocketbar, and in this case I'd personally like to have the option to turn it off.

In any case, there are two pieces of potential work to do here. 
One, separate and prioritize AM/PM indicated differently from time (Bug 1043381).
Two, allow AM/PM indicator to be disabled with a setting (this bug).

If we do the first option, we will also have to move the icon priorities into an operator configurable json file. This is something may do down the road anyway, but we would have to do it now to solve the AM/PM spacing issue and address potential operator concerns.

Rob, can you comment on which piece is more important from a UX perspective?

Comment 6

[Kevin Grandon :kgrandon]

I still think that a setting is overkill for this. You can easily extend the current priorities array to control whether things show up in the first place and keeping everything together in one place is a big win. Splitting these up into different control mechanisms is going to cause a lot of headache for the long run.

Comment 7

[Rob MacDonald [:robmac]]

Let me clarify the requirement first. The AM/PM indicator shouldn't be prioritized separately from the time. So we shouldn't have scenarios where sometimes AM/PM shows and sometimes it doesn't. 

Also, the setting isn't meant to be a user-facing setting in 2.1. This is something that is intended for configuration by partners or mozilla if we have to reintroduce the AM/PM indicator.

Does this help at all?

Comment 8

[Guillaume Marty [:gmarty]]

Attachment: Github WIP patch

Before going further with code cleaning, I'd like Rob to have a look.

Apply the patch, Flash and go to Settings > Date and Time. The flag is at the bottom of the page. This may not refresh directly, so change Time Format back and forth to retrigger a status bar refresh.

Please have a look and let me know if it looks OK.

Comment 9

[Rob MacDonald [:robmac]]

Comment on attachment 8489345 [details] [review]
Github WIP patch

Hi Guillaume...

Sorry - Minusing based on the following

- As per comment 7, there should not be a user-facing setting for this. Instead this is something that's intended to be configured by partners or Mozilla if we have to reintroduce the indicator.

- The setting doesn't always seem to work. Although initially I was able to toggle it on and off, after a few tries it stopped working.

- The spacing of the AM/PM indicator relative to the time seems a bit off so flagging Eric on this.

Thanks!
Rob

Comment 10

[Eric Pang [:epang] UX]

(In reply to Rob MacDonald [:robmac] from comment #9)
> Comment on attachment 8489345 [details] [review]
> Github WIP patch
> 
> Hi Guillaume...
> 
> Sorry - Minusing based on the following
> 
> - As per comment 7, there should not be a user-facing setting for this.
> Instead this is something that's intended to be configured by partners or
> Mozilla if we have to reintroduce the indicator.
> 
> - The setting doesn't always seem to work. Although initially I was able to
> toggle it on and off, after a few tries it stopped working.
> 
> - The spacing of the AM/PM indicator relative to the time seems a bit off so
> flagging Eric on this.
> 
> Thanks!
> Rob

Thanks Rob, the gap is a little wide, Guillaume are you following this spec?  I think what's implemented is what is currently used.  The space should be 0.333rem (or 0.5rem on flame)
Here's a link to the spec: https://mozilla.box.com/s/ekd7l17a1rxs2su8lgry

Thanks!

Comment 11

[Guillaume Marty [:gmarty]]

Comment on attachment 8489345 [details] [review]
Github WIP patch

Thanks Rob for the feedback.
The space between the time and the AM/PM element can't be styled by CSS. It is a typographic space using the same font size than the time. Doing any kind of processing in JavaScript might have some impact on other locales. We can remove the space though, but this is something that needs to be discussed with the l10n guys and tested in all locales.
Regarding the intermittent working issue, tough I haven't been able to replicate, this won't happen when we remove the user facing settings.

Vivien, can you please review this code?

Comment 12

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

Comment on attachment 8489345 [details] [review]
Github WIP patch

I'm not the best one to review this change. Redirecting to Tim so he can found someone who makes more sense.

Comment 13

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

Comment on attachment 8489345 [details] [review]
Github WIP patch

* You need test.
* adding span at L927 and only to remove it in an |if| block in L929 does not really make sense.
* Why is the pull request comes with "Do not merge" description?

Comment 14

[Guillaume Marty [:gmarty]]

Sorry I was hoping to fix the failing test before getting it reviewed but didn't have time.

I tried removing both the |%p| placeholder and the span but they both create an issue in shared/js/l10n_date.js. Still working on that. Will ping you when ready for review.

Comment 15

[Guillaume Marty [:gmarty]]

Comment on attachment 8489345 [details] [review]
Github WIP patch

OK, this time I'm ready to get reviewed. I fixed the failing test and add a new unit test for the feature.

Comment 16

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

Comment on attachment 8489345 [details] [review]
Github WIP patch

Looks good, thanks, but I am not sure you have addressed everything Rob asked in ui-review.

Comment 17

[Guillaume Marty [:gmarty]]

Thanks for the review. We discussed this with Rob and Eric offline. In a nutshell:

> - As per comment 7, there should not be a user-facing setting for this.
> Instead this is something that's intended to be configured by partners or
> Mozilla if we have to reintroduce the indicator.

This is now only activable via a settings (no dedicated UI is available)

> - The setting doesn't always seem to work. Although initially I was able to
> toggle it on and off, after a few tries it stopped working.

I couldn't replicate that but I suspect it was related to the way activate/deactivate it was implemented in the Settings app.

> - The spacing of the AM/PM indicator relative to the time seems a bit off so
> flagging Eric on this.

This is a typographic space so we can't style it precisely. Also it may or may not be present depending on the locale.

Comment 18

[Guillaume Marty [:gmarty]]

Landed in https://github.com/mozilla-b2g/gaia/commit/8756efc2d509f85d4ae8af6ff57d9a7ac93952a3

Comment 19

[Michael Henretty [:mikehenrty][:mhenretty]]

Guillaume, this is something UX wants for 2.1, since they really care about statusbar space. Please request uplift.

Comment 20

[Guillaume Marty [:gmarty]]

Comment on attachment 8489345 [details] [review]
Github WIP patch

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Status bar icons prioritisation
[User impact] if declined: Less good UI, some icons of importance may be hidden with this feature unimplemented/deactivated.
[Testing completed]: Manual testing
[Risk to taking this patch] (and alternatives if risky): Very low risk
[String changes made]: None

Comment 21

[bhavana bajaj [:bajaj]]

Need confirmation with a ui-review+ before uplift. Rob can you please help here to review the updated patch ?

Comment 22

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8489345 [details] [review]
Github WIP patch

See comment 21.

Comment 25

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8489345 [details] [review]
Github WIP patch

Or Eric.

Comment 26

[Eric Pang [:epang] UX]

Comment on attachment 8489345 [details] [review]
Github WIP patch

The patch seems to be working great, only one thing that we need to clarify with Rob before uplift.  I believe the AM/PM should be off by default instead of on because of his comment in the description: 

"Retain an option to allow partners or mozilla to easily restore AM/PM"

R+, but wait on Rob's confirmation before uplift.  Thanks!

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

Yes, this was supposed to be off by default, that's the whole point of this bug. If that is not the case this needs to be fixed on master and 2.1.

Comment 28

[Rob MacDonald [:robmac]]

(In reply to Michael Henretty [:mhenretty] from comment #27)
> Yes, this was supposed to be off by default, that's the whole point of this
> bug. If that is not the case this needs to be fixed on master and 2.1.

That's correct. Thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/c6794d2ce389a3c2c4ee1e77264902e17153b14f

Comment 30

[Guillaume Marty [:gmarty]]

Attachment: Github PR for feature activation

Comment 27 makes it clear that this feature must be enable by default. This patch landed in master as of https://github.com/mozilla-b2g/gaia/commit/d1be4a677b80649da3fdc40fec953278eb1a74b8
But still needs to be uplifted. Ryan can you land this patch in v2.1?

Comment 31

[Michael Henretty [:mikehenrty][:mhenretty]]

Guillaume, when you land code make sure you put it in the bug. In this case, it would have been better to file a follow-up bug to fix the AM/PM default since we have to do the uplift process anyway. As it is, it's in a confusing state right now.

Patch to fix AM/PM default:
https://github.com/mozilla-b2g/gaia/commit/9f53a4d4eba18a7306b7b7748916400c4b84857c

Comment 32

[Ryan VanderMeulen [:RyanVM]]

The second patch has to go through the approval process also AFAIK. Once it has approval, it'll appear in the regular bug queries we use.

Comment 33

[Guillaume Marty [:gmarty]]

Comment on attachment 8498316 [details] [review]
Github PR for feature activation

Thanks Ryan for the clarification.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Status bar icons prioritisation. This patch enables the feature already uplifted (see Comment 29)
[User impact] if declined: This feature won't be activated and the status bar icons won't be optimised
[Testing completed]: Manual testing required
[Risk to taking this patch] (and alternatives if risky): None
[String changes made]: None

Comment 34

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/5d71e6b568cfe6d923936ff19535da3eed74afb1

Comment 35

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.2 and Flame 2.1.

Result: AM/PM indicator on the status bar is removed.

Flame 2.2

Environmental Variables:
Device: Flame Master (319mb)(KitKat Base)(FullFlash)
Build ID: 20141024040202
Gaia: d893a9b971a0f3ee48e5a57dca516837d92cf52b
Gecko: a5ee2769eb27
Version: 36.0a1 (Master)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141024001204
Gaia: 0f76e0baac733cca56d0140e954c5f446ebc061f
Gecko: 7d78ff7d25b6
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0