1484312 - Add UI for restricting third-party cookies to the Control Centre

Status: RESOLVED FIXED

(Firefox :: Site Identity, enhancement, P1)

Comment 1

[(no longer active)]

Attachment: Add UI for restricting third-party cookies to the Control Centre

Comment 2

[(no longer active)]

Mock-up: https://mozilla.invisionapp.com/share/QVIITATG4JT#/screens/301705296

Comment 3

[(no longer active)]

Attachment: Add UI for restricting third-party cookies to the Control Centre

Comment 4

[Francesco Lodolo [:flod]]

Comment on attachment 9002126 [details] [diff] [review]
Add UI for restricting third-party cookies to the Control Centre

Review of attachment 9002126 [details] [diff] [review]:
-----------------------------------------------------------------

l10n-wise it looks good

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 9002126 [details] [diff] [review]
Add UI for restricting third-party cookies to the Control Centre

Review of attachment 9002126 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good mostly, but I'd like to take another look after you fixed the issues below :)

::: browser/base/content/browser-contentblocking.js
@@ +127,5 @@
>  
> +var ThirdPartyCookies = {
> +  PREF_ENABLED: "network.cookie.cookieBehavior",
> +  PREF_ENABLED_VALUE: Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER,
> +  PREF_UI_ENABLED: "browser.contentblocking.control-center.ui.enabled",

That pref name seems a little broad, maybe "browser.contentblocking.rejecttrackers.control-center.ui.enabled"? Why keep this separate from "browser.contentblocking.rejecttrackers.ui.enabled"?

@@ +141,5 @@
> +                                          Ci.nsICookieService.BEHAVIOR_ACCEPT);
> +    XPCOMUtils.defineLazyPreferenceGetter(this, "uiPref", this.PREF_UI_ENABLED, false);
> +  },
> +  get enabled() {
> +    return this.uiPref && this.behaviorPref == this.PREF_ENABLED_VALUE;

I don't think that does what you expect it to. Instead, you should probably write code in your init() function that hides your categoryItem if the UI pref is set to false.

::: browser/themes/shared/controlcenter/3rdpartycookies-disabled.svg
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="context-fill" fill-opacity="context-fill-opacity">
> +  <path fill="context-fill" d="M15.379,8a.142.142,0,0,0,.02,0,7.978,7.978,0,0,1-1.858-.356.981.981,0,0,1-.054.847,1,1,0,1,1-1.735-.994.981.981,0,0,1,.481-.407c-.069-.036-.13-.083-.2-.121L6.613,12.387a.977.977,0,0,1-.4.476.85.85,0,0,1-.117.04L4.031,14.969c.509.219,1,.24,1.161-.147-.424,1.025,2.823,1.668,2.822.558,0,1.11,3.246.461,2.821-.564.425,1.025,3.175-.816,2.39-1.6.785.784,2.621-1.97,1.6-2.394C15.846,11.246,16.489,8,15.379,8Zm-5.073,5a1,1,0,1,1,1-1A1,1,0,0,1,10.306,13Z"/>

nit: this doesn't need fill="context-fill"

@@ +2,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16" fill="context-fill" fill-opacity="context-fill-opacity">
> +  <path fill="context-fill" d="M15.379,8a.142.142,0,0,0,.02,0,7.978,7.978,0,0,1-1.858-.356.981.981,0,0,1-.054.847,1,1,0,1,1-1.735-.994.981.981,0,0,1,.481-.407c-.069-.036-.13-.083-.2-.121L6.613,12.387a.977.977,0,0,1-.4.476.85.85,0,0,1-.117.04L4.031,14.969c.509.219,1,.24,1.161-.147-.424,1.025,2.823,1.668,2.822.558,0,1.11,3.246.461,2.821-.564.425,1.025,3.175-.816,2.39-1.6.785.784,2.621-1.97,1.6-2.394C15.846,11.246,16.489,8,15.379,8Zm-5.073,5a1,1,0,1,1,1-1A1,1,0,0,1,10.306,13Z"/>
> +  <path fill="context-fill" d="M14.707,1.293a1,1,0,0,0-1.414,0L9.679,4.907A7.942,7.942,0,0,1,8,.61C8,.619,8,.626,8,.635,8-.474,4.753.174,5.179,1.2,4.753.174,2,2.016,2.788,2.8,2,2.016.167,4.77,1.193,5.193.167,4.77-.476,8.016.634,8.015c-1.11,0-.461,3.247.564,2.821-.639.265-.163,1.428.475,2.077l-.38.38a1,1,0,1,0,1.414,1.414l12-12A1,1,0,0,0,14.707,1.293Zm-9,1.7a1,1,0,1,1-1,1A1,1,0,0,1,5.706,3ZM2.524,7.508a1,1,0,1,1,.37,1.364A1,1,0,0,1,2.524,7.508Zm4.769-.215Z"/>

nit: this doesn't need fill="context-fill"

Comment 6

[(no longer active)]

Attachment: Add UI for restricting third-party cookies to the Control Centre

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 9002886 [details] [diff] [review]
Add UI for restricting third-party cookies to the Control Centre

Review of attachment 9002886 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thank you!

I would really like us to have a test that shows that this can show the tracking UI independently of Tracking Protection, but considering that this is blocking other work and that FastBlock doesn't have these tests either (because the UI was developed before the feature) I think we can solve this in a follow-up. I filed bug 1485318.

::: browser/base/content/test/trackingUI/browser_trackingUI_open_preferences.js
@@ +55,5 @@
>      [CB_PREF, true],
>      [CB_UI_PREF, true],
>      [FB_PREF, false],
>      [TP_PREF, false],
> +    [TPC_PREF, Ci.nsICookieService.BEHAVIOR_ACCEPT],

This test will also need the "browser.contentblocking.rejecttrackers.control-center.ui.enabled" pref set to true.

::: browser/components/controlcenter/content/panel.inc.xul
@@ +93,5 @@
>                <label flex="1" class="identity-popup-content-blocking-category-state-label">&contentBlocking.trackingProtection.blocked.label;</label>
>                <label flex="1" class="identity-popup-content-blocking-category-add-blocking text-link"
>                       onclick="ContentBlocking.openPreferences('identityPopup-CB-tracking-protection');">&contentBlocking.trackingProtection.add.label;</label>
>              </hbox>
> +            <hbox id="identity-popup-content-blocking-category-3rdpartycookies"

Note that in the UX mockups I've seen (e.g. https://mozilla.invisionapp.com/share/QVIITATG4JT#/screens/309042357) this item was in the middle, between fastblock and trackers.

Might want to double-check with Bryan.

Comment 8

[(no longer active)]

I couldn't get a hold of Bryan.  :-(  I'm gonna land the patch as is.  If we didn't like the order, we could address it in a follow-up.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee539247d48b
Add UI for restricting third-party cookies to the Control Centre; r=johannh,flod

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ee539247d48b