1120733 - [Privacy Panel] The Back arrow button disappears after doing some actions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

Attachment: logcat on Flame 2.2

Related bug:
Bug 1113398 and Bug 1060174

Description:
Multiple actions could trigger the Privacy Panel to lose its back arrow button to return to Settings app. 
Looks like it runs as an individual app, but since it's nested under Settings, it could confuse the user when back button disappears.
I've also seen it happen after changing some settings within Privacy Panel, but repro rate isn't 100% so its STR isn't included; and I think it's probably related to either one of the STRs.

STR 1:
1) Navigate to Settings > Privacy Panel (dismiss the tutorial if displayed)
2) Long press Home button to bring up cards view
3) Tap Privacy Panel in cards view

STR 2:
1) Navigate to Settings > Privacy Panel (dismiss the tutorial if displayed)
2) Press power button to lock DUT
3) Press power button to unlock

Expected behavior for STR 1 & 2:
Back arrow button should always remain on Privacy Panel if it's launched via Settings

Actual behavior for STR 1 & 2:
Back arrow button disappears

Repro rate:
5/5 for both STRs.

Attaching a logcat doing both STR's.

Device: Flame 2.2
BuildID: 20150112010228
Gaia: f5e481d4caf9ffa561720a6fc9cf521a28bd8439
Gecko: bb8d6034f5f2
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 1

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

Video:
http://youtu.be/0WJTQyzJxLA

Comment 2

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

Buttons should not disappear. This can lead to end user confusion. Nominating 2.2?

Comment 3

[Gerry Chang [:gchang]]

NI developer for this.

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

EJ, any ideas?

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi QA, for PP related bugs, if they are not happened in Settings app, please ni? Marta([e-mail]) for more information because DT is mainly character who develops this feature. THanks :)

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #5)
> Hi QA, for PP related bugs, if they are not happened in Settings app, please
> ni? Marta([e-mail]) for more information because DT is
> mainly character who develops this feature. THanks :)
  ~~~~~~~~~~~~~~~~
typo:    main role xD"

Comment 7

[Manuel Casas Barrado [:mancas]]

Attachment: Proposed patch

Hey Arthur, we need to preserve the state of the |privacy.panel.launched.by.settings| even if we lost focus. However we need to set it to |false| when the app is closed. Is there any way to execute code before the end of the application?

Thanks

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

EJ, could you help check the patch? Thanks.

Comment 9

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

This patch looks nice to me, can we add one more test case for this patch ?

Thanks :)

Comment 10

[Manuel Casas Barrado [:mancas]]

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

EJ, can you review the patch? I took into account your comments and I've also added a unit test to check if the settings flag is updated with the events.

Thanks for your time! =)

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Manuel,

I just realized that PP app has been moved out from homescreen last month !! (https://github.com/mozilla-b2g/gaia/commit/b84a94ee25edaf3ee32461ea3299d57ff1a6a4f0)

If that change has been approved, then it means that we don't have any entry point other than Settings app. With this design, I think we should follow how we did in Settings/keyboard (https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/settings/keyboard_settings_app.js#L100) - We should close PP app when visibility got changed.

By doing so, we can make this cross-app behavior more consistent in Settings app and we don't have to do so many hacks in PP at the same time.

What do you think ?

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

Please feel free to put r? on me after we clarified some questions :)

Comment 13

[Manuel Casas Barrado [:mancas]]

Good catch! But just one question, in that case, the back arrow button should always be displayed no matter the setting flag, am I right?

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Yes, you are right !

Because "settings -> privacy panel" would be the only path, we should always let the arrow button there.
In this way, we don't even need that settings key ! woooo

Comment 15

[Manuel Casas Barrado [:mancas]]

Hey EJ, I'm doing a refactor to remove the setting key, however, when using the blur event to close the Privacy Panel App, the second time I try to enter in it, the screen  disappear. In the other hand, using |visibilitychange| event, it works fine but it takes a long time, when opening the cardview per example, to close the app, so the visual effect is strange. Any ideas?

If you want more information, don't hesitate to contact me =)

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hey @Manuel,

Keyboard uses `visibilitychange` (https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/settings/keyboard_settings_app.js#L118) to handle this part so I think we can also use this event to close PP app.

And yes, because PP app is opened as a separate app, it takes a while to receive `visibilitychange` event and kill itself, this is normal if we follow this way to kill apps.

Comment 17

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hope this helps ! :)

Comment 18

[Manuel Casas Barrado [:mancas]]

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

I think we are really close. Could you take a look at the new patch? 

Thanks! =)

Comment 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

Great !! This patch looks nice to me and please make sure CI is green before merge.

Thanks Manuel, r+++.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

In the future, please don't resolve bugs that haven't landed on master yet.

Also, the Gaia Try run has real-looking Gaia unit test failures.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=441828&repo=gaia-try

Comment 21

[Jamie Charlton (:Jamie_)]

this is happening in 2.2 with


appid	        {3c2e2abc-06d4-11e1-ac3b-374f68613e61}
apptype	        b2g
vendor	        Mozilla
name	        B2G
version	        2.2.0.0-prerelease
appbuildid	20150126002536
platformbuildid	20150126002536
platformversion	37.0a2
geckobuildid	20150126002536
geckoversion	37.0a2
changeset	c6aa604a7967

also with the 3.0 with

appid	        {3c2e2abc-06d4-11e1-ac3b-374f68613e61}
apptype	        b2g
vendor	        Mozilla
name	        B2G
version	        3.0.0.0-prerelease
appbuildid	20150126010231
platformbuildid	20150126010231
platformversion	38.0a1
geckobuildid	20150126010231
geckoversion	38.0a1
changeset	fa91879c8428

Comment 22

[marta]

there is a bug in the tests, fixed in bug 1105304. Once that lands, please rebase the patch here.

Comment 24

[Manuel Casas Barrado [:mancas]]

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

Hey EJ, after make a rebase we face to some conflicts. Could you take a quick look at the patch to ensure that everything is ok before we land it?

Thanks!

Comment 25

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

This looks okay to me ! f+ :)

Comment 26

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 27

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 28

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 29

[Francisco Jordano [:arcturus] [:francisco]]

:eragon can you land this bug?

The contributor doesn't have commit access for this, and the only people in the recommendation list for autolander are Vivien and Tim.

Thanks!

Comment 30

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

okay, this patch was merged into Gaia/master : https://github.com/mozilla-b2g/gaia/commit/29eee46654b9f83bb6cc28f05ac459e26071c925

Comment 31

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): no
[User impact] if declined: Users may get confused why PP would lose its back arrow sometimes and can't go back to Settings app.
[Testing completed]: with tests already this patch did remove some old tests.
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: no

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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