### 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

Yeah, 1px missing from the height of #dialog-overlay. Should be in System app. Do you want to nominate?

[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?

Hm I thought this was fixed with bug 1023201?

Tim, I think this bug belongs to your team? Thanks, Candice

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!

Yeah, I'm on it.

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.

(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

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.

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

Triage: no significant user impact, de-nom.

Comment on attachment 8484804 [details] 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?

Comment on attachment 8484804 [details] 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 !

Comment on attachment 8484804 [details] Patch (PR @ GitHub) Discussed offline. Let's avoiding using getter if possible.

Comment on attachment 8484804 [details] Patch (PR @ GitHub) Alright, here's the updated patch.

Comment on attachment 8484804 [details] Patch (PR @ GitHub) Tim: I modified the patch again. Thanks!

Master: https://github.com/mozilla-b2g/gaia/commit/515b2d7f57bcdd11159b98c8dfff9fbed85ed967

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