[NFC][Software Home Button] yes/no button covered by SW home button

VERIFIED FIXED in 2.1 S9 (21Nov)

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: wachen, Assigned: mancas)

Tracking

unspecified
2.1 S9 (21Nov)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: [2.1-bug-bash][TPE][systemsfe])

Attachments

(6 attachments)

*** Build Information
Gaia-Rev        1e48e3e40e0780c0cd07a3457e5fe2efeeb542d1
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/09fb60a37850
Build-ID        20141023001201
Version         34.0
Device-Name     flame
Base image: ***Please update this field to say if you're running the v180 or the v188 base image***

*** Description
yes/no button covered by SW home button

*** Steps to Reproduce
1. enable a software home button in settings
2. enable NFC in settings app
3. pair a first time connect NFC earphone with the phone

*** Expected Results
I can see yes/no or confirm/cancel button

*** Actual Results
I can't see yes/no or confirm/cancel button

*** Other Notes

*** Reproduction Frequency: 100%
QA Whiteboard: [COM=NFC]
Whiteboard: [2.1-FC-bug-bash][TPE] → [2.1-bug-bash][TPE]
[Blocking Requested - why for this release]:

moving to SysFE team, who owns Soft Home button
Blocks: 1077579
blocking-b2g: --- → 2.1?
Component: NFC → Gaia::System
blocking-b2g: 2.1? → 2.1+
Assignee: nobody → b.mcb
Hey etienne, I've a question about how we want to solve this kind of situation. AFAIK, some dialogs (Sim pin and modal dialogs) recalculates the parent's height based on the layoutManager. This is good. However, the other ones, don't perform this operation. So the two possible options are:

1- Use window.layoutManager.height in every dialog, inside |dialog-overlay| element
2- Add this style to |dialog-overlay| element -> |height: calc(100% - var(--software-home-button-height) - var(--statusbar-height));|

The second option seems to be easier to fix all dialogs inside the parent element but does not take in account the keyboard height, per example.

What do you think?
Flags: needinfo?(etienne)
Option 1 should only apply to dialogs that live either inside the app window, or inside the dialog overlay container. These windows/divs get resized according to the app that is in the foreground, and therefore the layout manager height calculation applies. If they are direct children (or siblings) of #screen, we don't want to use the layout manager. I haven't looked at the NFC dialog, so I am not sure.

If we go with option 2, we need to make sure this dialog still works with fullscreen apps (or make sure this dialog never appears over fullscreen apps).
Whiteboard: [2.1-bug-bash][TPE] → [2.1-bug-bash][TPE][systemsfe]
Posted file Proposed patch
Attachment #8518853 - Flags: review?(timdream)
Posted image nfc-dialog.png
Now the dialog calculates the height based on the foreground app.
(In reply to Manuel Casas Barrado [:mancas] from comment #2)
> Hey etienne, I've a question about how we want to solve this kind of
> situation. AFAIK, some dialogs (Sim pin and modal dialogs) recalculates the
> parent's height based on the layoutManager. This is good. However, the other
> ones, don't perform this operation. So the two possible options are:
> 
> 1- Use window.layoutManager.height in every dialog, inside |dialog-overlay|
> element
> 2- Add this style to |dialog-overlay| element -> |height: calc(100% -
> var(--software-home-button-height) - var(--statusbar-height));|
> 
> The second option seems to be easier to fix all dialogs inside the parent
> element but does not take in account the keyboard height, per example.
> 
> What do you think?

Basically what Mike says :)
We're trying to move everything we can inside appWindows, and we should end with just 2-3 real system dialogs.
In the meantime, I think it's safe to go with the simpler solution.
Flags: needinfo?(etienne)
Target Milestone: --- → 2.1 S8 (7Nov)
Attachment #8518853 - Flags: review?(timdream) → review?(alive)
Comment on attachment 8518855 [details]
nfc-dialog-full-screen-app.png

Add an unit test please and then we are ok to go.
Attachment #8518855 - Flags: feedback+
Comment on attachment 8518853 [details] [review]
Proposed patch

comment 8
Attachment #8518853 - Flags: review?(alive) → feedback+
Target Milestone: 2.1 S8 (7Nov) → 2.1 S9 (21Nov)
Comment on attachment 8518853 [details] [review]
Proposed patch

I've added the test cases for the patch. Please check it and let me know if we have to change something

Thanks!
Attachment #8518853 - Flags: review?(alive)
Attachment #8518853 - Flags: review?(alive) → review+
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/25e4285f4c67198a3c3b20b18166bfd8862b6f4d

Please request Gaia v2.1 approval on this when you get a chance.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(b.mcb)
Keywords: checkin-needed
Resolution: --- → FIXED
Issue verified fixed on Flame 2.2

Actual Results: Confirmation buttons appear without being cut off by SHB

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141113040205
Gaia: be8b0151d2f9a4c41fc63952128e0b723cd1161d
Gecko: ab137ddd3746
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [COM=NFC] → [COM=NFC][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Leaving verify me for 2.1 uplift
Keywords: verifyme
QA Whiteboard: [COM=NFC][QAnalyst-Triage?] → [COM=NFC][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Posted file Patch v2.1
[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): No regression AFAIK
[User impact] if declined: If the device has software button enabled, the user will be unable to click the buttons when trying to connect to another device.
[Testing completed]: Manually and unit tests
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: No
Flags: needinfo?(b.mcb)
Attachment #8524541 - Flags: approval-gaia-v2.1?
Attachment #8524541 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
Posted video VIDEO0063[1].mp4
This issue has been successfully verified on Flame 2.1:
Gaia-Rev        f8d3bf44029e0afc0124600a4bb34dba8fc1ad21
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f70a67a7f846
Build-ID        20141120001207
Version         34.0
Device-Name     flame
FW-Release      4.4.2
(In reply to Elie from comment #16)
> Created attachment 8526477 [details]
> VIDEO0063[1].mp4
> 
> This issue has been successfully verified on Flame 2.1:
> Gaia-Rev        f8d3bf44029e0afc0124600a4bb34dba8fc1ad21
> Gecko-Rev      
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f70a67a7f846
> Build-ID        20141120001207
> Version         34.0
> Device-Name     flame
> FW-Release      4.4.2

Hi Elie,
I think the video you attached is not correct. 
I do not see Software Home button in your video.
Please verify again and attach correct video.
Flags: needinfo?(zikui.yang)
This issue is verified fixed on Flame 2.1.

Result: The confirmation dialog is displayed properly with the SHB enabled.

Device: Flame 2.1 (319mb, KK, Shallow Flash)
BuildID: 20141121001202
Gaia: 6c739275e963465658c18c7a9ebaa48cbe927d34
Gecko: 9bfc7a166a94
Version: 34.0 (2.1) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [COM=NFC][QAnalyst-Triage+] → [COM=NFC][QAnalyst-Triage?]
Flags: needinfo?(zikui.yang) → needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [COM=NFC][QAnalyst-Triage?] → [COM=NFC][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.