When user is charging their phone, we should show them the remaining time until the phone is charged.

That's a screenshot from Flame running my branch. wrt. the code, I'd like to make sure that we don't have the event listener on while the screen is off to limit CPU wakeups. It also seems that currently, at least on Flame, we battery.chargingTime is always Infinite. We'll need to fix that before we add this feature.

Hi Zibi, thanks for your patience with this! :). I've talked with Amy and we looked at all possible scenarios on the lockscreen. Thanks for the patch but unfortunately it won't work because the space is taken up by notifications and/or the music player. I've spec'ed out adding the charging string under the date. But also note that I've also changed the vertically spacing of the time and date since the notifications were too close to the string. Is adjusting the layout of these elements something that's easy to change or would this cause problems? Thanks and please let me know if you have any questions!

Updated the spec to include the % since it will no longer appear in the status bar.

(In reply to Eric Pang [:epang] from comment #3) > Created attachment 8546019 [details] > Lockscreen_charging_spec.jpg > > Updated the spec to include the % since it will no longer appear in the > status bar. Thanks for the spec! I like it a lot and will be happy to hack it :) Two questions: 1) We do not have the charging estimation (waiting for :dliang in bug 1116368). Does it make sense to implement the spec partially for now to just show "59% Charged" until we have the remaining piece? 2) One of the UX flows I had in mind while planning it was the scenario similar to the charging one, when the user is looking at an unlocked phone and he wants to learn battery charge status. I can imagine this happening commonly in two user stories: a) User is charging the phone, but his screen is unlocked as he is browsing the web or chatting. He'd like to check how much of the battery is charged. b) User is not charging the phone. He'd like to quickly learn his battery status to estimate how long will the battery will hold. In my initial approach based on how iOS/Android solve that, I was thinking about teaching the user that the battery percentage is an additional bit of information available next to the battery icon in certain modes (lockscreen and notification unfolded). With your spec this will not be possible. Do you plan to address the flow to allow the user to quickly check battery status in such cases? I don't have a strong opinion here, I'm just curious if you think it's something worth exploring later and we may end up wanting to show battery percentage on the statusbar in some cases? If you think it's worth getting partial implementation, I'll update my patch tomorrow and will try to get it into 2.2 :)

(In reply to Zibi Braniecki [:gandalf] from comment #4) > (In reply to Eric Pang [:epang] from comment #3) > > Created attachment 8546019 [details] > > Lockscreen_charging_spec.jpg > > > > Updated the spec to include the % since it will no longer appear in the > > status bar. > > Thanks for the spec! I like it a lot and will be happy to hack it :) > > Two questions: > > 1) We do not have the charging estimation (waiting for :dliang in bug > 1116368). Does it make sense to implement the spec partially for now to just > show "59% Charged" until we have the remaining piece? This works for me > > 2) One of the UX flows I had in mind while planning it was the scenario > similar to the charging one, when the user is looking at an unlocked phone > and he wants to learn battery charge status. > > I can imagine this happening commonly in two user stories: > > a) User is charging the phone, but his screen is unlocked as he is browsing > the web or chatting. He'd like to check how much of the battery is charged. > > b) User is not charging the phone. He'd like to quickly learn his battery > status to estimate how long will the battery will hold. > > In my initial approach based on how iOS/Android solve that, I was thinking > about teaching the user that the battery percentage is an additional bit of > information available next to the battery icon in certain modes (lockscreen > and notification unfolded). > > With your spec this will not be possible. Do you plan to address the flow to > allow the user to quickly check battery status in such cases? This all comes down to the real estate of the status bar in 2.2. With the Rocket bar in there as well we already don't have enough room for all icons so it wouldn't make sense to squish the % in there. But in the future if this changes we can consider something like this. I'll make sure to speak with Rob about it when we start exploring future versions. > > I don't have a strong opinion here, I'm just curious if you think it's > something worth exploring later and we may end up wanting to show battery > percentage on the statusbar in some cases? > > If you think it's worth getting partial implementation, I'll update my patch > tomorrow and will try to get it into 2.2 :) Let's give a try and see how it looks implemented, flag me for ui-review when ready :). Thanks again for all your work on this Zibi!

Alive, Can you take a look at that? I need to add tests, but it seems to work well. I'd like to make sure that I hooked up the code properly. Also, if you can help me verify if I conformed to the spec CSS spacing, that would help. I did not adjusted date/time spacing because I didn't know how to translate spec sizes into CSS sizes that we use in the lockscreen.css.

Eric, I did not adjust the spacing, so I only would like you to look at this. When the time of charging is above an hour I'd like to display it in a form of "Xh and Y mins". Instead of "147 mins remaining". Can you review that piece?

Also, Alive, do I have to add the charging here: https://github.com/mozilla-b2g/gaia/blob/f5e481d4caf9ffa561720a6fc9cf521a28bd8439/apps/callscreen/index.html#L80-L85 ? I'm not sure what this screen is.

Comment on attachment 8546962 [details] pull request Forwarding to Greg since this touches lockscreen and only lockscreen.

Comment on attachment 8546966 [details] charging screenshot with estimation over an hour R+ based on the string format. Can you flag me for review again when the spacing has been updated? Thanks!

Comment on attachment 8546962 [details] pull request Thanks for this nice feature. I've left some comments on the PR page, please set review if you fixed them and add the unit test.

Thanks a lot Greg! Your feedback is awesome! I based my patch on the Clock xlass which was probably not the best choice ;) I'll refactor the patch tomorrow and add tests. Can you provide me tips on how to apply spacing changew from the spec? I've never worked with this and the line-heights from the spec dont match to anything in the css file I can find and also I dont know what the hires version should be.

"spacing change from the spec"? Do you mean media-query?

And I updated the title and keyword for management.

In comment 2 you can see that Eric added the spec and modified vertically spacing of the time and date to fit the new notifications. So in the spec attachment in this bug the line height and vertical paddings are updated. I don't know how to translate the numbers from the spec into our lockscreen.css. can you provide me some guideline here?

So the problem is you want to use line-height to match the lines & spaces but it failed. I think to set the DIV element's height or margin could solve the issue. (For UX:) Eric: I've noticed the spec is only with one SIM. Should UX updated it with two SIMs case? If we only adjust the line height to match the one SIM case only, it may be broken while user is on a multi-SIM device.

Greg, unfortunately don't think I understand how to achieve that. Maybe I'll try to ask specific questions: Question 1: Eric's spec specifies the space between time and Sim info to be 1.6 rem (2.4 rem on fvga). In lockscreen/index.html [0] we have #lockscreen-conn-states and #lockscreen-clock-time. conn-states have padding-bottom: 1.4rem; while clock-time has margin-top: -1.4rem. How 1.6rem translates to 1.4rem + -1.4rem here and I'm not sure if I should increase conn-states padding, clock-time margin, or change something else. I also don't know what to do with fvga. Question 2: Eric's spec specifies the size of clock-time to be 7.3333R rem (11 rem on fvga). The font for clock-time is specifies as 8.5 rem. Should I decrease it to 7.333 rem? What to do about fvga? Question 3: The line-height for #lockscreen-date is specified as 7.3333R rem (11 rem on fvga). I'm not sure if that's an error on Eric's side (copied from the previous line?), or should the line-height of this line match the line-height of the previous. Also, no margins/paddings are defined between date and clock-time. Question 4: The line-height for the new #lockscreen-charging line is defined in spec as 1.6 rem (2.4 on fvga). In my pull-request that's how I defined it. Not sure if I have to do anything special for fvga. Can you help me with those? (also Eric, can you look at my question 3) [0] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/lockscreen/lockscreen.html [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/lockscreen/style/lockscreen.css

I updated the pull request. Waiting for the instructions wrt. CSS, will add tests and re-request review.

Unfortunately I'm afraid that I can't help you a lot: those margins & paddings are always a mess that inherited from very early version like v1.2. I once tried to re-organize them with more clear rules but I've found there are too much things would fail mysteriously and you need to fix them one-by-one. So I'm not sure if for this feature to re-arrange and re-check them all is a good idea (the intention in the spec). In my recollection, for the v2.1 notification feature I even measured whether the size is correct with a physical ruler while I connected to the device via AppManager and adjusted them at the same time, since I failed to make the CSS rules more reasonable. Yes, this sucks, but the fact is I'm the only person for LockScreen and need to face so much messy code & styles should be refactored, which are all inherited from the old version and stuck there like hell's cornerstones which are also protected by the abyss named TBPL & regression, I barely have resources to fix them all (right now I'm refactoring another mess that I really want to kill it twice). So no, I have no reasonable answers for your rem questions. What I can do is to suggest that we don't fix them all at this moment, but focus on the line your feature would add, and ask UX if they can review it as I said.

Hi Zibi/Greg, Sorry the time should stay the same size, I've removed it from the spec now. I'm wondering how the notifications portion of the screen works. Does the gradient adjust vertically based on the space? Or is it in a set position? If it's variable we may not need to mess with the vertical spacing at all. Which leaves us with two options: 1. If it's variable, I think we should leave all vertically spacing and see how it looks (we'll just need to make sure to test on hvga, flame with and without dual sim). 2. If it's in a set position we run into more issues. As Grey mentioned, multi sim takes up even more vertical space (Which will need another spec). In this case I'm wondering if it will be less trouble to change the positioning of the notification gradient? Let me know what you think, Eric

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #22) > Unfortunately I'm afraid that I can't help you a lot: those margins & > paddings are always a mess that inherited from very early version like v1.2. > I once tried to re-organize them with more clear rules but I've found there > are too much things would fail mysteriously and you need to fix them > one-by-one. So I'm not sure if for this feature to re-arrange and re-check > them all is a good idea (the intention in the spec). Oh, ok! I wasn't aware of that. I would be happy to help you sorting this out, but that's definitely out of the scope for this bug. Maybe LockScreen 2.0 next to System 2.0? ;) I'll try to test the space with overflowing notifications and dual-sim and if it'll fit I'll post screenshots here. I'm still not sure how to treat the fvga. I only test on Flame and I don't know if it's fvga or not.

For FVGA part I think you need to have a phone with such resolution, like Buri or Open. I'm not sure if B2G desktop is a good tool to simulate that, unless you know how to bridge your AppManager with the simulator. I could post a how-to after I asked others.

The argument for B2G desktop is: -start-debugger-server 6000 So I think the full line of command is: ./b2g-bin --screen=320x480@160 -profile /path/to/your/gaia/profile -start-debugger-server 6000 With the screen argument it can be FVGA

Eric, so I turned on dual sim, music and made a bunch of screenshots to see how it looks on flame. Here's a screenshot. I can adjust positioning of the elements around Date/Time/Sim if you want, I just need to know what to move/resize :) I'll try to get the fvga from emulator now and finish tests.

(In reply to Zibi Braniecki [:gandalf] from comment #27) > Created attachment 8552130 [details] > Screenshot on Flame with all whistles > > Eric, so I turned on dual sim, music and made a bunch of screenshots to see > how it looks on flame. > > Here's a screenshot. > > I can adjust positioning of the elements around Date/Time/Sim if you want, I > just need to know what to move/resize :) > > I'll try to get the fvga from emulator now and finish tests. Hey Zibi, thanks for working on this. Is it possible to have the vertical alignment changed between 1 and 2 sim(s)? So for 1 sim we can leave your current implementation (will need to test, but I think it will look fine). For 2 sims can we move the sim info, time, date and charging up 1.2rem (on HVGA or 1.8 on flame). Would this be possible? Also, from my understanding FWVGA is the flame device and HVGA is a device like the hamachi. We need to find HVGA device to test on.

(In reply to Eric Pang [:epang] from comment #28) > Hey Zibi, thanks for working on this. Is it possible to have the vertical > alignment changed between 1 and 2 sim(s)? Sure. > So for 1 sim we can leave your current implementation (will need to test, but I think it will look fine). > For 2 sims can we move the sim info, time, date and charging up 1.2rem (on > HVGA or 1.8 on flame). Would this be possible? > > Also, from my understanding FWVGA is the flame device and HVGA is a device > like the hamachi. We need to find HVGA device to test on. Yeah. I got Buri with master so I can test on both Flame and Buri. I'll try to get the patch and screenshots for you tomorrow.

(In reply to Zibi Braniecki [:gandalf] from comment #29) > (In reply to Eric Pang [:epang] from comment #28) > > Hey Zibi, thanks for working on this. Is it possible to have the vertical > > alignment changed between 1 and 2 sim(s)? > > Sure. > > > So for 1 sim we can leave your current implementation (will need to test, but I think it will look fine). > > For 2 sims can we move the sim info, time, date and charging up 1.2rem (on > > HVGA or 1.8 on flame). Would this be possible? > > > > Also, from my understanding FWVGA is the flame device and HVGA is a device > > like the hamachi. We need to find HVGA device to test on. > > Yeah. I got Buri with master so I can test on both Flame and Buri. I'll try > to get the patch and screenshots for you tomorrow. Great, thanks Zibi!

Eric, so, actually adjusting CSS to how many card state lines we have is not trivial. I took a different approach and modified the CSS to make room for charging. Those are the changes I made: https://github.com/zbraniecki/gaia/commit/35e2f08ecfcccab3e8d780be626f65e2b843ad84 I attached screenshots from Flame and Buri to show the outcome. I also confirmed that "Emergency calls only (no SIM)" two-liner takes exactly the same lines as two SIM lines. In Buri scenario, when there's music there is only one notification at a time. In both scenarios, when there's one SIM card, all other header components go up making more room between the charging line and notifications. I can probably adjust the spacing more for the Buri, notifications scenario, by either reducing the header top-padding a bit more (I reduced it by 0.5rem) or reducing the space between time and date (I reduced the line-height of time from 1.0 to 0.9 already). Let me know what you think. :) With Buri and Flame at hand, I can easily test and iterate.

(In reply to Zibi Braniecki [:gandalf] from comment #39) > Eric, so, actually adjusting CSS to how many card state lines we have is not > trivial. > > I took a different approach and modified the CSS to make room for charging. > Those are the changes I made: > https://github.com/zbraniecki/gaia/commit/ > 35e2f08ecfcccab3e8d780be626f65e2b843ad84 > > I attached screenshots from Flame and Buri to show the outcome. > > I also confirmed that "Emergency calls only (no SIM)" two-liner takes > exactly the same lines as two SIM lines. > In Buri scenario, when there's music there is only one notification at a > time. > In both scenarios, when there's one SIM card, all other header components go > up making more room between the charging line and notifications. > > I can probably adjust the spacing more for the Buri, notifications scenario, > by either reducing the header top-padding a bit more (I reduced it by > 0.5rem) or reducing the space between time and date (I reduced the > line-height of time from 1.0 to 0.9 already). > > Let me know what you think. :) With Buri and Flame at hand, I can easily > test and iterate. Hey Zibi, this looks really good (thanks for attaching all the screens for me to see!). Only a couple of comments. 1. In comment 36 the notifications are overlapping, but I feel this is an edge case since the user will more then likely have a sim card in. Once in we shouldn't have any problems. So I think this is fine. 2. In comment 38 is there anyway we can vertically center the notification between the charge and music player (since there's only 1)? Let me know, thanks again!

Centered.

Comment on attachment 8546962 [details] pull request Hi Greg, I think the patch is ready for your review. I followed Eric's feedback on CSS updates. I also added unit tests. One limiting feature is that I can't overwrite navigator.battery so I can't mock it which makes it impossible to test detailed result of chargingstatus.refresh() since it reads navigator.battery properties. I hope that once we update the API to match the spec (bug 1047119), we will be able to override it and test more details. For now, I hope it covers the feature well enough.

Comment on attachment 8558168 [details] Screenshot: Buri, music and notifications, charging Looks good to me, thanks again Zibi!

Comment on attachment 8546962 [details] pull request Okay, I think the code is good, thanks.

Commit: https://github.com/mozilla-b2g/gaia/commit/3c43615cf728840be9e0165f16388672d8e97f2e Merge: https://github.com/mozilla-b2g/gaia/commit/c23229ab79d46070dc8f0743ab592df552be7506 Thanks Eric and Greg!

Comment on attachment 8546962 [details] pull request [Approval Request Comment] [Bug caused by] (feature/regressing bug #): feature [User impact] if declined: no charging info on lockscreen [Testing completed]: unit tests and tests on Flame and Buri [Risk to taking this patch] (and alternatives if risky): no known risks [String changes made]: two additional strings for System/Lockscreen app

v2.2: https://github.com/mozilla-b2g/gaia/commit/70730ccda6465b846b152fee8404d163b417aec8