Closed Bug 1095142 Opened 5 years ago Closed 5 years ago

[SHB] Dialer screen does not fully cover lockscreen

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

VERIFIED FIXED
2.1 S9 (21Nov)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: mikehenrty, Assigned: gmarty)

References

Details

(Whiteboard: [systemsfe])

Attachments

(3 files)

STR:

1.) Enable SHB and lockscreen
2.) Make a phone call
3.) Lock phone

Expected results:
Callscreen takes up entire height of device.

Actual result:
Lockscreen displays partially below the callscreen, see attachment.
[Blocking Requested - why for this release]:
Broken UI.
blocking-b2g: --- → 2.1?
Alberto, this will most likely block 2.1. Can you take a look?
Flags: needinfo?(apastor)
I think this issue is a dupe of bug 1068470 which is still not fixed on 2.1
Assignee: nobody → apastor
Flags: needinfo?(apastor)
It worked for me with latest 2.1 pvt build. Waiting for Michael's input for closing it as dupe.
Flags: needinfo?(mhenretty)
This is definitely still happening on my device. I flashed from pvt, and then flashed the latest v2.1 gaia. The weird thing is, sometimes it works sometimes it doesn't. ie, sometimes the window will resize after the turning on and off the lockscreen. In any case, you're right KTucker, this is a dupe of bug 1068470.

BuildID: 20141107001205
Gaia: f08c2743bdb875ece34aaaaa6b4a8b12bd67a635

https://github.com/mozilla-b2g/gaia/commit/f08c2743bdb875ece34aaaaa6b4a8b12bd67a635
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(mhenretty)
Resolution: --- → DUPLICATE
Duplicate of bug: 1068470
blocking-b2g: 2.1? → 2.1+
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: [SHB] Callscreen does not fully cover lockscreen → [SHB] Dialer screen does not fully cover lockscreen
I think we need some clarification from UX. The current behaviour is that, when a call is in progress, the SHB disappears after a lock/unlock sequence. This doesn't look consistent to me.

Rob, can you tell me what is supposed to happen after unlocking the screen? Should the status bar remains as it as before locking? Or should it disappears and the dialer be resized fullscreen?
Flags: needinfo?(rmacdonald)
Assignee: apastor → gmarty
Talked to Rob offline and the SHB should always be visible even after the screen has been locked and unlocked.
Flags: needinfo?(rmacdonald)
Target Milestone: --- → 2.1 S9 (21Nov)
Attached file Github PR
Here's the patch. It's kept to a minimal set of changes for uplifting to v2.1 but eventually, we'll want a refactoring to get rid of some CSS class set in screen.
Alive, I worked with Etienne on this patch, so can you review it please?
Attachment #8525438 - Flags: review?(etienne)
Attachment #8525438 - Flags: review?(alive)
Comment on attachment 8525438 [details] [review]
Github PR

Commented on github, mainly missing tests :)

Alive a bit more information about the approach:
we're having more and more special cases in `layoutManager.height` and looks like it would be way more manageable if the layoutManager had an optional argument so you could request height for the lockscreenwindow or an attentionwindow. Basically replacing a bunch of global state with a simple class condition.

We don't want to change everything in this 2.1+ patch of course, but do you like the direction?
Attachment #8525438 - Flags: review?(etienne)
Comment on attachment 8525438 [details] [review]
Github PR

Now with added tests. How does it look?
Attachment #8525438 - Flags: review?(etienne)
Comment on attachment 8525438 [details] [review]
Github PR

r=me with the comments addressed, I'd like to wait for Alive's feedback before landing though.
Attachment #8525438 - Flags: review?(etienne) → review+
Comment on attachment 8525438 [details] [review]
Github PR

Kevin, can you take over the review from Alive while he's away?
Attachment #8525438 - Flags: review?(alive) → review?(kgrandon)
Comment on attachment 8525438 [details] [review]
Github PR

If Alive is out, I think we are ok with landing this with Etienne's review due to the deadline. I'll leave my F+ here. Would prefer a cleaner abstraction around the height methods, but I think this is probably ok to land.
Attachment #8525438 - Flags: review?(kgrandon) → feedback+
As Kevin said, landing as is for now because of the deadline.
In master: https://github.com/mozilla-b2g/gaia/commit/012a9d7a77942f3cf267b60c6a2c756c2b8bfea0
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Comment on attachment 8525438 [details] [review]
Github PR

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Software home button and attention windows
[User impact] if declined: The software home button disappears on some edge cases of the attention window.
[Testing completed]: There are unit and marionette tests. Manual testing of the dialer should be done.
[Risk to taking this patch] (and alternatives if risky): We made sure to reduce the impact of the patch for the uplift, so the risk is low.
[String changes made]: None.
Attachment #8525438 - Flags: approval-gaia-v2.1?(bbajaj)
Keywords: verifyme
Attachment #8525438 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
Trying to cherry pick this to v2.1, there's a conflict that I don't feel confident in fixing for myself in apps/system/js/layout_manager.js:

<<<<<<< HEAD
/* global KeyboardManager, softwareButtonManager, System,
          AppWindowManager */
=======
/* global inputWindowManager, softwareButtonManager, Service, AttentionWindow */
>>>>>>> 012a9d7... Merge pull request #26297 from gmarty/Bug-1095142-Dialer-screen-does-not-fully-cover-lockscreen

I mean, it's all commented out so probably doesn't matter, but still. Could I get a branch-specific patch to apply to be sure it's all correct?
Flags: needinfo?(gmarty)
Just add AttentionWindow at the end of the list like this:
/* global KeyboardManager, softwareButtonManager, System, AppWindowManager, AttentionWindow */

This annotation is used by JSHint for linting, so no impact on code or tests.
Flags: needinfo?(gmarty) → needinfo?(kwierso)
See Also: → 1105043
This issue is verified fixed on Flame 2.2.

Result: SHB appears on the callscreen when the device is locked.

Device: Flame 2.2 (319mb, KK, Shallow Flash)
BuildID: 20141125040209
Gaia: 824a61cccec4c69be9a86ad5cb629a1f61fa142f
Gecko: acde07cb4e4d
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
=======================================================

However, I found another issue that SHB does not disappear on the lockscreen when the user selects SHB from the callscreen. I filed a new bug 1105043.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Yeojin, please verify 2.1 on tomorrow's build.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage-]
Flags: needinfo?(ktucker) → needinfo?(ychung)
Attached video VIDEO0044.mp4
This issue has been successfully verified on Flame 2.1:
Gaia-Rev        1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db893274d9a6
Build-ID        20141125001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
Here is a patch that fixes both linting and tests:
https://github.com/mozilla-b2g/gaia/pull/26480
Flags: needinfo?(gmarty)
This issue is verified fixed on Flame 2.1.

Result: SHB appears on the callscreen when the device is locked.

Device: Flame 2.1 (319mb, KK, Full Flash)
BuildID: 20141201001201
Gaia: ccb49abe412c978a4045f0c75abff534372716c4
Gecko: 18fb67530b22
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage-] → [QAnalyst-Triage?]
Flags: needinfo?(ychung) → needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Depends on: 1107355
You need to log in before you can comment on or make changes to this bug.