Closed Bug 1059683 Opened 10 years ago Closed 10 years ago

Thin line appears between keypad and pin request page

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 verified)

VERIFIED FIXED
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- affected
b2g-v2.1 --- affected
b2g-v2.2 --- verified

People

(Reporter: ericcc, Assigned: mnjul)

Details

(Whiteboard: [FT:System-Platform])

Attachments

(3 files)

Attached image SIM_Pin_Request.png
### STR 
1. Turn SIM pin on.
2. Reboot.
3. SIM pin request page shown.

### Actual 
A space appears between keypad and pin request page, change the background to another color, you will also observe that.
See the peach line in SIM_Pin_Request.png

### Version
Gaia      3a838afca295c9db32e1a3ec76d49fb7fe7fd2d2
Gecko     https://hg.mozilla.org/mozilla-central/rev/5f0b5cc8f78d
BuildID   20140827160207
Version   34.0a1
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230
Hi Tim, Is it a keyboard issue or from window management? Thanks
QA Whiteboard: [COM=Gaia::Keyboard]
Component: Gaia → Gaia::Keyboard
Flags: needinfo?(timdream)
Yeah, 1px missing from the height of #dialog-overlay.

Should be in System app.

Do you want to nominate?
Component: Gaia::Keyboard → Gaia::System
Flags: needinfo?(timdream)
[Blocking Requested - why for this release]: to make UI better.

Yes.
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #2)
> Yeah, 1px missing from the height of #dialog-overlay.
> 
> Should be in System app.
> 
> Do you want to nominate?
blocking-b2g: --- → 2.1?
QA Whiteboard: [COM=Gaia::Keyboard] → [COM=Gaia::System]
Hm I thought this was fixed with bug 1023201?
Tim,

I think this bug belongs to your team?

Thanks,
Candice
Flags: needinfo?(timdream)
This is one of the bugs in the vague area -- SIM dialog are of systemsfe teams but it's interaction w/ keyboard is of input mgmt. John, could you take this bug? Thanks!
Flags: needinfo?(timdream) → needinfo?(jlu)
Yeah, I'm on it.
Assignee: nobody → jlu
Flags: needinfo?(jlu)
Quick note: I've flashed pvt 1.4 onto my Flame and the problem IS still there. And on Buri it's not there with any versions.

I think it's some round-off issue in dimensions since our calculation during TrustUI's |setHeight|ing uses calculation from |window.innerHeight|, which does not take dpx-related round-off issue into account. We probably should be using |layoutManager.height|, which does. See [1] and [2]. (On flame, device height in CSS px is 569.333px).

Thus my proposed patch would be [3] using |layoutManager.height| to calculate TrustedUI overlay height. However that doesn't seem to work -- even though the |overlay.style.height| is being set to (on Flame) 334.333px on [4], what I see in Nightly App Manager Inspector is still showing 334px for the element. However, if I manual write the CSS property in the inspector *or* run in console |TrustedUIManager.overlay.style.height="334.333px";|, it would work (i.e. this bug is fixed). 
^^^ NI'ing Etienne for comment for this paragraph ^^^

[1] https://github.com/mnjul/gaia/commit/f1067654132d137bbfaf8fcadde485951db61c75
[2] https://github.com/mnjul/gaia/blob/f1067654132d137bbfaf8fcadde485951db61c75/apps/system/js/layout_manager.js#L59-L64
[3] https://github.com/mnjul/gaia/tree/bug_1059683_concept_layout
[4] https://github.com/mnjul/gaia/blob/f1067654132d137bbfaf8fcadde485951db61c75/apps/system/js/trusted_ui.js#L313-L315

With that said, it probably would be easier if alternatively we fix the bug by that if we detect some round-off issue that's to take place (like if we have fractions in the resulting calculation of [2]), we would simply subtract 1 from |KeyboardManager|'s reported |height| to calculate other layout's actual height. Easier, but kinda ad-hoc, though.
Flags: needinfo?(etienne)
(In reply to John Lu [:mnjul] [MoCoTPE] from comment #8)
> Thus my proposed patch would be [3] using |layoutManager.height| to
> calculate TrustedUI overlay height. However that doesn't seem to work --
> even though the |overlay.style.height| is being set to (on Flame) 334.333px
> on [4], what I see in Nightly App Manager Inspector is still showing 334px
> for the element. However, if I manual write the CSS property in the
> inspector *or* run in console
> |TrustedUIManager.overlay.style.height="334.333px";|, it would work (i.e.
> this bug is fixed). 
> ^^^ NI'ing Etienne for comment for this paragraph ^^^

Weird, can you try doing the little dance from [1] directly in TruestedUI._setHeight?
(To make sure the keyboardHeight subtraction isn't making us fall on a value that's not an integer in device pixels.)

[1] https://github.com/mnjul/gaia/blob/f1067654132d137bbfaf8fcadde485951db61c75/apps/system/js/layout_manager.js#L59-L64
Flags: needinfo?(etienne)
Attached file Patch (PR @ GitHub)
Well, I figured out why |TUI._setHeight()| didn't apparently work: it was followed almost immediately by |SystemDialog.resize()| which also sets the height on the overlay [1].

|SD.updateHeight()| (called by its resize) correctly uses |window.layoutManager.height| but there is something quite strange: On Flame with dpx=1.5, |lM.height| getter sees that the "available height" would not have a fraction in the logics in [2] and the logics return an integer -- even though we're kinda 0.333px short [*]. 

What I'm having in mind is that since this is just a minor graphics glitch, if we really want to fix it quick and dirty then here is my patch: in |KeyboardManager.getHeight()|, if we see that the keyboard height would result a fractional device pixel, we report one less CSS pixel to the caller.

Etienne, please see if this is good enough for you. Flagging Tim for feedback too.

[1] https://github.com/mnjul/gaia/blob/b88dbac652005f7cb0c8aff7625b8f6cfaebc8d7/apps/system/js/system_dialog.js#L166-L177

[2] https://github.com/mnjul/gaia/blob/b88dbac652005f7cb0c8aff7625b8f6cfaebc8d7/apps/system/js/layout_manager.js#L55-L64

[*] height === 364px = (window.innerHeight = 569px) - (KM.getHeight() = 205px); (height * (dpx = 1.5) === 546dpx) % 1 === 0
Status bar is 30px high so in previous comments I talked about 334px.
On Flame, |window.innerHeight| is 569, while it actually should be 569.333, I presume.
Attachment #8484804 - Flags: review?(etienne)
Attachment #8484804 - Flags: feedback?(timdream)
Comment on attachment 8484804 [details] [review]
Patch (PR @ GitHub)

Nice find, although I have some doubt on this patch:

- We are sending the height as detail of keyboard* events in InputAppsTransitionManager -- you might want to put your calculations there.
- Need comment in the code on the reason of having this complexity
Attachment #8484804 - Flags: feedback?(timdream) → feedback+
Whiteboard: [FT:System-Platform]
Triage: no significant user impact, de-nom.
blocking-b2g: 2.1? → backlog
Comment on attachment 8484804 [details] [review]
Patch (PR @ GitHub)

I think I understand the "quick fix" and agree with Tim's comment.

But do we need to keep the |Math.ceil()| if we're doing a |height--| right after anyway? Do we want to use |.floor()| instead?
Attachment #8484804 - Flags: review?(etienne)
Comment on attachment 8484804 [details] [review]
Patch (PR @ GitHub)

(In reply to Etienne Segonzac (:etienne) from comment #13)
> But do we need to keep the |Math.ceil()| if we're doing a |height--| right
> after anyway? Do we want to use |.floor()| instead?

Yah, makes sense. Modified that.

Since we all are good on the concept, here's the formal patch, with height adjustment implemented in InputAppTransitionManager as mentioned by Tim, and tests properly added/modified.

As the modification now lies in the input management realm, I've now requested Tim's review on this. (I didn't set f? for Etienne as I believe he's good on the concept but please feel free to tell me to do otherwise)

Again thanks for your feedback folks !
Attachment #8484804 - Attachment description: Proposed Patch (PR @ GitHub) → Patch (PR @ GitHub)
Attachment #8484804 - Flags: review?(timdream)
Comment on attachment 8484804 [details] [review]
Patch (PR @ GitHub)

Discussed offline. Let's avoiding using getter if possible.
Attachment #8484804 - Flags: review?(timdream)
Comment on attachment 8484804 [details] [review]
Patch (PR @ GitHub)

Alright, here's the updated patch.
Attachment #8484804 - Flags: review?(timdream)
Attachment #8484804 - Flags: review?(timdream)
Comment on attachment 8484804 [details] [review]
Patch (PR @ GitHub)

Tim: I modified the patch again. Thanks!
Attachment #8484804 - Flags: review?(timdream)
Attachment #8484804 - Flags: review?(timdream) → review+
Master: https://github.com/mozilla-b2g/gaia/commit/515b2d7f57bcdd11159b98c8dfff9fbed85ed967
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attached image 2014-10-08 17.19.38.jpg
Fixed in v2.2
Gaia-Rev        9050edcda308b65d86577c8ed0eedc5c568d8e44
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/0c8ae792f1c0
Build-ID        20141007160202
Version         35.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141007.191609
FW-Date         Tue Oct  7 19:16:20 EDT 2014
Bootloader      L1TC00011840
Status: RESOLVED → VERIFIED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.