1100341 - [Statusbar][Edge gestures] Icons overlay when performing edge gestures while app opening

Status: VERIFIED FIXED

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

Description

[Alberto Pastor [:albertopq]]

Attachment: Screenshot

STR:

1.- Open any app
2.- Perform an edge gesture while the app is opening

Expected:

The statusbar icons shouldn't be shown twice

Actual:

There are 2 statusbar icons when doing the edge gesture.

Comment 1

[Manuel Casas Barrado [:mancas]]

Hey Etienne, what do you think about if set |lifecycleEnabled| once the app is completely opened instead of doing it in the |launchapp| event? This way we prevent the user to switch between apps that are being opened and of course, this fix will solve the issue.

Comment 2

[Gregor Wagner [:gwagner]]

Is this still happening? I believe this is fixed now.

Comment 3

[Manuel Casas Barrado [:mancas]]

Gregor, I've could reproduce this issue with the latest master.

Comment 4

[Alberto Pastor [:albertopq]]

Yep, I can still repro this

Comment 5

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #1)
> Hey Etienne, what do you think about if set |lifecycleEnabled| once the app
> is completely opened instead of doing it in the |launchapp| event? This way
> we prevent the user to switch between apps that are being opened and of
> course, this fix will solve the issue.

Sounds good!
I think we should remove the `launchapp` case, and change the `appopen` one to `appopened`.
I can't remember why we didn't do that already, so let's be extra careful about edge cases.

Comment 6

[Manuel Casas Barrado [:mancas]]

Etienne, I've found an issue that this approach doesn't solve. If we open any app and then we open Email app, the app is opened and you can start using edge gestures, however, the status bar color is changed right after the app opened event, so |apptitlestatechanged| is thrown and the |hidden| class of the statusbar is removed making it visible while dragging the pages from left to right or viceversa.

How can we face this behaviour?

Comment 7

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #6)
> Etienne, I've found an issue that this approach doesn't solve. If we open
> any app and then we open Email app, the app is opened and you can start
> using edge gestures, however, the status bar color is changed right after
> the app opened event, so |apptitlestatechanged| is thrown and the |hidden|
> class of the statusbar is removed making it visible while dragging the pages
> from left to right or viceversa.
> 
> How can we face this behaviour?

Maybe we shouldn't remove the hidden class between the sheets-gesture-begin and the sheets-gesture-end event.

Comment 8

[Manuel Casas Barrado [:mancas]]

Attachment: Proposed patch

Hey Etienne, we have a first patch. Could you review it when you get a chance? Also, check my comments in github about one of the old unit tests

Thanks

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8529025 [details] [review]
Proposed patch

comments on github :)

Comment 10

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8529025 [details] [review]
Proposed patch

I took into account your comments. Could you review the patch please?

Thanks!

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8529025 [details] [review]
Proposed patch

Ha! I'm really sorry I missed it earlier, the gesture-begin/end events aren't symmetrical, you can get multiple begin and only one end.

So the statusbar stays "paused" and going back to the homescreen won't show the statusbar...(after doing multiple edge swipes)

There's no easy and clean fix, I think our best option is to keep a _pausedForGesture variable to make sure we only call "pauseUpdate" once, and then reset it when we get the gesture-end event.

(Please not that to test this you'll want to displatch multiple begin events, not set the _internal var from the test).

Thanks!

Comment 12

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8529025 [details] [review]
Proposed patch

Eitenne, the issue should be fixed now. Review it when you get a chance

Thanks!

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8529025 [details] [review]
Proposed patch

Sorry this is taking so long, almost there! Made one last comment on github.

Comment 14

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8529025 [details] [review]
Proposed patch

Hey Etienne, thanks for your patience. I've restored the method as you said in the comments you left on github.

Thanks!

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8529025 [details] [review]
Proposed patch

\o/

Comment 16

[Joshua Mitchell (Inactive)]

QA-Wanted: Can we check that this is not present on 2.1 and 2.0 KK

Comment 17

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

Besides Flame 2.2, this issue also occurs on Flame 2.1.

Observed behavior: With one app in the background, cold launch another app and perform an edge gesture and hold finger on screen while it finishes launching causes status bar icons to overlap.

Device: Flame 2.1 (shallow flash, 512MB mem)
BuildID: 20141209084847
Gaia: 155424d51cf9bb2ea41dc6667f94741f82f35bc0
Gecko: fe5329227c54
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

---------

This issue does NOT occur on Flame 2.0. Status bar stays intact when edge gesturing; it's not part of the edge gesturing window and it is independent.

Device: Flame 2.0 (shallow flash, 512MB mem)
BuildID: 20141208061237
Gaia: 856863962362030174bae4e03d59c3ebbc182473
Gecko: 2d0860bd0225
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

-----

Not sure if this is a regression since 2.1 and 2.2 seem to display status bar WITH the edge gesturing window, as opposed to 2.0 that the status bar is independent from edge gesturing.

Comment 18

[Joshua Mitchell (Inactive)]

Manuel - this bug might have gotten lost in the shuffle, it looks like your 2.2 patch was last +'d. 
but this issue is also present in 2.1

Comment 19

[Manuel Casas Barrado [:mancas]]

Attachment: Proposed patch for v2.1

Hey Etienne, could you review the patch for v2.1?

Thanks!

Comment 20

[Etienne Segonzac (:etienne)]

Comment on attachment 8536416 [details] [review]
Proposed patch for v2.1

lgtm, you'll need to ask for approval though :)

Comment 21

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8536416 [details] [review]
Proposed patch for v2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): No regression
[User impact] if declined: Bad UX experience. The statusbar icons overlay the rest of the screen when the user performs an edge gesture. However this is an edge case.
[Testing completed]: Yes, unit tests and manual testing
[Risk to taking this patch] (and alternatives if risky): Low/Medium
[String changes made]:No

Comment 22

[Manuel Casas Barrado [:mancas]]

Etienne, I've noticed that we have an issue with one of the system integration test [1]. As you can see the statusbar is paused, so when we come back to the homescreen this condition is true [2] which means a new reflow. What do you think about changing the reflows counts of the test to match 1 reflow instead of 0?

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/homescreen_navigation_test.js#L58

[2] https://github.com/mozilla-b2g/gaia/pull/26481/files#diff-293e71c28c7a1b418148d8af71d58804R587

Comment 23

[bhavana bajaj [:bajaj]]

I think this is bad enough that we'd want to uplift to 2.1.

I am waiting for eitenne to comment on the test failure before we uplift this to 2.1 and also NI'ing QA to verify this on master.

Comment 24

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #22)
> Etienne, I've noticed that we have an issue with one of the system
> integration test [1]. As you can see the statusbar is paused, so when we
> come back to the homescreen this condition is true [2] which means a new
> reflow. What do you think about changing the reflows counts of the test to
> match 1 reflow instead of 0?
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/marionette/
> homescreen_navigation_test.js#L58
> 
> [2]
> https://github.com/mozilla-b2g/gaia/pull/26481/files#diff-
> 293e71c28c7a1b418148d8af71d58804R587

Ha!
In |resumeUpdate()|, shouldn't we call |_updateIconVisibility()| only if |this._paused === 0|?
The _paused count was introduced exactly for this, since the test is pausing the statusbar other events should not cause it to unpause.

Comment 25

[Manuel Casas Barrado [:mancas]]

(In reply to Etienne Segonzac (:etienne) from comment #24)
> Ha!
> In |resumeUpdate()|, shouldn't we call |_updateIconVisibility()| only if
> |this._paused === 0|?
> The _paused count was introduced exactly for this, since the test is pausing
> the statusbar other events should not cause it to unpause.

In that case, we should change this line:
https://github.com/mozilla-b2g/gaia/pull/26481/files#diff-293e71c28c7a1b418148d8af71d58804R587

to this:

if (this._pausedForGesture)

I was using the pause state because of the issue we have with the asymmetric events of the edge gestures

Comment 26

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #25)
> (In reply to Etienne Segonzac (:etienne) from comment #24)
> > Ha!
> > In |resumeUpdate()|, shouldn't we call |_updateIconVisibility()| only if
> > |this._paused === 0|?
> > The _paused count was introduced exactly for this, since the test is pausing
> > the statusbar other events should not cause it to unpause.
> 
> In that case, we should change this line:
> https://github.com/mozilla-b2g/gaia/pull/26481/files#diff-
> 293e71c28c7a1b418148d8af71d58804R587
> 
> to this:
> 
> if (this._pausedForGesture)
> 
> I was using the pause state because of the issue we have with the asymmetric
> events of the edge gestures

Don't think so. _pausedForGesture should only be used to prevent calling |pauseUpdate| more than once in sheets-gesture-begin.

Comment 27

[Manuel Casas Barrado [:mancas]]

(In reply to Etienne Segonzac (:etienne) from comment #26)
> (In reply to Manuel Casas Barrado [:mancas] from comment #25)
> > (In reply to Etienne Segonzac (:etienne) from comment #24)
> > > Ha!
> > > In |resumeUpdate()|, shouldn't we call |_updateIconVisibility()| only if
> > > |this._paused === 0|?
> > > The _paused count was introduced exactly for this, since the test is pausing
> > > the statusbar other events should not cause it to unpause.
> > 
> > In that case, we should change this line:
> > https://github.com/mozilla-b2g/gaia/pull/26481/files#diff-
> > 293e71c28c7a1b418148d8af71d58804R587
> > 
> > to this:
> > 
> > if (this._pausedForGesture)
> > 
> > I was using the pause state because of the issue we have with the asymmetric
> > events of the edge gestures
> 
> Don't think so. _pausedForGesture should only be used to prevent calling
> |pauseUpdate| more than once in sheets-gesture-begin.

Yep you're right. However, if we are switching apps so fast and quickly we press the home button, the sheets-gesture-ends will not be received and the status bar still is in pause. Because of that, I was using |this.isPaused()| to resume the statusbar when we go back to the homescreen.

Comment 28

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #27)
> (In reply to Etienne Segonzac (:etienne) from comment #26)
> > (In reply to Manuel Casas Barrado [:mancas] from comment #25)
> > > (In reply to Etienne Segonzac (:etienne) from comment #24)
> > > > Ha!
> > > > In |resumeUpdate()|, shouldn't we call |_updateIconVisibility()| only if
> > > > |this._paused === 0|?
> > > > The _paused count was introduced exactly for this, since the test is pausing
> > > > the statusbar other events should not cause it to unpause.
> > > 
> > > In that case, we should change this line:
> > > https://github.com/mozilla-b2g/gaia/pull/26481/files#diff-
> > > 293e71c28c7a1b418148d8af71d58804R587
> > > 
> > > to this:
> > > 
> > > if (this._pausedForGesture)
> > > 
> > > I was using the pause state because of the issue we have with the asymmetric
> > > events of the edge gestures
> > 
> > Don't think so. _pausedForGesture should only be used to prevent calling
> > |pauseUpdate| more than once in sheets-gesture-begin.
> 
> Yep you're right. However, if we are switching apps so fast and quickly we
> press the home button, the sheets-gesture-ends will not be received and the
> status bar still is in pause. Because of that, I was using |this.isPaused()|
> to resume the statusbar when we go back to the homescreen.

Oh sorry, it makes perfect sense then :) Can you add a small comment in the |homescreenopened| case explaining that we might miss the |sheets-gesture-end|? Thanks!

Comment 29

[bhavana bajaj [:bajaj]]

mancas/eitenne, is this ready to land on the 2.1 branch? The last few comments are slightly unclear in terms of I should be expecting a revised patch here....

Comment 30

[Etienne Segonzac (:etienne)]

(In reply to bhavana bajaj [:bajaj] from comment #29)
> mancas/eitenne, is this ready to land on the 2.1 branch? The last few
> comments are slightly unclear in terms of I should be expecting a revised
> patch here....

Sorry about that.
Just took a look and we're good to go!
Commit 5be2f725bc28e38e5e6db32b865b753c46bc3009 has all the latest comments addressed.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/0fe45b16a3719977d370744da00119a672fde717
v2.1:   https://github.com/mozilla-b2g/gaia/commit/b04a8cb7b2482e0a44e6702b48c42283a00b5b1e

Comment 32

[Paladin]

Attachment: one_statusbar.3gp

The problem is verified not happen in latest build of Flame 2.1/2.2
See Attachment: one_statusbar.3gp
Reproducing rate: 0/5

Flame 2.1 build:
Gaia-Rev        b04a8cb7b2482e0a44e6702b48c42283a00b5b1e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/99cea2c818f6
Build-ID        20150107001244
Version         34.0
---------------------------
Flame 2.2 build:
Gaia-Rev        69ac77cfa938fae2763ac426a80ca6e5feb6ad25
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/33781a3a5201
Build-ID        20150107010216
Version         37.0a1

Comment 33

[Elie]

Attachment: VIDEO0214.mp4

This issue dose not exist in Flame 2.1&2.2
Flame 2.1 build:
Gaia-Rev        b04a8cb7b2482e0a44e6702b48c42283a00b5b1e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/99cea2c818f6
Build-ID        20150107001244
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150107.035133
FW-Date         Wed Jan  7 03:51:45 EST 2015
Bootloader      L1TC000118D0

Flame 2.2 build:
Gaia-Rev        69ac77cfa938fae2763ac426a80ca6e5feb6ad25
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/33781a3a5201
Build-ID        20150107010216
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150107.044401
FW-Date         Wed Jan  7 04:44:12 EST 2015
Bootloader      L1TC000118D0

Comment 34

[Shine(Leave from Mozilla)]

According to comment 33 and my own verification, the problem is fixed now, mark as VERIFIED FIXED.