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

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

9 months ago
No description provided.
Assignee

Comment 1

9 months ago
Attachment #9002536 - Flags: review?(jhofmann)
Attachment #9002536 - Flags: review?(francesco.lodolo)
Assignee

Updated

9 months ago
Assignee: nobody → ehsan
Blocks: 1475097
Attachment #9002536 - Flags: review?(francesco.lodolo) → review+
Assignee

Comment 2

9 months ago
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.
Attachment #9002536 - Attachment is obsolete: true
Attachment #9002536 - Flags: review?(jhofmann)
Priority: -- → P1
Assignee

Comment 3

9 months ago
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!
Attachment #9003655 - Flags: review?(jhofmann)
Attachment #9003655 - Flags: review?(francesco.lodolo)
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.
Attachment #9003655 - Flags: review?(francesco.lodolo) → review+
Assignee

Updated

9 months ago
Duplicate of this bug: 1485904
Assignee

Comment 6

9 months ago
Oops, Third-Party is correct.  My bad!
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?
Attachment #9003655 - Flags: review?(jhofmann) → feedback+
Flags: needinfo?(tanvi)
Flags: needinfo?(ehsan)
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.
Attachment #9003847 - Flags: review?(jhofmann) → review+
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 ;)
Attachment #9003843 - Flags: review?(jhofmann) → review+
Assignee

Comment 12

9 months ago
(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...
Flags: needinfo?(ehsan)
Assignee

Updated

9 months ago
Attachment #9003655 - Attachment is obsolete: true
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.)
Flags: needinfo?(tanvi)
Assignee

Updated

9 months ago
Depends on: 1486462
Assignee

Comment 15

9 months ago
(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!
Assignee

Updated

9 months ago
Duplicate of this bug: 1485030
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.
Attachment #9004212 - Flags: review?(jhofmann) → review+

Comment 18

9 months ago
Pushed by eakhgari@mozilla.com:
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

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d7c39303a566
https://hg.mozilla.org/mozilla-central/rev/9672e76cb424
https://hg.mozilla.org/mozilla-central/rev/e3e0487c3651
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Assignee

Updated

8 months ago
Depends on: 1491449
You need to log in before you can comment on or make changes to this bug.