1159885 - Status bar is not visible on multiple attention windows

Status: VERIFIED FIXED

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

Description

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: logcat of issue

Description:
This seems to be a re-occurrence of bug 1108152. Status bar is not visible when 1) initiating a call (note that it IS visible when receiving a call), 2) an alarm is ringing on screen, 3) pairing with another device via bluetooth

STR:
1) Go to Phone app and dial out a number

Expected: Status bar is visible

Actual: It is not visible

Repro rate: 5/5

Video:
https://www.youtube.com/watch?v=KftvvsRH7Ek

Additional areas where this issue occurs on:
- Set an alarm in Clock app, and observe the screen when alarm is activating
- Go to Settings > enable Bluetooth and attempt to pair with another device. Observe the pairing request screen. I have seen status bar appearing on this screen, but most of the time it does not appear.

Device: Flame 3.0 Master (full flashed 319MB KK)
BuildID: 20150429010205
Gaia: 6e35b0948c42a4398b8a5916015de167121683a1
Gecko: 1ad65cbeb2f4
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Comment 1

[Pi Wei Cheng [:piwei] (inactive)]

v2.2 is also affected.

Device: Flame 2.2
BuildID: 20150429002501
Gaia: 1b7aa7e60788668ed09abf76022dfa231dbe88d4
Gecko: d38ff4717f39
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

----

v2.1 is NOT affected. Checked the three areas mentioned in the bug and status bar is visible on all of them.

Device: Flame 2.1
BuildID: 20150429001202
Gaia: 9fda4aec7f9495a27a335ccaf3b1a4dc9c4c6db0
Gecko: 38ab00c01159
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 2

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

The user should always see their status bar. This is a regression so nominating this 2.2?

Comment 3

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

B2g Inbound

Last Working
Environmental Variables:
Device: Flame 3.0
BuildID: 20150227061344
Gaia: b3fe0b0741252e18fdceded00595bd559e6c2bf1
Gecko: 337a83bd43ea
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

First Broken
Environmental Variables:
Device: Flame 3.0
BuildID: 20150227062844
Gaia: 640712b2b5773ece064a4958cf812a3ff348ed06
Gecko: 79e9b035ae8d
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Last Working Gaia First Broken Gecko: Issue does NOT reproduce
Gaia: b3fe0b0741252e18fdceded00595bd559e6c2bf1
Gecko: 79e9b035ae8d

First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: 640712b2b5773ece064a4958cf812a3ff348ed06
Gecko: 337a83bd43ea

Gaia pushlog:
https://github.com/mozilla-b2g/gaia/compare/b3fe0b0741252e18fdceded00595bd559e6c2bf1...640712b2b5773ece064a4958cf812a3ff348ed06

This looks to have been caused by Bug 1128618

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

Guillaume, can you take a look at this please? This might have been caused by the work done for Bug 1128618.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1159885-Status-bar-is-not-visible-on-multiple-attention-windows > mozilla-b2g:master

Comment 5

[Guillaume Marty [:gmarty]]

Comment on attachment 8599962 [details] [review]
[gaia] gmarty:Bug-1159885-Status-bar-is-not-visible-on-multiple-attention-windows > mozilla-b2g:master

Etienne, ca you review this patch?

Comment 6

[Etienne Segonzac (:etienne)]

Comment on attachment 8599962 [details] [review]
[gaia] gmarty:Bug-1159885-Status-bar-is-not-visible-on-multiple-attention-windows > mozilla-b2g:master

r=me with some comments on github

Comment 7

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

gmarty is going on vacation, I'll take over this one in the meantime.

Comment 8

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

Attachment: [Gaia PR] updated patch to fix test

There was a failing integration test on this patch (task_manager_test.js). The problem is when we changed the way we fetch the current app in StatusBar.setAppearance from Service.currentApp to Service.query('getTopMostWindow'), there is a case where getTopMostWindow returns undefined. What happens is this:

TaskManager.exitToApp calls someApp.open('from-cardview')
...this eventually leads to the 'rocketbar-deactivated' event invoking StatusBar.setAppearance where Service.query('getTopMostWindow') returns undefined.

Now, before this change, this string of events would lead to Service.currentApp simply returning the homescreen (which is default with no active app I believe) and setting the appearance would be based on homescreen. My thinking here is, when 'getTopMostWindow' returns null like in the case when transition from cardsview to active app, we can just not set appearance.

Alive, what do you think? The feedback I am asking about is specifically:
https://github.com/mozilla-b2g/gaia/commit/63bc8e76ec03e880b5bb1a48a037db011fa8a1ba

Comment 9

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

Comment on attachment 8600453 [details] [review]
[Gaia PR] updated patch to fix test

Basically fine, but I would really like to see we have no special rules for maximized statusbar; what's blocking us to return true in AttentionWindow's appChrome.isMaximized() ?

The last resort to me is add this.app.isAttentionWindow check in AppChrome#isMaximized(), so statusbar.js does not need to involve too much window management details and differential

Comment 10

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

(In reply to Michael Henretty [:mhenretty] from comment #8)
> Created attachment 8600453 [details] [review]
> [Gaia PR] updated patch to fix test
> 
> There was a failing integration test on this patch (task_manager_test.js).
> The problem is when we changed the way we fetch the current app in
> StatusBar.setAppearance from Service.currentApp to
> Service.query('getTopMostWindow'), there is a case where getTopMostWindow
> returns undefined. What happens is this:
> 
> TaskManager.exitToApp calls someApp.open('from-cardview')
> ...this eventually leads to the 'rocketbar-deactivated' event invoking
> StatusBar.setAppearance where Service.query('getTopMostWindow') returns
> undefined.
> 
> Now, before this change, this string of events would lead to
> Service.currentApp simply returning the homescreen (which is default with no
> active app I believe) and setting the appearance would be based on
> homescreen. My thinking here is, when 'getTopMostWindow' returns null like
> in the case when transition from cardsview to active app, we can just not
> set appearance.
> 
> Alive, what do you think? The feedback I am asking about is specifically:
> https://github.com/mozilla-b2g/gaia/commit/
> 63bc8e76ec03e880b5bb1a48a037db011fa8a1ba

Use top most window is also the direction in my wip patch. I don't see any problem right now.

Comment 11

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

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> Comment on attachment 8600453 [details] [review]
> [Gaia PR] updated patch to fix test
> 
> Basically fine, but I would really like to see we have no special rules for
> maximized statusbar; what's blocking us to return true in AttentionWindow's
> appChrome.isMaximized() ?
> 
> The last resort to me is add this.app.isAttentionWindow check in
> AppChrome#isMaximized(), so statusbar.js does not need to involve too much
> window management details and differential

I'm afraid that moving that logic into isMaximized would have unintended consequences. For instance, rocketbar does it's expanding animation based on if app chrome isMaximized. Similarly, the utility tray opens the rocketbar search only if app chrome is not maximized. Are we sure we don't want to be able to expand the rocketbar when any attention window is up?

Comment 12

[Etienne Segonzac (:etienne)]

> > Basically fine, but I would really like to see we have no special rules for
> > maximized statusbar; what's blocking us to return true in AttentionWindow's
> > appChrome.isMaximized() ?
> > 
> > The last resort to me is add this.app.isAttentionWindow check in
> > AppChrome#isMaximized(), so statusbar.js does not need to involve too much
> > window management details and differential
> 
> I'm afraid that moving that logic into isMaximized would have unintended
> consequences. For instance, rocketbar does it's expanding animation based on
> if app chrome isMaximized. Similarly, the utility tray opens the rocketbar
> search only if app chrome is not maximized.

Yes, FTR Guillaume initially started looking at isMaximized for AttentionWindows but it made me uncomfortable too.
First, the chrome isn't maximized in our case :) But also isMaximized is changing through time where isHomescreen/isAttentionWindow isn't.
So I thought our special case here was much closer to the homescreen case than the maximized case.

Comment 13

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

Maybe I am not that familiar with the expand/collapsed/min/max state control here, so it's fine to keep the window type check in statusbar if you guys think it's necessary.

Comment 14

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

Comment on attachment 8600453 [details] [review]
[Gaia PR] updated patch to fix test

Let's get another review here since I moved some of the logic around slightly. Etienne, can you take a look?

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8600453 [details] [review]
[Gaia PR] updated patch to fix test

r=me with small comment on github

Comment 16

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

There are two commits in this PR, so I needed to merge manually.

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

Comment 17

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

Comment on attachment 8600453 [details] [review]
[Gaia PR] updated patch to fix test

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

[User impact] if declined:
User won't see statusbar details when on the attention screen (like when they are in a call).

[Testing completed]:
Manually tested, and added unit tests.

[Risk to taking this patch] (and alternatives if risky):
Risk is to the statusbar appearance, but there are no real alternatives to approach as far as I can tell.

[String changes made]: none.

Comment 18

[bhavana bajaj [:bajaj]]

(In reply to Michael Henretty [:mhenretty] from comment #17)
> Comment on attachment 8600453 [details] [review]
> [Gaia PR] updated patch to fix test
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #):
> bug 1128618.
> 
> [User impact] if declined:
> User won't see statusbar details when on the attention screen (like when
> they are in a call).
> 
> [Testing completed]:
> Manually tested, and added unit tests.
> 
> [Risk to taking this patch] (and alternatives if risky):
> Risk is to the statusbar appearance, but there are no real alternatives to
> approach as far as I can tell.
> 
> [String changes made]: none.

Requesting QA verification and some exploratory testing on master here. Will also give it sometime to bake and go through a round of smoke tests for nightlies tomorrow.

Comment 19

[Pi Wei Cheng [:piwei] (inactive)]

Adding qawanted so this will appear on our queue.

Comment 20

[Pi Wei Cheng [:piwei] (inactive)]

This issue is verified fixed on Flame 3.0. Status bar is displayed on callscreens, bluetooth pairing request windows, and alarm activating screens. Also tested around different apps, having alarm ringing while on different apps, calling and receiving calls via different apps... etc and saw no issues.

Device: Flame (KK, 319MB, Full flashed)
BuildID: 20150507064907
Gaia: 83b27f522642ea573c57e882657ab5c73d4b07f4
Gecko: 403e3c2380b5
Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed
Version: 40.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Leaving verifyme for v2.2 after uplift.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for v2.2 uplift.

Comment 22

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

v2.2: https://github.com/mozilla-b2g/gaia/commit/aac58a063e3e6acae6ba77fe4cec224fb69450bc

Comment 23

[helen（leave from Mozilla）]

Attachment: verified v2.2.mp4

This bug has been verified as pass on latest build of Flame v2.2 by the STR in Comment 0.

Results:
 Status bar is displayed on callscreens, bluetooth pairing request windows, and alarm activating screens.And tested around other apps, the above-mentioned three situatons shows no issues.
See attachment: verified v2.2.mp4
Reproduce rate: 0/10

Device: Flame 2.2 build(Pass)
Build ID               20150513162505
Gaia Revision          aac58a063e3e6acae6ba77fe4cec224fb69450bc
Gaia Date              2015-05-13 12:59:48
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7a4f3cb5bf7b
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150513.205814
Firmware Date          Wed May 13 20:58:25 EDT 2015
Bootloader             L1TC000118D0