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)
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 .
Assignee | ||
Comment 2•10 years ago
|
||
Pull request : https://github.com/mozilla-b2g/gaia/pull/22005
commit : 1114be63c3806f33f42652736dc91e85313caf9d
Flags: needinfo?(jlu)
Reporter | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
(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)
Assignee | ||
Comment 6•10 years ago
|
||
(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)
Reporter | ||
Comment 7•10 years ago
|
||
(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)
Assignee | ||
Comment 8•10 years ago
|
||
(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)
Assignee | ||
Comment 9•10 years ago
|
||
Reporter | ||
Comment 10•10 years ago
|
||
(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)
Assignee | ||
Comment 11•10 years ago
|
||
(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)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8461575 -
Attachment is obsolete: true
Attachment #8462431 -
Flags: feedback?(jlu)
Reporter | ||
Comment 13•10 years ago
|
||
Reporter | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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)
Reporter | ||
Comment 16•10 years ago
|
||
(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)
Assignee | ||
Comment 17•10 years ago
|
||
:D
Attachment #8462431 -
Attachment is obsolete: true
Attachment #8463957 -
Flags: review?(jlu)
Flags: needinfo?(jlu)
Assignee | ||
Comment 18•10 years ago
|
||
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)
Reporter | ||
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
I fixed it and pushed it back to the same pull request as a new commit. :)
Flags: needinfo?(jlu)
Reporter | ||
Comment 21•10 years ago
|
||
Everything is super!
Merged on master: https://github.com/mozilla-b2g/gaia/commit/e0e9f7fec72d62eb0ff5f5f267d7323d7464f136
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(jlu)
Resolution: --- → FIXED
Updated•10 years ago
|
feature-b2g: --- → 2.1
You need to log in
before you can comment on or make changes to this bug.
Description
•