1095142 - [SHB] Dialer screen does not fully cover lockscreen

Status: VERIFIED FIXED

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

Description

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

Attachment: [screenshot] weird callscreen

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.

Comment 1

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

[Blocking Requested - why for this release]:
Broken UI.

Comment 2

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

Alberto, this will most likely block 2.1. Can you take a look?

Comment 3

[KTucker [:KTucker][Inactive 3/4/2016]]

I think this issue is a dupe of bug 1068470 which is still not fixed on 2.1

Comment 4

[Alberto Pastor [:albertopq]]

It worked for me with latest 2.1 pvt build. Waiting for Michael's input for closing it as dupe.

Comment 5

[Michael Henretty [:mikehenrty][: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

Comment 6

[Guillaume Marty [:gmarty]]

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?

Comment 7

[Guillaume Marty [:gmarty]]

Talked to Rob offline and the SHB should always be visible even after the screen has been locked and unlocked.

Comment 8

[Guillaume Marty [:gmarty]]

Attachment: 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?

Comment 9

[Etienne Segonzac (:etienne)]

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?

Comment 10

[Guillaume Marty [:gmarty]]

Comment on attachment 8525438 [details] [review]
Github PR

Now with added tests. How does it look?

Comment 11

[Etienne Segonzac (: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.

Comment 12

[Guillaume Marty [:gmarty]]

Comment on attachment 8525438 [details] [review]
Github PR

Kevin, can you take over the review from Alive while he's away?

Comment 13

[Kevin Grandon :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.

Comment 14

[Guillaume Marty [:gmarty]]

As Kevin said, landing as is for now because of the deadline.
In master: https://github.com/mozilla-b2g/gaia/commit/012a9d7a77942f3cf267b60c6a2c756c2b8bfea0

Comment 15

[Guillaume Marty [:gmarty]]

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.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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?

Comment 17

[Guillaume Marty [: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.

Comment 18

[Yeojin Chung [:YeojinC]]

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.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

v2.1: https://github.com/mozilla-b2g/gaia/commit/f20bbb0e24c065499df176a390c6d0e45d90eeb3

Comment 20

[KTucker [:KTucker][Inactive 3/4/2016]]

Yeojin, please verify 2.1 on tomorrow's build.

Comment 21

[Elie]

Attachment: 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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Reverted for Linter failures.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=45918&repo=mozilla-b2g34_v2_1

v2.1: https://github.com/mozilla-b2g/gaia/commit/db2e84860f5a7cc334464618c6ea9e92ff82e9dd

Comment 23

[Guillaume Marty [:gmarty]]

Here is a patch that fixes both linting and tests:
https://github.com/mozilla-b2g/gaia/pull/26480

Comment 24

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/0e557640a58b7e5288ecbe23ac09d96c762bba0d

Comment 25

[Yeojin Chung [:YeojinC]]

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