1484769 - Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P1)

Comment 1

[(no longer active)]

Attachment: Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options

Comment 2

[(no longer active)]

Comment on attachment 9002536 [details] [diff] [review]
Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options

The design has changed significantly here per Tanvi's proposals for handling the edge cases in the UX spec.  This patch will need serious reworking.

Comment 3

[(no longer active)]

Attachment: Part 1: Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options

I think this part implements the major parts of the desired UX for the preferences UI.  What's left to do is to write some tests for the edge cases of the interaction of this UI and the Cookies and Site Data UI, and also file follow-up bugs for the edge cases that this patch doesn't handle as per https://docs.google.com/document/d/1-XJxLCQae66CMWYTHStuqWa2TlxvOtChYFlhbFonQ4k.

flod, there are a couple of new strings in this patch, would you mind having another look please?  Hopefully should be a quick review round.

johannh, I'm going to submit the tests in other patches tomorrow hopefully, but I'd like to land this part sooner than later, since I think it's good to go.  If you see things that can be addressed in follow-ups and don't need to block the initial landing, that'd be my preference, since this I'll be working on this UI for a while.  :-)

The new parts in this patch are the warnings, and the interaction of the Cookies and Site Data section's "All cookies" and "Cookies from unvisited websites" options with this UI that activate the warnings and disable the UI.  I've made sure the disabling/enabling behaves sanely with the global toggle for turning Content Blocking as a whole on and off, in the following way: Turning Content Blocking off deactivates everything forcefully, and turning it on only activates the UI if it is not being deactivated by one of those two choices from the Cookies and Site Data section.

Thanks for sticking with me changing things mid-air here!

Comment 4

[Francesco Lodolo [:flod]]

Comment on attachment 9003655 [details] [diff] [review]
Part 1: Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options

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

Strings and comment look good, only need to clarify the space vs hyphen.

::: browser/locales/en-US/browser/preferences/preferences.ftl
@@ +839,5 @@
>  content-blocking-tracking-protection-option-disabled =
>    .label = Never block
>  content-blocking-tracking-protection-change-blocklist = Change Block List…
>  
> +content-blocking-reject-trackers-label = Third Party Cookies

Somehow missed this detail in the previous pass: looks like we're mixing "third party" and "third-party". 

Can we confirm with UX/Copy which one is correct? We currently have 2 "third party" in Firefox, and 4 "third-party". In case, fixing consistency wouldn't require new IDs.

Comment 6

[(no longer active)]

Oops, Third-Party is correct.  My bad!

Comment 7

[(no longer active)]

Attachment: Part 2: Add a test to ensure that the dependent controls in the Content Blocking Third-Party Cookies section correctly get disabled based on the cookieBehavior pref values

Comment 8

[(no longer active)]

Attachment: Part 3: Add a test to ensure that the warnings in the Third-Party Cookies subsection of Content Blocking appear correctly based on the values of the cookieBehavior pref

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 9003655 [details] [diff] [review]
Part 1: Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options

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

I don't understand some of the interactions in this patch. Shouldn't the "Third Party Cookies" group in "Content Blocking" have a "Never Block" option that maps to Cookies and Site Data > Accept Cookies and Site Data?

Sorry and feel free to re-request review if I'm missing something.

Thanks!

::: browser/components/preferences/in-content/privacy.js
@@ +473,5 @@
> +    // Disable the Content Blocking Third-Party Cookies UI based on a pref
> +    if (!contentBlockingRejectTrackersUiEnabled) {
> +      let elements = document.querySelectorAll(".reject-trackers-ui");
> +      for (let element of elements) {
> +        element.hidden = "true";

nit: please set element.hidden = true (boolean, not a string)

@@ +1113,5 @@
> +   * Converts between network.cookie.cookieBehavior and the new content blocking UI
> +   */
> +  readBlockCookiesCB() {
> +    let pref = Preferences.get("network.cookie.cookieBehavior");
> +    switch (pref.value) {

This is not handling Ci.nsICookieService.BEHAVIOR_ACCEPT, which breaks the UX for this case. As mentioned above, it should probably map that value to a "Never Block" option in the CB UI.

::: browser/themes/shared/incontentprefs/privacy.css
@@ +153,5 @@
> +  padding: 4px;
> +}
> +
> +#contentBlockingChangeCookieSettings {
> +  padding: 0.25em 0.75em;

Can you please give this "margin: 4px 8px;" to match the menulists above it?

::: toolkit/themes/shared/in-content/common.inc.css
@@ +232,5 @@
>  html|*.numberbox-input:disabled::-moz-number-spin-box,
>  xul|button[disabled="true"],
>  xul|colorpicker[type="button"][disabled="true"],
>  xul|menulist[disabled="true"] {
> +  color: GrayText;

What element are you trying to update here? Why isn't opacity enough?

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 9003847 [details] [diff] [review]
Part 3: Add a test to ensure that the warnings in the Third-Party Cookies subsection of Content Blocking appear correctly based on the values of the cookieBehavior pref

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

The test looks fine, I don't think I need to review this again, but please cleanup the pref properly :)

::: browser/components/preferences/in-content/tests/browser_contentblocking.js
@@ +310,5 @@
> +    [Ci.nsICookieService.BEHAVIOR_LIMIT_FOREIGN, 2],
> +    [Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER, 0],
> +  ]);
> +
> +  Services.prefs.setBoolPref(CB_PREF, true);

Wait, where are you cleaning this up? Can you please just add this to the pushPrefEnv above?

@@ +319,5 @@
> +  let deck = doc.getElementById("blockCookiesCBDeck");
> +
> +  for (let obj of expectedDeckIndex) {
> +    await SpecialPowers.pushPrefEnv({set: [
> +      [NCB_PREF, obj[0]],

nit: I would suggest using Services.prefs.setIntPref here instead, since you clear the pref below with clearUserPref.

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 9003843 [details] [diff] [review]
Part 2: Add a test to ensure that the dependent controls in the Content Blocking Third-Party Cookies section correctly get disabled based on the cookieBehavior pref values

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

Thanks for covering this with tests!

::: browser/components/preferences/in-content/tests/browser_contentblocking.js
@@ +205,5 @@
> +  is(Services.prefs.getBoolPref(CB_PREF), false, "Content Blocking is off");
> +  checkControlState(doc, dependentControls);
> +  checkControlStateWorker(doc, alwaysDisabledControls, false);
> +
> +  Services.prefs.clearUserPref("browser.contentblocking.enabled");

nit: Please use CB_PREF here

(I know it's originally my code, sorry for that ;)

Comment 12

[(no longer active)]

(In reply to Johann Hofmann [:johannh] from comment #9)
> Comment on attachment 9003655 [details] [diff] [review]
> Part 1: Update the Content Blocking section of the Preferences UI to add
> Third-Party Cookies options
> 
> Review of attachment 9003655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand some of the interactions in this patch. Shouldn't the
> "Third Party Cookies" group in "Content Blocking" have a "Never Block"
> option that maps to Cookies and Site Data > Accept Cookies and Site Data?

Not according to <https://docs.google.com/document/d/1-XJxLCQae66CMWYTHStuqWa2TlxvOtChYFlhbFonQ4k>.  That document is what this patch has been implementing.  In fact, you are bringing up a good point.  Right now, that document leaves the case of what should happen when the user selects "Accept cookies and site data" in the Cookies and Site Data section unmentioned, which is why I missed it last week.  Not sure what I should be doing about it right now.  :-/

> Sorry and feel free to re-request review if I'm missing something.

Any chance you could review this patch as is while I try to find Tanvi today to ask her how she wants me to handle this case?  I will not land the patches while this issue is pending, and will submit the fixes for this issue as an interdiff on top of part 1 for easier reviewing...

> @@ +1113,5 @@
> > +   * Converts between network.cookie.cookieBehavior and the new content blocking UI
> > +   */
> > +  readBlockCookiesCB() {
> > +    let pref = Preferences.get("network.cookie.cookieBehavior");
> > +    switch (pref.value) {
> 
> This is not handling Ci.nsICookieService.BEHAVIOR_ACCEPT, which breaks the
> UX for this case. As mentioned above, it should probably map that value to a
> "Never Block" option in the CB UI.

Well, yes, see above!

> ::: toolkit/themes/shared/in-content/common.inc.css
> @@ +232,5 @@
> >  html|*.numberbox-input:disabled::-moz-number-spin-box,
> >  xul|button[disabled="true"],
> >  xul|colorpicker[type="button"][disabled="true"],
> >  xul|menulist[disabled="true"] {
> > +  color: GrayText;
> 
> What element are you trying to update here? Why isn't opacity enough?

Good question!  I _think_ I added this for the Change Cookie Settings button, but now that I remove this property the button's color looks correct when it gets disabled, so now I'm not sure why it was needed before...

Comment 13

[(no longer active)]

Attachment: Part 1: Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options

Comment 14

[Tanvi Vyas[:tanvi]]

Clearing my needinfo.  With a checkbox, you don't need a "Never" option.  (If we have a dropdown, then you would need a "Never" option.)

Comment 15

[(no longer active)]

(In reply to Tanvi Vyas[:tanvi] from comment #14)
> Clearing my needinfo.  With a checkbox, you don't need a "Never" option. 
> (If we have a dropdown, then you would need a "Never" option.)

The patch to add checkboxes is up for review!

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 9004212 [details] [diff] [review]
Part 1: Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options

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

r=me if we fix the mentioned edge case in bug 1486462.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c39303a566
Part 1: Update the Content Blocking section of the Preferences UI to add Third-Party Cookies options; r=johannh,flod
https://hg.mozilla.org/integration/mozilla-inbound/rev/9672e76cb424
Part 2: Add a test to ensure that the dependent controls in the Content Blocking Third-Party Cookies section correctly get disabled based on the cookieBehavior pref values; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e0487c3651
Part 3: Add a test to ensure that the warnings in the Third-Party Cookies subsection of Content Blocking appear correctly based on the values of the cookieBehavior pref; r=johannh

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d7c39303a566
https://hg.mozilla.org/mozilla-central/rev/9672e76cb424
https://hg.mozilla.org/mozilla-central/rev/e3e0487c3651