Closed
Bug 1074123
Opened 10 years ago
Closed 10 years ago
SIM PIN dialog margin bottom is wrong with soft home button enabled
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(blocking-b2g:2.1+, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | verified |
b2g-v2.2 | --- | verified |
People
(Reporter: apastor, Assigned: mikehenrty)
References
Details
(Keywords: regression, Whiteboard: [systemsfe])
Attachments
(5 files, 2 obsolete files)
68.61 KB,
image/png
|
Details | |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
13.34 KB,
image/png
|
Details | |
46 bytes,
text/x-github-pull-request
|
mikehenrty
:
review+
bajaj
:
approval-gaia-v2.1+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
kgrandon
:
review+
bajaj
:
approval-gaia-v2.1+
|
Details | Review |
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
Reporter | ||
Comment 1•10 years ago
|
||
Comment 3•10 years ago
|
||
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
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.2:
--- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted → regression
QA Contact: croesch
Comment 4•10 years ago
|
||
***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
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Reporter | ||
Comment 5•10 years ago
|
||
[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)
blocking-b2g: --- → 2.1?
Reporter | ||
Updated•10 years ago
|
Whiteboard: [systemsfe]
Updated•10 years ago
|
Attachment #8496761 -
Attachment mime type: text/plain → image/png
Updated•10 years ago
|
Attachment #8496761 -
Attachment mime type: image/png → text/plain
Reporter | ||
Comment 6•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8496761 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → apastor
Assignee | ||
Comment 8•10 years ago
|
||
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
Assignee: apastor → mhenretty
Assignee | ||
Comment 9•10 years ago
|
||
See comment 8.
Attachment #8498528 -
Flags: review?(alive)
Attachment #8498528 -
Flags: feedback?(apastor)
Comment 10•10 years ago
|
||
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?
Attachment #8498528 -
Flags: review?(alive) → review+
Reporter | ||
Comment 11•10 years ago
|
||
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
Reporter | ||
Updated•10 years ago
|
Attachment #8498528 -
Flags: feedback?(apastor)
Assignee | ||
Comment 12•10 years ago
|
||
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.
Attachment #8498528 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
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.
Reporter | ||
Comment 14•10 years ago
|
||
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.
Updated•10 years ago
|
Target Milestone: --- → 2.1 S6 (10oct)
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Comment 16•10 years ago
|
||
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.
Attachment #8499941 -
Flags: review?(alive)
Attachment #8499941 -
Flags: feedback?(eperelman)
Attachment #8499941 -
Flags: feedback?(apastor)
Comment 17•10 years ago
|
||
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
Attachment #8499941 -
Flags: review?(alive) → review+
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][COM=Gaia::System]
Reporter | ||
Comment 19•10 years ago
|
||
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.
Attachment #8499941 -
Flags: feedback?(apastor) → feedback+
Updated•10 years ago
|
Attachment #8499941 -
Flags: feedback?(eperelman) → feedback+
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
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.
Attachment #8499941 -
Attachment is obsolete: true
Attachment #8502244 -
Flags: review+
Attachment #8502244 -
Flags: approval-gaia-v2.1?
Comment 24•10 years ago
|
||
(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.
Flags: needinfo?(tchung)
Comment 25•10 years ago
|
||
kevin, we need a tester to verifyme this patch on trunk, before uplifting to 2.1. can you please assign? thanks, Tony
Flags: needinfo?(tchung) → needinfo?(ktucker)
Keywords: verifyme
Updated•10 years ago
|
Assignee | ||
Comment 26•10 years ago
|
||
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.
Attachment #8502244 -
Flags: approval-gaia-v2.1?
Assignee | ||
Comment 27•10 years ago
|
||
This needs another patch. Working on that now.
Comment 28•10 years ago
|
||
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
QA Whiteboard: [QAnalyst-Triage+][COM=Gaia::System] → [QAnalyst-Triage?][COM=Gaia::System]
Flags: needinfo?(jmitchell)
Comment 29•10 years ago
|
||
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
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?][COM=Gaia::System] → [QAnalyst-Triage+][COM=Gaia::System]
Flags: needinfo?(jmitchell)
Assignee | ||
Comment 30•10 years ago
|
||
We were missing both the update manager, and quick settings dialogs. Fixed and added tests for each.
Kevin can you take a look?
Attachment #8503388 -
Flags: review?(kgrandon)
Comment 31•10 years ago
|
||
Comment on attachment 8503388 [details] [review]
[Gaia PR] fix update and quick settings dialogs
Sounds good to me. Thanks!
Attachment #8503388 -
Flags: review?(kgrandon) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 33•10 years ago
|
||
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.
Attachment #8502244 -
Flags: approval-gaia-v2.1?
Assignee | ||
Comment 34•10 years ago
|
||
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.
Attachment #8503388 -
Flags: approval-gaia-v2.1?
Updated•10 years ago
|
Attachment #8502244 -
Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Updated•10 years ago
|
Attachment #8503388 -
Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Comment 36•10 years ago
|
||
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+][COM=Gaia::System] → [QAnalyst-Triage?][COM=Gaia::System]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Contact: ddixon
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?][COM=Gaia::System] → [QAnalyst-Triage+][COM=Gaia::System]
Flags: needinfo?(ktucker)
Comment 37•10 years ago
|
||
Reporter | ||
Comment 38•10 years ago
|
||
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?
Flags: needinfo?(mhenretty)
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite?
Comment 40•10 years ago
|
||
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
QA Whiteboard: [QAnalyst-Triage+][COM=Gaia::System] → [QAnalyst-Triage?][COM=Gaia::System]
Flags: needinfo?(ktucker)
Keywords: verifyme
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?][COM=Gaia::System] → [QAnalyst-Triage+][COM=Gaia::System]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•