950884 - [VsD Refresh] Lockscreen Visual Refresh

Status: RESOLVED FIXED

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

Description

[Patryk Adamczyk [:patryk] UX]

Update the look of the lockscreen to a new design.

Comment 1

[Amy Lee [:amylee] UX]

Attachment: Lockscreen.zip

Lockscreen mockups

Comment 2

[Amy Lee [:amylee] UX]

Patch for review:

https://github.com/caseyyee/gaia/tree/1.4-lockscreen-update

Comment 3

[Amy Lee [:amylee] UX]

Attachment: 2014-03-17-16-05-01.png

(In reply to Amy from comment #2)
> Patch for review:
> 
> https://github.com/caseyyee/gaia/tree/1.4-lockscreen-update

Hey Casey, 

I flashed the patch and it looks like the new version haven't been updated yet.

Comment 4

[Casey Yee]

I sent you the wrong branch.  I need to bring it up to date with the changes in master.   I will post back here once I have done that.

Comment 5

[Casey Yee]

Amy, here is the working branch for the bug:
https://github.com/caseyyee/gaia/tree/bug-950884

There are a few things that need to be done still:
- Icons need to be replaced using new set from Peter.
- The unlock handle should be white
- There needs to be a default color range set.  Palette is only created on wallpaper changes currently.
- Still some refinements to play control needed but the style pretty much patches what amy had designed.

Additional notes:
- There has been changes to the unlock mechanism code in the last month so i've just taken the changes straight from master.   So some styles have reverted because of this.  (Should be dark background track with no border)

Comment 6

[Patryk Adamczyk [:patryk] UX]

See comment 6

Comment 7

[Amy Lee [:amylee] UX]

Attachment: Pincode_Screen_Spec.png

Hi, 

Here are my answers:

-Color and transparency ratio on the unlock slide bar.

I've attached a spec for the unlock toggle

-Do we need any color change on the notification bar based on the background color? Or is it just transparent? (This will have impact how color picker will be implemented).

I'm not quite following what you mean. The notification background colour is determined by the colour picker that Casey created.


- Is PIN code key pad style the same as keyboard? Also, what's the transparent ratio on the pin number area?

I've attached a spec for the pin number area. The PIN code key pad should be the same style as the keypad when you create the PIN code in Settings. Please see Settings -> Screen lock --> Passcode Lock

I'm working on a complete spec for the lockscreen and will attach it as soon as it's done in the next day or so. If you need any information sooner let me know and I'll post it.

Comment 8

[Amy Lee [:amylee] UX]

Attachment: Unlock_Toggle_Spec.jpg

Unlock toggle spec

Comment 9

[Amy Lee [:amylee] UX]

Just an update, there has been some additional redesigning of the lock screen happening this week so the specs will come end of the week/early next week.

Comment 10

[Amy Lee [:amylee] UX]

Attachment: Proposed_Lockscreen_Changes.png

Hi, 

I've attached two mocks of the proposed changes to lockscreen. I would like to know that these changes are possible before creating full specs. Currently there is no one assigned to this bug. Is Ivan Tsay the developer on this? Let me know if there are any questions, I would be happy to walk the developer through these proposed changes. Thanks

Comment 11

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_Flow.png

Mock to show lockscreen flow

Comment 12

[jachen]

Amy- Please do a brain dump to Fang so she can work with the developers in that time zone. Thanks!

Comment 13

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_1.4_Spec.png

Hi Fang, 

I've attached the full specs for lockscreen. I'll email you to figure out a meeting time to do a knowledge transfer.

Comment 14

[Amy Lee [:amylee] UX]

Just an FYI, there may be some minor visual updates to the lockscreen icons and toggle. I will post updated graphics here.

Comment 15

[Fang Shih [:fang]]

Got it! I'll check the spec before the meeting. See you at the meeting! Thanks!

Comment 16

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_Flow_1.4.png

Updated Lockscreen flow

Comment 17

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_1.4_Spec.png

Updated Lockscreen Spec

Comment 18

[Prateek Jadhwani [:prateekjadhwani]]

I dont see any UI updates for the user story 942837 : https://bugzilla.mozilla.org/show_bug.cgi?id=942837

Comment 19

[Stephany Wilkes]

Prateek, have you seen the lockscreen spec attached to this bug? If it does not suffice please let us know what you are looking for.

Comment 20

[howie [:howie]]

This ticket is being tracked for 2.0

Comment 21

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_Flow.png

Updated lockscreen flow spec

Comment 22

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_Spec_2.0.png

Updated lock screen spec with new unlock toggle controls

Comment 23

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_Flow_2.0.png

Comment 24

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_Assets.zip

SVG icons for lockscreen

Comment 25

[howie [:howie]]

feature, no blocking

Comment 26

[Stephany Wilkes]

Assigning to Greg Weng, with whom Fang has been working. Setting ni? for Tim to see if this is OK. Tim, please reassign as necessary and/or update the target milestone for the sprint in which this might fit. Thank you!

Comment 27

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

Yeah that's OK.

Comment 28

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

https://moztrap.mozilla.org/manage/case/13061/
https://moztrap.mozilla.org/manage/case/13060/
https://moztrap.mozilla.org/manage/case/13059/
https://moztrap.mozilla.org/manage/case/13039/

Test cases may need to be modified to what gets implemented after UX/FC.

Comment 29

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Amy,

Could we have png files for the assets? As usual, 1x, 1.5x, and 2x versions are needed. Thanks.

Comment 30

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Amy

With current releases we have a "PM" or "AM" after time string, like "8:36 PM", but in the spec it is omitted. Do we really want to remove the "PM"/"AM"? Thanks

Comment 31

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Amy (and sorry for the triple mentioning...)

Where did you get your Fira Sans Ultra Light font? (in your spec, "Time Text" in the "3+ Notifications" image). It appears that we're currently not shipping the specific font weight, ref: https://github.com/mozilla-b2g/moztt/tree/master/FiraSans-2.001 . Thanks!

Comment 32

[John Lu [:mnjul][Please NI or I'll miss]]

Okay, those new fonts are on PR of bug 987872, not merged yet...

Comment 33

[Amy Lee [:amylee] UX]

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #29)
> Hi Amy,
> 
> Could we have png files for the assets? As usual, 1x, 1.5x, and 2x versions
> are needed. Thanks.

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #30)
> Hi Amy
> 
> With current releases we have a "PM" or "AM" after time string, like "8:36
> PM", but in the spec it is omitted. Do we really want to remove the
> "PM"/"AM"? Thanks

Hi John, 

Yes we are removing AM/PM. It seems obvious if the time is AM or PM and other OS's don't include AM/PM either.

Also, Fang will be taking over lockscreen assets/reviewing implementation. Thanks

Comment 34

[Stephany Wilkes]

Thanks, Amy. Fang, please put reviews in this bug as comments. Thanks, all!

Comment 35

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_Flow_2.0.png

Updated spec with camera icon in pin code screen

Comment 36

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_Flow_2.0.png

Updated spec. Added camera icon to pin code screen.

Comment 37

[Amy Lee [:amylee] UX]

Attachment: Lockscreen_2.0_Spec.png

Updated spec. Added camera icon to pin code screen.

Comment 38

[Amy Lee [:amylee] UX]

Attachment: music_player_icons.zip

Hi, 

Just an FYI that the music player controls in lockscreen have been made slightly bigger so it's easier for users to navigate (see Bug 1011710, comment 8). I've attached the larger music player controls here in @1, @1.5, and @2 scale. Thanks!

Comment 39

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Amy: [re: removal of AM/PM] If we want to remove "AM/PM", do we need to show 24hr time? For example, when it is 1:30PM in the afternoon, do we want to show "13:30" instead of "1:30"? Thanks.

Comment 40

[Fang Shih [:fang]]

Attachment: Lockscreen_Assets_png.zip

Hi John,
I've attached the png icons in @1, @1.5, and @2 scale. Thanks!

Comment 41

[John Lu [:mnjul][Please NI or I'll miss]]

Hi Fang,

Thank for the png icons! However, I need one extra set of the camera icon: as the camera on the pincode screen is larger (26*20px) than the one on the lockscreen track (21*16px), I need the camera icon png files (1x, 1.5x, 2x) for the pincode screen. Thanks!

Comment 42

[Fang Shih [:fang]]

Attachment: Pin_Camera_icon.zip

Hi John,
Camera icon attached. Thanks!

Comment 43

[Amy Lee [:amylee] UX]

Attachment: Big Music Player Icons.zip

My mistake. Here are the correct sizes for the music player icons in lockscreen. 

Thanks!

Comment 44

[John Lu [:mnjul][Please NI or I'll miss]]

Attachment: Patch

WIP patch for almost everything excluding the "more notifications" vertical arrow as in "3+ Notifications"

Comment 45

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8429923 [details] [review]
Patch

Great work. Please fix some nits, especially add some comments to help to explain the code better, especially about the 'new style'.

Comment 46

[Stephany Wilkes]

Nice work, everyone. Can't wait to see this land!

Comment 47

[John Lu [:mnjul][Please NI or I'll miss]]

Master: https://github.com/mozilla-b2g/gaia/commit/7a09fc92d54368e7dcbe7ec76bf2ebf0ff854d8c

Comment 48

[Amy Lee [:amylee] UX]

Hi, 

I just wanted to check in if visual design has been flagged for UI review?

Comment 49

[Germán Toro del Valle (:gtorodelvalle)]

Hi John, I am currently using your code for the call screen to attend calls when the lock screen is activated and I found an element (#lockscreen-mute) in system/index.html which does not seem to be used. Or is there any scenario where this element is used? Just to know if I should used in the call screen case :) Thanks!

Comment 50

[Fang Shih [:fang]]

Attachment: 2.0_lockscreen_bug_spec.pdf

Hi John,

I think it is really close now. According to our discussed earlier. Just a couple things that need to adjust.  I’ve also put them into one pdf. You can refer to the file attached.

Some notes: 

1) Missing gradient layer on the top of lock screen, when there is no notifications, it should show a light gradient layer on the top of the background image. 

2) Time font: It's not Fira Sans Ultra Light yet, but updates for this should happen when font landed into the device. 
  
3) The "Toggle arrows” is a bit fuzzy, looks like it stretch a little bit so it should be shorter than the gray toggle circle also.

4) Unlock background stroke is a bit heavy, Stroke Gradient Opacity is: Top 100%, Bottom 0%


5) On the Pin lock screen, the camera icon is not centered.


6) The clear bottom is incorrect size of the Pin lock keypad. 


7) Notification is missing arrow icon, and also scrolling up with “Time” is not showing. But according to your feedback, we are not able to have this function right now. 

Note that notification gradient may still a bit greenish, maybe we can adjust this a bit. Thanks!

Comment 51

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

(In reply to Fang Shih from comment #50)
> Created attachment 8433852 [details]
> 2.0_lockscreen_bug_spec.pdf
> 

Fan, thank you for the UI review. I've filed bug 1020115 to follow-up.

Comment 52

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

Hum, it looks like this bug should depend on that instead of blocks.

Comment 53

[Doug Sherk (:drs) (inactive)]

I'm not sure how this landed as there are several problems here that I can see that should have prevented this. Please correct me if I'm wrong, as there may have been offline discussion or I may be missing something, but:

1) Greg Weng is not a peer on any module according to the Firefox OS Module Owners page, let alone Gaia::System. [1] It doesn't appear that this review was redirected from a module peer or owner. I see in comment 26 that he was assigned to it, but wasn't given the review. Perhaps Tim's silence here was an implicit go-ahead.
2) This patch has no tests added or even modified. [2]
3) This is less important, and maybe I'm nitpicking. However, there are some fairly obvious problems that I spotted from just a quick skim, without knowing anything about the code, and which were not caught in review. [3]

[1] https://wiki.mozilla.org/Modules/FirefoxOS
[2] ctrl+f "_test" on https://github.com/mozilla-b2g/gaia/pull/19712/files
[3] This list does not include whitespace, indentation, and stylistic problems I found.
https://github.com/mozilla-b2g/gaia/pull/19712/files#diff-61c16a0a0190e95d51031835f62b30d7R1064 - using a rowX class instead of just wrapping these elements with a row div
https://github.com/mozilla-b2g/gaia/pull/19712/files#diff-3fb4b6ea1cebdea57ada1ac50c2c9c7aR914 - repeating a variable declaration
https://github.com/mozilla-b2g/gaia/pull/19712/files#diff-61c16a0a0190e95d51031835f62b30d7R1010 - use of this .blank class instead of override patterns
https://github.com/mozilla-b2g/gaia/pull/19712/files#diff-335953a0df10e3fb9613930bca786abfR534 - overly specific CSS selectors all over this code

As a consequence of these problems, my review of bug 1041879 is going to take much longer than it should because I can't read the tests to see expected functionality, or get clues as to what the changes there may be breaking. There's also the bigger problem that this code is completely untested so changes to it are much more liable to break it.

These things happen, and that's ok. But at this point, I think it's reasonable to expect everyone knows the Gaia policies, and it's the job of reviewers to enforce these. Perhaps the bigger problem is that I can't actually find these policies documented anywhere, and we should fix that.

Comment 54

[Greg Weng [:snowmantw][:gweng][:λ]]

1. Good. If peer is the perquisite to review a patch, I'm glad to give the responsibility to anybody has the title, while there are some patches reviewed by me may now lost the legitimacy now, although they're mostly delegated by System owner & peer.

2. It passed all tests on Gaia-Try[1] include linter check. I don't mean Gaia-Try is enough to protect us from regressions, but the result shows either

a. at least the existing tests are not broken with the patch
b. need to add a whole bunch of tests to get our code base stronger, but because of the risk and what we have adopted usually, this should be another standalone bug and patch to to that (to add tests 'files', not only add some lines).

3. In my experiences, reviewers make differences decisions when they're in different point of views and this includes the personal opinions in even different times. For example, one of my reviewer asked nothing about one of patch and give r+ without any issue. But the next time (after rebasing, maybe the GitHub CC mail noticed him), he gave about 20+ opinions with the same codes. Since the 'useNewStyle' is a workaround we introduced, what I concerned is to remove it to make our codebase more clear. And I saw what you addressed on the GitHub pages, they're most not critical errors, while

a. indentation issue: as you said:

'I'd say indentation here is off, but there's no real standard, as long as it's either 2 spaces or lined up with the previous line.'

I've seen lot's of tiny variations of indentations in our codebase. If you want to unify them and clear all code we should fire another bug to do that, and send a mail to dev-gaia to communicate people whom wrote all of those code. But if I'm permitted to express my opinion that I think the 2-spaces sometime is not clear as what we've left in our codebase, although I know people would like to forcibly ask this to eliminate the possible arguments.

b. stale comments & dead images: yes, my fault to miss them. Thanks.

c. 'You should remove this file from xfail.list and fix the linting errors.' -- While the purpose of this patch is removing the workaround, and as far as I know there seems no forcible requirement to ask every patch to fix xfail.list at the same time (from the patches I've submitted and the principles I may only know a little; or do we?), I'd say this seems depends on reviewers, so I don't think this is a fail to ask nothing about that. 

4. John discussed this offline with me at our office. And on real device test his phone has no serious visual breaking, while reviewer should delegate real UI grant to UI review and that's the thing I suppose John would do, that's why I said I 'assume' the UI is okay. However, maybe the word is not correct and reviewer should do more things UX does to test the patch.

5. Some unclear comments and code I missed maybe because the code was mainly written by me (which make the comments are meaningful) , so there is a review blindness I think. And I should consider if someone else suddenly jump in to re-write the code.

[1]: https://tbpl.mozilla.org/?rev=70d40a0943d8c5efbb6f4233ac6666e5aac3bdaa&tree=Gaia-Try

Comment 55

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

Commented in bug 1041879 comment 8.

Comment 56

[Doug Sherk (:drs) (inactive)]

Let's keep the discussion for this in bug 1041879. I just posted bug 1041879 comment 10.