1074123 - SIM PIN dialog margin bottom is wrong with soft home button enabled

Status: VERIFIED FIXED

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

Description

[Alberto Pastor [:albertopq]]

Attachment: Screenshot

Tested on a Flame device.

STR:

1.- Enable soft home button on the Developer menu
2.- Enable SIM PIN security
3.- Restart the phone

Expected

There is not space between the keyboard and the pin dialog

Actual

There is a gap between the keyboard and pin dialog

Comment 1

[Alberto Pastor [:albertopq]]

Comment on attachment 8496761 [details]
Screenshot

https://drive.google.com/a/mozilla.com/file/d/0ByGvk9QDMNxZRERha011ZWFUdXM/edit?usp=sharing

Comment 2

[Alberto Pastor [:albertopq]]

Versions affected?

Comment 3

[Cody Roesch [:croesch]]

This bug repro's on Flame KK builds: Flame 2.2 KK,

Actual Results: Large gap seen between the keyboard and the buttons for the SIM PIN Entry screen.

Repro Rate: 4/4

Environmental Variables:
Device: Flame Master KK
BuildID: 20140929070359
Gaia: 4d663b1f7d63e4d3d69c181a58f21b38145044b2
Gecko: 66ab05e2a667
Version: 35.0a1 (Master) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

-----------------------------------------------------------------
-----------------------------------------------------------------

This bug does NOT repro on Flame kk build: Flame 2.1 KK, Flame 2.0 KK

Actual Result: No gap seen between the keyboard and the SIM PIN Entry screen.

Repro Rate: 0/4

Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20140929004258
Gaia: 063de64a4ffc606e931ed7b09e93282713c46eca
Gecko: 5d0ec7211d63
Version: 34.0a2
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
-----------------------------------------------------------------
Environmental Variables:
Device: Flame 2.0 KK
BuildID: 20140929063300
Gaia: 5c2303ec4e367da060aa1b807d541a6549b3d72a
Gecko: d7b2e9cc93f8
Version: 32.0 (2.0) 
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 4

[Cody Roesch [:croesch]]

***Correction***
Flame 2.1 KK IS affected. I was looking at the wrong screen.

Environmental Variables:
Device: Flame 2.1 KK
BuildID: 20140929004258
Gaia: 063de64a4ffc606e931ed7b09e93282713c46eca
Gecko: 5d0ec7211d63
Version: 34.0a2
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 5

[Alberto Pastor [:albertopq]]

[Blocking Requested - why for this release]: Bad UX in one of the first screens the user is able to see (only happening with soft button, though)

Comment 6

[Alberto Pastor [:albertopq]]

Attachment: Screenshot

Comment 7

[Gregor Wagner [:gwagner]]

Bad UX

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

Sorry to steal this one from you Alberto, but I had already investigated and come up with a patch for this in bug 1074608 (see [1]). The problem is that most dialogs have a wrapping div around the .generic-dialog which takes up the entire height of the screen (whether or not SHB is enabled). But SIM PIN dialog is unique in that .generic-dialog is a direct descendant of the .modal-dialog container. So, the height of this dialog depends on that container (which gets dynamically resized by the system), and thus it's height does not include the SHB and so does not need to account for it in it's bottom property. I'll submit my patch here for FB and review.

1.) https://bugzilla.mozilla.org/show_bug.cgi?id=1074608#c2

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Gaia PR] Anchor SIM PIN to bottom of modal dialog

See comment 8.

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8498528 [details] [review]
[Gaia PR] Anchor SIM PIN to bottom of modal dialog

r+ but
Don't we need a general fix for all system dialog?
Don't we have the same issue with FirefoxAccounts?

Comment 11

[Alberto Pastor [:albertopq]]

Attachment: Screenshot error

Apparently, there is a race condition between showing the softbutton and calculating the height. It doesn't happen every time. After showing the keyboard, everything gets resized correctly

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8498528 [details] [review]
[Gaia PR] Anchor SIM PIN to bottom of modal dialog

Good catch Alberto, I need to come up with a better solution.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> Don't we need a general fix for all system dialog?

We do. I think this will require some refactoring, and better tests. Let's do this after FC.

> Don't we have the same issue with FirefoxAccounts?

Probably. I'll investigate.

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

Here's an update on what I've found so far:

One of the main sources of problems is that both system_dialog.js and modal_dialog.js updates the height of the #dialog-overlay container. However, system_dialog takes into account the SHB using the layout manager, while modal_dialog does not. This make's showing dialogs racy and brittle, and is an inconsistency we should fix here (which will require some css fixes, but worth it in the long run).

Another problem is that not all dialogs reside in this dynamically updated #dialog-overlay, but reuse many of the same css classes for SHB. This makes fixing them globally hard, since they have different sized containers with respect to SHB. So far I have found the following classes of dialogs:

In #dialog-overlay
 - Sometimes can break (show extra space at the bottom, like this bug mentions) depending on who sets the container height.
 - ex. sin pin, modal dialogs, crash-dialog

Not in #dialog-overlay, but still a .generic-dialog:
 - These generally work in non-fullscreen mode, but in fullscreen they often have a gap at the bottom the size of the statusbar. Perhaps most of these aren't meant to be displayed when an app runs in fullscreen, but we might as well fix it in case.
 - ex. icc-view, ime-layout, quick settings

Not in #dialog-overlay, not .generic-dialog:
 - This seems to work fine as is.
 - ex. Sleep menu

Sibling of #screen
 - Broken, overlays the SHB.
 - ex. gaia-confirm web component

Child of App window:
 - Works, but brittle. If we set to absolute position, it breaks when it probably shouldn't (we already use top/left/bottom).
 - ex. "..." options menu in system browser.

I think we can fix most of these issues with a couple of structural changes. I'm looking into this presently.

Comment 14

[Alberto Pastor [:albertopq]]

Hey Michael, Etienne made a great point on https://bugzilla.mozilla.org/show_bug.cgi?id=1074033#c31. We should make sure everything works in landscape mode as well.

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Alberto Pastor [:albertopq] from comment #14)
> Hey Michael, Etienne made a great point on
> https://bugzilla.mozilla.org/show_bug.cgi?id=1074033#c31. We should make
> sure everything works in landscape mode as well.

That's a good point. I noticed in bug 1074608 that permission prompts in landscape were just broken with SHB enabled, and I imagine there are many more prompts in this state.

Comment 16

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Gaia PR] modal dialogs use layout manager

This patch makes modal dialogs rely on the layout manager, similar to system dialogs. This way, we can depend on the height of any dialogs inside #dialog-overlay to have a height that takes into consideration the SHB. Any dialogs outside of this container need to be fixed on a case by case basis.

All, please review and try to find bugs with this approach. I won't land until I write some tests, but I want to start the review process in the meantime.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8499941 [details] [review]
[Gaia PR] modal dialogs use layout manager

good investigation. r+ and We need a better solution afterwards.
* Refactor all these stuff into a class inherited from SystemDialog so SystemDialogManager could manage the layout correctly and once for all.
** ModalDialog -> SystemModalDialog comparing with AppModalDialog
** PermissionDialog should be moved into AppWindow (permission comes from an app)
** SimPinDialog should be instantiable and moved into AppWindow.
** CrashDialog should be a SystemDialog subclass.
** Not sure: icc, quicksettings(why is this here?), ime-menu, sleep-menu

Comment 19

[Alberto Pastor [:albertopq]]

Comment on attachment 8499941 [details] [review]
[Gaia PR] modal dialogs use layout manager

Look good to me. Just found a case in which it doesn't work:

- Open the Browser app and click on a Top site
- Click the menu button
- Switch to landscape

Not sure if you prefer to create a new bug for this.

Comment 22

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/dfb0ab3d73ffe78b35ed56be3bbcfd8f2fb24a3f

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Gaia PR] modal dialogs use layout manager

I used a different PR since the old one had the wrong bug number. But the code was the same, so R+ carries.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Probably bug 1059967.

[User impact] if declined:
Many system dialogs are currently broken. See comment 13 for the list of things this patch addresses.

[Testing completed]:
Manually testing on 2.2 and 2.1. There are some integration tests currently in place for system dialogs that this doesn't break. I am also working on a integration test for this bug in https://github.com/mozilla-b2g/gaia/pull/24965, but properly mocking the RIL here in the system app has proven difficult. So since this patch was already r+, I decided to land this now to give QA time to run through some smoke tests while I finished the integration test in bug 1077579.

[Risk to taking this patch] (and alternatives if risky):
This patch is risky because it touches many system dialogs. However, we need this patch because there are a lot of cases currently broken and at the very least this fixes many of the cases (fullscreen vs non, landscape vs. portrait, etc). In bug 1077579 we are working on getting better test coverage for SHB issues so they don't keep biting us going forward.

[String changes made]: none.

Comment 24

[bhavana bajaj [:bajaj]]

(In reply to Michael Henretty [:mhenretty] from comment #23)
> Created attachment 8502244 [details] [review]
> [Gaia PR] modal dialogs use layout manager
> 
> I used a different PR since the old one had the wrong bug number. But the
> code was the same, so R+ carries.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):
> Probably bug 1059967.
> 
> [User impact] if declined:
> Many system dialogs are currently broken. See comment 13 for the list of
> things this patch addresses.
> 
> [Testing completed]:
> Manually testing on 2.2 and 2.1. There are some integration tests currently
> in place for system dialogs that this doesn't break. I am also working on a
> integration test for this bug in
> https://github.com/mozilla-b2g/gaia/pull/24965, but properly mocking the RIL
> here in the system app has proven difficult. So since this patch was already
> r+, I decided to land this now to give QA time to run through some smoke
> tests while I finished the integration test in bug 1077579.
> 
> [Risk to taking this patch] (and alternatives if risky):
> This patch is risky because it touches many system dialogs. However, we need
> this patch because there are a lot of cases currently broken and at the very
> least this fixes many of the cases (fullscreen vs non, landscape vs.
> portrait, etc). In bug 1077579 we are working on getting better test
> coverage for SHB issues so they don't keep biting us going forward.
> 
> [String changes made]: none.

Can you please work with QA to verify this or get some testing done on trunk prior to 2.1 uplift ? I think we are short staffed tomorrow, but lets see if tony can help find someone so the basics are tested here given the risk.

Comment 25

[Tony Chung [:tchung]]

kevin, we need a tester to verifyme this patch on trunk, before uplifting to 2.1.  can you please assign?  thanks, Tony

Comment 26

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8502244 [details] [review]
[Gaia PR] modal dialogs use layout manager

This is not ready for upflift. :gerard-majax found that the SHB overlaps the update screen.

Comment 27

[Michael Henretty [:mikehenrty][:mhenretty]]

This needs another patch. Working on that now.

Comment 28

[Duane Dixon [:ddixon]]

Bug DOES occur in latest 2.2 Flame Master (Full Flash). 

Actual Results: User Restarts device, unlocks screen, issue occurs.  

Note: These results are the same as "Screenshot error" in the Attachments field above. 

Nightly Full Flash
Device: Flame Master
Build ID: 20141010040202
Gaia: 1036b544b7e102592bd9fab95cd9317329ac1293
Gecko: 50b689feab5f
Version: 35.0a1 (Master)
Firmware Version: L1TC10011800
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 29

[Duane Dixon [:ddixon]]

Please disregard Comment 28. 

After further testing, I cannot reproduce this bug using the latest Flame Master build (Nightly, Full Flash). 

Repro attempts: 0/10

Environmental Variables:

Device: Flame Master
Build ID: 20141010040202
Gaia: 1036b544b7e102592bd9fab95cd9317329ac1293
Gecko: 50b689feab5f
Version: 35.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Comment 30

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [Gaia PR] fix update and quick settings dialogs

We were missing both the update manager, and quick settings dialogs. Fixed and added tests for each.

Kevin can you take a look?

Comment 31

[Kevin Grandon :kgrandon]

Comment on attachment 8503388 [details] [review]
[Gaia PR] fix update and quick settings dialogs

Sounds good to me. Thanks!

Comment 32

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/cc448fecc6d27347135911ef94cdeab5b9aae655

Comment 33

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8502244 [details] [review]
[Gaia PR] modal dialogs use layout manager

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
Probably bug 1059967.

[User impact] if declined:
Many system dialogs are currently broken. See comment 13 for the list of things this patch addresses.

[Testing completed]:
Manually testing on 2.2 and 2.1. There are some integration tests currently in place for system dialogs that this doesn't break. I am also working on a integration test for this bug in https://github.com/mozilla-b2g/gaia/pull/24965, but properly mocking the RIL here in the system app has proven difficult. So since this patch was already r+, I decided to land this now to give QA time to run through some smoke tests while I finished the integration test in bug 1077579.

[Risk to taking this patch] (and alternatives if risky):
This patch is risky because it touches many system dialogs. However, we need this patch because there are a lot of cases currently broken and at the very least this fixes many of the cases (fullscreen vs non, landscape vs. portrait, etc). In bug 1077579 we are working on getting better test coverage for SHB issues so they don't keep biting us going forward. We also had QA manually test this, and it is working. See comment 29.

[String changes made]: none.

Comment 34

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8503388 [details] [review]
[Gaia PR] fix update and quick settings dialogs

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
bug 1059967.


[User impact] if declined:
Broken quick-settings and update dialogs in system app.

[Testing completed]:
Manual testing, and 2 new integration tests.

[Risk to taking this patch] (and alternatives if risky):
This is a follow-up to the original patch where we noticed we had missed two dialog types. This patch is very low risk since it is css that is very specific to the dialogs it applies to, and it has integration tests.

[String changes made]: none.

Comment 36

[Duane Dixon [:ddixon]]

Issue is verified fixed in Flame 2.2 (Full Flash, Nightly) 

Actual Results: SIM pin UI functions correctly with the Software Home Button enabled. 

Device: Flame Master
Build ID: 20141014040203
Gaia: 4f86c631e0465c0e56ccebeb1324fd28be9ea32f
Gecko: 54217864bae9
Version: 36.0a1 (Master)
Firmware Version: v180
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Leaving "verifyme" tag until 2.1 fix is uplifted.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/96cf6c041a909d2c3634efacfb9673a68939bded
v2.1: https://github.com/mozilla-b2g/gaia/commit/8b573b0f1693784411d721e689772f6c7c098c6c

Comment 38

[Alberto Pastor [:albertopq]]

I can still repro https://bugzilla.mozilla.org/show_bug.cgi?id=1074123#c11 in both 2.1 and 2.2 (not 100% times). Should we open a new bug, Michael?

Comment 39

[Michael Henretty [:mikehenrty][:mhenretty]]

Yes please do and assign me. Thanks.

Comment 40

[Brogan Zumwalt [Inactive]]

Issue is verified fixed on Flame 2.1 KK

Actual Results: SIM PIN UI appears correctly with SHB enabled. There is a 1-3 px gap between the number pad and "OK"/"Skip" buttons, but is not readily noticeable.

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141022001201
Gaia: 3d9cc667f4e929861a9a77c41096bbf5a9c1bde0
Gecko: 928b18f7d8ff
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0