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

RESOLVED FIXED in 2.2 S6 (20feb)

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: pcheng, Assigned: mancas)

Tracking

({privacy})

unspecified
2.2 S6 (20feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments)

Posted file 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
(Reporter)

Comment 1

4 years ago
Video:
http://youtu.be/0WJTQyzJxLA
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: privacy
Buttons should not disappear. This can lead to end user confusion. Nominating 2.2?
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
NI developer for this.
Flags: needinfo?(arthur.chen)
EJ, any ideas?
Flags: needinfo?(arthur.chen) → needinfo?(ejchen)
Hi QA, for PP related bugs, if they are not happened in Settings app, please ni? Marta(marta@sec.t-labs.tu-berlin.de) for more information because DT is mainly character who develops this feature. THanks :)
Flags: needinfo?(ejchen) → needinfo?(marta)
(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(marta@sec.t-labs.tu-berlin.de) for more information because DT is
> mainly character who develops this feature. THanks :)
  ~~~~~~~~~~~~~~~~
typo:    main role xD"
(Assignee)

Updated

4 years ago
Assignee: nobody → b.mcb
(Assignee)

Comment 7

4 years ago
Posted file 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
Attachment #8550156 - Flags: feedback?(arthur.chen)
Comment on attachment 8550156 [details] [review]
Proposed patch

EJ, could you help check the patch? Thanks.
Attachment #8550156 - Flags: feedback?(arthur.chen) → feedback?(ejchen)

Updated

4 years ago
blocking-b2g: 2.2? → 2.2+

Updated

4 years ago
Status: NEW → ASSIGNED
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 :)
Attachment #8550156 - Flags: feedback?(ejchen) → feedback+
(Assignee)

Comment 10

4 years ago
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! =)
Attachment #8550156 - Flags: review?(ejchen)

Updated

4 years ago
Flags: needinfo?(marta)
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 ?
Flags: needinfo?(b.mcb)
Comment on attachment 8550156 [details] [review]
Proposed patch

Please feel free to put r? on me after we clarified some questions :)
Attachment #8550156 - Flags: review?(ejchen)
(Assignee)

Comment 13

4 years ago
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?
Flags: needinfo?(b.mcb) → needinfo?(ejchen)
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
Flags: needinfo?(ejchen)
(Assignee)

Comment 15

4 years ago
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 =)
Flags: needinfo?(ejchen)
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.
Flags: needinfo?(ejchen)
(Assignee)

Comment 18

4 years ago
Comment on attachment 8550156 [details] [review]
Proposed patch

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

Thanks! =)
Attachment #8550156 - Flags: review?(ejchen)
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+++.
Attachment #8550156 - Flags: review?(ejchen) → review+

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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
Status: RESOLVED → REOPENED
Flags: needinfo?(marta)
Keywords: checkin-needed
Resolution: FIXED → ---
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

4 years ago
there is a bug in the tests, fixed in bug 1105304. Once that lands, please rebase the patch here.
Flags: needinfo?(marta)

Updated

4 years ago
Depends on: 1105304
Duplicate of this bug: 1127736
(Assignee)

Comment 24

4 years ago
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!
Attachment #8550156 - Flags: feedback+ → feedback?(ejchen)
Comment on attachment 8550156 [details] [review]
Proposed patch

This looks okay to me ! f+ :)
Attachment #8550156 - Flags: feedback?(ejchen) → feedback+
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][COM=Privacy Panel]

Updated

4 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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.
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.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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.
: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!
Flags: needinfo?(ejchen)
okay, this patch was merged into Gaia/master : https://github.com/mozilla-b2g/gaia/commit/29eee46654b9f83bb6cc28f05ac459e26071c925
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: needinfo?(ejchen)
Resolution: --- → FIXED
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
Attachment #8550156 - Flags: approval-gaia-v2.2?
Target Milestone: --- → 2.2 S6 (20feb)
Attachment #8550156 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.