John Lu [:mnjul][Please NI or I'll miss] Reporter
	Description • 10 years ago

+++ 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 .

	Sudheesh Singanamalla (:ShellHacker) Assignee
	Comment 2 • 10 years ago

Pull request : https://github.com/mozilla-b2g/gaia/pull/22005
commit : 1114be63c3806f33f42652736dc91e85313caf9d

	John Lu [:mnjul][Please NI or I'll miss] 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.

	Sudheesh Singanamalla (:ShellHacker) 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.

	John Lu [:mnjul][Please NI or I'll miss] 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 ;)

	Sudheesh Singanamalla (:ShellHacker) 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 ?

	John Lu [:mnjul][Please NI or I'll miss] 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).

	Sudheesh Singanamalla (:ShellHacker) 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).

	John Lu [:mnjul][Please NI or I'll miss] 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).

	Sudheesh Singanamalla (:ShellHacker) 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

	John Lu [:mnjul][Please NI or I'll miss] 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 :(

	Sudheesh Singanamalla (:ShellHacker) 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

	John Lu [:mnjul][Please NI or I'll miss] 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!

	Sudheesh Singanamalla (:ShellHacker) Assignee
	Comment 17 • 10 years ago

:D

	Sudheesh Singanamalla (:ShellHacker) Assignee
	Comment 18 • 10 years ago

Sorry for the previous attachment. Forgot to choose patch.

	John Lu [:mnjul][Please NI or I'll miss] Reporter
	Comment 19 • 10 years ago

Comment on attachment 8463960 [details]
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.

	Sudheesh Singanamalla (:ShellHacker) Assignee
	Comment 20 • 10 years ago

I fixed it and pushed it back to the same pull request as a new commit. :)

	John Lu [:mnjul][Please NI or I'll miss] Reporter
	Comment 21 • 10 years ago

Everything is super!

Merged on master: https://github.com/mozilla-b2g/gaia/commit/e0e9f7fec72d62eb0ff5f5f267d7323d7464f136