1073646 - [Settings] "Clear bookmarks data" does not work properly

Status: VERIFIED FIXED

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

Description

[Yeojin Chung [:YeojinC]]

Attachment: logcat_20140926_ClearBookmarksData.txt

Description:
The private data is not cleared when the user selects "Clear bookmarks data". The email is still logged in after the user cleared the bookmarks data.

Repro Steps:
1) Update a Flame device to BuildID: 20140926000202
2) Search an email service provider (e.g. Gmail, Outlook, etc.) on the rocket bar.
3) When the search result shows up, long press, and select "Add to Home Screen", and Select "Done".
4) Go to Home screen, and open the added website.
5) Log into an email account.
6) Close the browser window.
7) Go to Settings > Browsing Privacy.
8) Select "Clear bookmarks data" and "Clear".
9) Re-open the email page from the Home screen.

Actual:
Email account is still logged in. (Still logged in after tapping Refresh button.)

Expected: 
Email account is logged out as the user cleared the private data.

Environmental Variables:
Device: Flame 2.1
BuildID: 20140926000202
Gaia: 6a6ed9433fce47e76c07fd35bc5952acb108f4c8
Gecko: 7b09a378588c
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
 
Repro frequency: 100%
See attached: logcat, video
http://youtu.be/GFzhjCIN9jE

Comment 1

[Yeojin Chung [:YeojinC]]

This issue also reproduces on Flame 2.2:

Flame 2.2 KitKat Base (319mb)

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20140926040203
Gaia: a06714c555ca7068545f10b4437a16c14cd8e7f5
Gecko: 9e3d649b80a2
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

The email account is still logged in after the user cleared the bookmark data.

======================================
This issue does NOT reproduces on Flame 2.0:

Flame 2.0 KitKat Base (319mb)

Device: Flame 2.0
BuildID: 20140926063008
Gaia: c1aa7829548e65360472c31544dbe2839eaf5be1
Gecko: 5a9f1f402425
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

"Clear bookmarks data" is under App permissions on v.2.0.
After clearing bookmarks data, the email account added to home screen is logged out when re-opening the page.

Comment 2

[Yeojin Chung [:YeojinC]]

https://bugzilla.mozilla.org/show_bug.cgi?id=938177#c17

According to Comment 17 and Comment 19 on Bug 938177, "Clear bookmarks data" can be merged with "Clear cookies and stored data", since bookmark has taken out of the new browser.

Comment 3

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

Francis, can you take a look at this? Is this the plan to still merge the two buttons?

Comment 4

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

[Blocking Requested - why for this release]:

The button currently does not function as far as we can tell so nominating this 2.1?

Comment 5

[Kevin Grandon :kgrandon]

Cristian - this might be bookmarks related. Could you help us debug this?

Comment 6

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request

Kevin,

    The bug was in home screen because it was not consuming the "cleared" event. The datastore was cleared properly and there was not any issue in settings app (for this reason I moved the component in the bug from settings to homescreen).

Arthur,

    I added a new panel BrowsingPrivacyPanel in marionette tests in settings app just in case you want to take a look at this change

Thanks guys

Comment 7

[Gregor Wagner [:gwagner]]

Regression

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8498718 [details]
Github pull request

Looks good. There is just one problem with leaving two dividers in the code, it will create some weird interaction with dragdrop if we don't clean that up. Left a comment on github. Thanks for the quick patch!

Comment 9

[Francis Djabri [:djabber]]

(In reply to KTucker [:KTucker] from comment #3)
> Francis, can you take a look at this? Is this the plan to still merge the
> two buttons?

Yes, that's still the desired outcome, so this bug is valid. Thanks, Francis

Comment 10

[Kevin Grandon :kgrandon]

This doesn't sound right to me. I don't there's anything to merge this button with.

The current behavior of this button is to delete all bookmarks. TBH - I'm not sure it's a button we actually want in the product? The wording is also confusing - is this a major bug that we have not identified until now?

It seems like we should remove this button.

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8498718 [details]
Github pull request

Clearing review requests for now. After speaking with UX - we should remove this button. It's harmful to have a button here which will remove all of the bookmarks on the device.

Cristian - Can you submit another patch to simply remove the button and logic surrounding it? Thanks!

Comment 12

[Francis Djabri [:djabber]]

(In reply to Francis Djabri [:djabber] from comment #9)
> (In reply to KTucker [:KTucker] from comment #3)
> > Francis, can you take a look at this? Is this the plan to still merge the
> > two buttons?
> 
> Yes, that's still the desired outcome, so this bug is valid. Thanks, Francis

Just to clarify further:

The intent behind comment 17 in Bug 938177 was that the old "Clear bookmarks data" that used to be under the App permissions settings should be merged with the "Clear cookies and stored data" button. Tapping this button should clear private data from sites, whether they are bookmarks or not. 

It seems what has happened however is that the "Clear bookmarks data" button has remained and it in fact deletes the users' bookmarks, not just the private data from them. 

So the "Clear bookmarks data" should be removed altogether as it was never requested. 

Jenny, could you please remove this button from the Settings spec?

Thanks

Comment 13

[Jenny Lee]

Attachment: [Settings] Browsing v1.2.pdf

Hi Francis, 
Oops! Looks like I misunderstood what you meant before. See attached for updated spec (also added a flow to show a toast after the clearing is complete). Thanks!

Comment 14

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8498718 [details]
Github pull request

Removed button and logic surrounding it

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8498718 [details]
Github pull request

Thank you for handling this, and sorry for the confusion earlier. Just a note about uplift - I think we'll probably want to prepare a patch without the string deletions for 2.1, so we don't break string freeze here.

Comment 16

[Cristian Rodriguez (:crdlc)]

With Artuhur's review I gonna implement a proper pr like this one without string deletion

(In reply to Kevin Grandon :kgrandon from comment #15)
> Comment on attachment 8498718 [details]
> Github pull request
> 
> Thank you for handling this, and sorry for the confusion earlier. Just a
> note about uplift - I think we'll probably want to prepare a patch without
> the string deletions for 2.1, so we don't break string freeze here.

Comment 17

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

Comment on attachment 8498718 [details]
Github pull request

LGTM, thanks for the patch!

Comment 18

[Cristian Rodriguez (:crdlc)]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/71f19789154e6d1e0e094c3970fb5f71fb7a8e3c

Comment 19

[Cristian Rodriguez (:crdlc)]

Attachment: Github pull request for v2.1 (same patch without locales removed)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Cristian Rodriguez (:crdlc) from comment #19)
> Created attachment 8499456 [details]
> Github pull request for v2.1 (same patch without locales removed)

Please request Gaia v2.1 approval on this patch.

Comment 21

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8498718 [details]
Github pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: button to delete bookmarks which does not work
[Testing completed]: unit test/manual
[Risk to taking this patch] (and alternatives if risky): close to null
[String changes made]: No

NOTE: there is an specific pr for 2.1 which does not touch any locale

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[John Thomas [:Johnt]]

This bug does NOT occur on the Flame 2.1 (319mb) and the Flame 2.2 (319mb)

Flame 2.2 Master KK (319mb) (Full Flash)

Device: Flame 2.2
BuildID: 20141010040202
Gaia: 1036b544b7e102592bd9fab95cd9317329ac1293
Gecko: 50b689feab5f
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Flame 2.1 KK (319mb) (Full Flash)

Environmental Variables:
Device: Flame 2.1
BuildID: 20141010000201
Gaia: bc8eb493311c58f1f311a56b8b645b52bfbd2f71
Gecko: 72c13d8631ff
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Result: Clear bookmarks data button has been removed.