Closed Bug 1041449 Opened 10 years ago Closed 10 years ago

Lockscreen PIN keypad Visual Update for Bug 983043

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.1)

RESOLVED FIXED
feature-b2g 2.1

People

(Reporter: mnjul, Assigned: ShellHacker)

References

Details

Attachments

(4 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #887148 +++ This bug will address Lockscreen PIN keypad visual update as specified in page 15 of https://bug983043.bugzilla.mozilla.org/attachment.cgi?id=8454327 .
Pull request : https://github.com/mozilla-b2g/gaia/pull/22005 commit : 1114be63c3806f33f42652736dc91e85313caf9d
Flags: needinfo?(jlu)
Thanks Sudheesh! However, my intended goal for this bug is to fully implement the lockscreen PIN keypad visual update as specified in page 15 of https://bug983043.bugzilla.mozilla.org/attachment.cgi?id=8454327 . Besides removing the period, this also includes moving the English letters to the bottom to each digit, from their current position of being right to each digit. As such, besides removing the period from system app's index.html, CSS changes are needed too (and maybe other adjustments too). So, do you want to take this bug still? Thanks.
Flags: needinfo?(jlu)
The center aligning of the characters below the number involves working on this file path right [1] [1] gaia/apps/system/style/lockscreen/lockscreen.css I'd love to take this up and work if you could mentor me, I am new to the Gaia codebase but it seems pretty nice to read and understand.
Flags: needinfo?(jlu)
(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #4) > The center aligning of the characters below the number involves working on > this file path right [1] > > [1] gaia/apps/system/style/lockscreen/lockscreen.css Yes, I believe that's the only file you need to change. You might want to take reference from gaia/apps/keyboard/style/keyboard.css (note: *not* apps/system/style/system/keyboard.css) for some CSS values such as font size, color, etc. > I'd love to take this up and work if you could mentor me, I am new to the > Gaia codebase but it seems pretty nice to read and understand. Sounds good, let's see how it goes ;)
Flags: needinfo?(jlu)
(In reply to John Lu [:mnjul] [MoCoTPE] from comment #5) > Yes, I believe that's the only file you need to change. You might want to > take reference from gaia/apps/keyboard/style/keyboard.css (note: *not* > apps/system/style/system/keyboard.css) for some CSS values such as font > size, color, etc. I tried having a look at the keyboard.css file and figured out the colors to be used from there, also I noticed that the ABC,DEF,GHI etc.., are being placed below the number in the keyboard using > Sounds good, let's see how it goes ;) I realized that a new CSS rule has to be written for #lockscreen-passcode-pad > a.row0 > div > span {} #lockscreen-passcode-pad > a.row1 > div > span {} #lockscreen-passcode-pad > a.row2 > div > span {} #lockscreen-passcode-pad > a.row3 > div > span {} But its surely not text-align:center; I have tried reducing the pixel size and playing around with the padding values but wasn't successful in putting the characters to the bottom of the number, What am I missing ?
Flags: needinfo?(jlu)
(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #6) > > I realized that a new CSS rule has to be written for > > #lockscreen-passcode-pad > a.row0 > div > span {} > #lockscreen-passcode-pad > a.row1 > div > span {} > #lockscreen-passcode-pad > a.row2 > div > span {} > #lockscreen-passcode-pad > a.row3 > div > span {} > > But its surely not text-align:center; I have tried reducing the pixel size > and playing around with the padding values but wasn't successful in putting > the characters to the bottom of the number, What am I missing ? You're heading the right direction. Now, to force the alphabets in <span> to the bottom of the digit, I would just add a display: block CSS property to <span> such that it is wrapped to the next "line". At this moment, it probably will be too "bottom" to be visible. So you need to assign a negative margin-top for the <span> for it to be visible. You then need to adjust its padding-left (just as with its parent <div>) to make it horizontally centered. If things look a bit crowded by now, then you might need to adjust the parent <div>'s margin-top to ease the crowdedness. The placement of the digits and alphabets have been quite ad-hoc (many of the "offset" values such as padding/margin are empirical).
Flags: needinfo?(jlu)
(In reply to John Lu [:mnjul] [MoCoTPE] from comment #7) > Now, to force the alphabets in <span> to the bottom of the digit, I would > just add a display: block CSS property to <span> such that it is wrapped to > the next "line". At this moment, it probably will be too "bottom" to be > visible. So you need to assign a negative margin-top for the <span> for it > to be visible. You then need to adjust its padding-left (just as with its > parent <div>) to make it horizontally centered. If things look a bit crowded > by now, then you might need to adjust the parent <div>'s margin-top to ease > the crowdedness. I tried this and it works great so far except for the padding-left, I am having issues setting this value, since the padding can't have negative values, another option would be to shift the margin. But what else could be done regarding this padding issue. > The placement of the digits and alphabets have been quite ad-hoc (many of > the "offset" values such as padding/margin are empirical).
Assignee: nobody → sudheesh1995
Flags: needinfo?(jlu)
Attached patch Patch (obsolete) — Splinter Review
Attached image 2014-07-25-11-57-45.png
(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #8) > Created attachment 8461566 [details] > Bottom aligned characters (padding missing).png > > (In reply to John Lu [:mnjul] [MoCoTPE] from comment #7) > I tried this and it works great so far except for the padding-left, I am > having issues setting this value, since the padding can't have negative > values, another option would be to shift the margin. But what else could be > done regarding this padding issue. Good job on the work! Yes, you can't use negative padding values, so I suggest you change margin-left. I have observed two minor issues in your patch: 1. Blocks of different selectors but with same CSS rules may be combined. For example, instead of writing: #lockscreen-passcode-pad > a.row0 > div { margin-top: 0.1em; } #lockscreen-passcode-pad > a.row1 > div { margin-top: 0.1em; } #lockscreen-passcode-pad > a.row2 > div { margin-top: 0.1em; } we may write this: #lockscreen-passcode-pad > a.row0 > div, #lockscreen-passcode-pad > a.row1 > div, #lockscreen-passcode-pad > a.row2 > div { margin-top: 0.1em; } for the sake of reducing duplicated codes. 2. In Gaia we mostly use "rem" units. Think of 1 rem as 10px on a non-HiDPI (ie. devicePixelRatio=1) device. So the patch needs its em usage changed to rem usage. As a reference, the attached screenshot is how your patch looks right now on the reference device (Flame).
Flags: needinfo?(jlu)
(In reply to John Lu [:mnjul] [MoCoTPE] from comment #10) > Created attachment 8462332 [details] > 2014-07-25-11-57-45.png Nice. > Good job on the work! > > Yes, you can't use negative padding values, so I suggest you change > margin-left. Changed. > 2. In Gaia we mostly use "rem" units. Think of 1 rem as 10px on a non-HiDPI > (ie. devicePixelRatio=1) device. So the patch needs its em usage changed to > rem usage. Used rem instead of em. > As a reference, the attached screenshot is how your patch looks right now on > the reference device (Flame). Pull request made : https://github.com/mozilla-b2g/gaia/pull/22148 Git Diff : https://github.com/sudheesh001/gaia/commit/5a6d613a1c26d69cab0457a3fa3ff2a7fe5dffeb
Flags: needinfo?(jlu)
Attached patch Patch.patch (obsolete) — Splinter Review
Attachment #8461575 - Attachment is obsolete: true
Attachment #8462431 - Flags: feedback?(jlu)
Comment on attachment 8462431 [details] [diff] [review] Patch.patch Good, with two issues: 1. The colors of the alphabets don't match those from Settings's keypad (you can see https://bug1041449.bugzilla.mozilla.org/attachment.cgi?id=8462468 ). 2. The alphabets aren't precisely horizontally centered for every button. This is a natural result of the fact that we assign the same margin-left value for the same row, even though different buttons of the same row have alphabets of different widths. While it is possible to assign each <span> button with different margin-left values individually, that would be too much a hassle. So I'm proposing a resolution that actually goes back to using ``text-align: center``: For the outer <div>, remove ``padding-left: 4.3rem``, and change ``text-align: left`` to ``text-align: center``. For the inner <span>, keep the ``display: block`` and ``margin-top: -2.6rem``, and remove ``margin-left``. While this should solve the centering problem of the alphabets, with this, we might have issues with the func buttons ('Emergency Call' and 'Cancel'), but we will see what happens then. My apologies for having you try something that didn't work perfectly :(
Attachment #8462431 - Flags: feedback?(jlu)
Flags: needinfo?(jlu)
PR : https://github.com/mozilla-b2g/gaia/pull/22208 DIFF : https://github.com/sudheesh001/gaia/commit/9945744d9aedc011937b8362c412b0e6a7669ff6 >So I'm proposing a resolution that actually goes back to using ``text-align: center``: For the outer ><div>, remove ``padding-left: 4.3rem``, and change ``text-align: left`` to ``text-align: center``. For the >inner <span>, keep the ``display: block`` and ``margin-top: -2.6rem``, and remove ``margin-left``. Fixed. >My apologies for having you try something that didn't work perfectly :( Not a problem. It was a learning curve :D
Flags: needinfo?(jlu)
(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #15) > PR : https://github.com/mozilla-b2g/gaia/pull/22208 > DIFF : > https://github.com/sudheesh001/gaia/commit/ > 9945744d9aedc011937b8362c412b0e6a7669ff6 Looks great! Now for the last minor point let's match the color of the alphabets with the one seen in settings' PIN keypad. Lockscreen's: https://github.com/sudheesh001/gaia/blob/9945744d9aedc011937b8362c412b0e6a7669ff6/apps/system/style/lockscreen/lockscreen.css#L514 for normal status and https://github.com/sudheesh001/gaia/blob/9945744d9aedc011937b8362c412b0e6a7669ff6/apps/system/style/lockscreen/lockscreen.css#L581 for highlight ('active') status We want this color from num keypad from keyboard.css, for normal status: https://github.com/sudheesh001/gaia/blob/9945744d9aedc011937b8362c412b0e6a7669ff6/apps/keyboard/style/keyboard.css#L706 For highlight status, keyboard.css designates ``opacity: 0``: https://github.com/sudheesh001/gaia/blob/9945744d9aedc011937b8362c412b0e6a7669ff6/apps/keyboard/style/keyboard.css#L204 , so for lockscreen.css you may either want to change ``color: #fff`` to ``opacity: 0``, or use ``color: transparent``. Your patch should be well after this change, and at that moment please attach PR to this bug and set review flag. I will land the patch for you when everything is ready. Good job!
Flags: needinfo?(jlu)
:D
Attachment #8462431 - Attachment is obsolete: true
Attachment #8463957 - Flags: review?(jlu)
Flags: needinfo?(jlu)
Sorry for the previous attachment. Forgot to choose patch.
Attachment #8463957 - Attachment is obsolete: true
Attachment #8463957 - Flags: review?(jlu)
Attachment #8463960 - Flags: review?(jlu)
Comment on attachment 8463960 [details] [review] https://github.com/mozilla-b2g/gaia/pull/22256 Good with r+ ;) Yet, there is a small CSS nit that could be fixed before we merge the patch. Could you fix it? Thanks.
Attachment #8463960 - Flags: review?(jlu) → review+
Flags: needinfo?(jlu)
I fixed it and pushed it back to the same pull request as a new commit. :)
Flags: needinfo?(jlu)
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(jlu)
Resolution: --- → FIXED
feature-b2g: --- → 2.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: