Closed Bug 1483378 Opened Last year Closed Last year

Update Cookie and Site Data UI in preferences

Categories

(Firefox :: Preferences, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(6 files, 3 obsolete files)

I have yet to write tests for this, but I will submit patches for the tests in this bug too and will land it all together.

We want to hide this UI behind a separate pref that allows us to decide when to ship this UI independently from the larger set of content blocking UI being developed right now.  We would also like to have two strings for the first item in the new drop-down, one with the "(recommended)" tag and one without, and a preference will control which string will be used.  We will use a shield study during the 63 beta cycle to determine whether we consider the "Third-party trackers" settings as a recommended option or not.

Sorry about the pref-amageddon!
Attachment #9000089 - Flags: review?(jaws)
Attachment #9000089 - Flags: review?(francesco.lodolo)
These tests extend the existing tests for the old UI, and ensure the
new states in the new UI work properly.
Attachment #9000110 - Flags: review?(jaws)
Comment on attachment 9000089 [details] [diff] [review]
Part 1: Update the Cookies and Site Data UI in the Preferences window

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

Strings look good, but I have serious concerns about the layout of the "Type blocked" section.

::: browser/locales/en-US/browser/preferences/preferences.ftl
@@ +764,5 @@
> +
> +sitedata-disallow-cookies-option =
> +    .label = Block cookies and site data
> +
> +sitedata-block-desc = Type blocked

I would add a comment to give a bit more context, e.g. This label means 'type of content that is blocked', and it's followed by a dropdown list with content types.

@@ +775,5 @@
> +    .label = Cookies from unvisited websites
> +sitedata-block-all-third-parties-option =
> +    .label = All third-party cookies
> +sitedata-block-always-option =
> +    .label = All cookies (may cause websites to break)

I'm concerned by this one, in terms of layout. For example, in Italian it would become:

Tutti i cookie (potrebbe causare malfunzionamenti in alcuni siti)

And I'm pretty sure it would either break the layout, or make it awful. Does it need to be a dropdown? The only way I see this work, is by using radio buttons, and let the text wrap if needed.

You can get an idea of the translation of the parenthetical by looking at this
https://transvision.mozfr.org/string/?entity=browser/browser/preferences/preferences.ftl:sitedata-block-cookies-option.label&repo=gecko_strings
Attachment #9000089 - Flags: review?(francesco.lodolo) → review+
(In reply to Francesco Lodolo [:flod] from comment #3)
> @@ +775,5 @@
> > +    .label = Cookies from unvisited websites
> > +sitedata-block-all-third-parties-option =
> > +    .label = All third-party cookies
> > +sitedata-block-always-option =
> > +    .label = All cookies (may cause websites to break)
> 
> I'm concerned by this one, in terms of layout. For example, in Italian it
> would become:
> 
> Tutti i cookie (potrebbe causare malfunzionamenti in alcuni siti)
> 
> And I'm pretty sure it would either break the layout, or make it awful. Does
> it need to be a dropdown? The only way I see this work, is by using radio
> buttons, and let the text wrap if needed.

We're actually planning to make this radio buttons instead of a dropdown soon. Not sure if that helps with the issue at hand.
Attachment #9000110 - Flags: review?(jaws) → review?(jhofmann)
Attachment #9000089 - Flags: review?(jaws) → review?(jhofmann)
Thanks for the review, Francesco!

(In reply to Johann Hofmann [:johannh] from comment #4)
> (In reply to Francesco Lodolo [:flod] from comment #3)
> > @@ +775,5 @@
> > > +    .label = Cookies from unvisited websites
> > > +sitedata-block-all-third-parties-option =
> > > +    .label = All third-party cookies
> > > +sitedata-block-always-option =
> > > +    .label = All cookies (may cause websites to break)
> > 
> > I'm concerned by this one, in terms of layout. For example, in Italian it
> > would become:
> > 
> > Tutti i cookie (potrebbe causare malfunzionamenti in alcuni siti)
> > 
> > And I'm pretty sure it would either break the layout, or make it awful. Does
> > it need to be a dropdown? The only way I see this work, is by using radio
> > buttons, and let the text wrap if needed.
> 
> We're actually planning to make this radio buttons instead of a dropdown
> soon. Not sure if that helps with the issue at hand.

Does this address your concern?
Flags: needinfo?(francesco.lodolo)
(In reply to :Ehsan Akhgari from comment #5)
> > > And I'm pretty sure it would either break the layout, or make it awful. Does
> > > it need to be a dropdown? The only way I see this work, is by using radio
> > > buttons, and let the text wrap if needed.
> > 
> > We're actually planning to make this radio buttons instead of a dropdown
> > soon. Not sure if that helps with the issue at hand.
> 
> Does this address your concern?

Yes. As said, radio button can wrap, so we should be good with that approach.
Flags: needinfo?(francesco.lodolo)
Ah, but I was wrong, looking at the patch now I realize this is implementing something other than I expected. Ok, in this case we are NOT planning to change the dropdown to radio buttons at the moment.

Bryan, can you please take a look at comment 3? Do you think there are any alternatives to using a dropdown here, e.g radio buttons? The problem with using radio buttons here is that it adds quite a lot of visual noise to the UI.
Flags: needinfo?(bbell)
This looks pretty imminent - setting to P1.
Priority: -- → P1
Tanvi requested this menu item to also be possible to turn on/off based on a separate preference.
Attachment #9001705 - Flags: review?(jhofmann)
Comment on attachment 9000089 [details] [diff] [review]
Part 1: Update the Cookies and Site Data UI in the Preferences window

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

This looks good, thank you, still needs a little polish, but I don't think I need to approve it again.

::: browser/components/preferences/in-content/privacy.js
@@ +517,5 @@
> +    }
> +    if (contentBlockingCookiesAndSiteDataUiEnabled) {
> +      // The Keep Until row doesn't change in the new UI, so we just transplant it
> +      // from the old UI by moving it in the DOM!
> +      let keepUntil = document.getElementById("keepRow");

This needs a little top margin, looking at the UX spec maybe 1em? I think in this case it's fine to just add some inline style here, it can't get much hackier anyway :)

@@ +520,5 @@
> +      // from the old UI by moving it in the DOM!
> +      let keepUntil = document.getElementById("keepRow");
> +      keepUntil.parentNode.removeChild(keepUntil);
> +      let blockThirdPartyRow = document.getElementById("blockThirdPartyRow");
> +      blockThirdPartyRow.parentNode.appendChild(keepUntil);

nit: Why get the blockThirdPartyRow.parentNode and not just document.getElementById("blockCookies")?

@@ +521,5 @@
> +      let keepUntil = document.getElementById("keepRow");
> +      keepUntil.parentNode.removeChild(keepUntil);
> +      let blockThirdPartyRow = document.getElementById("blockThirdPartyRow");
> +      blockThirdPartyRow.parentNode.appendChild(keepUntil);
> +      keepUntil.removeAttribute("class"); // drop the indentation

Please use keepUntil.classList.remove("indent") instead.

@@ +526,5 @@
> +
> +      // Allow turning off the "(recommended)" label using a pref
> +      let blockCookiesFromTrackers = document.getElementById("blockCookiesFromTrackers");
> +      if (contentBlockingCookiesAndSiteDataRejectTrackersRecommended) {
> +        blockCookiesFromTrackers.setAttribute("data-l10n-id", "sitedata-block-trackers-option-recommended");

nit: use document.l10n.setAttributes(blockCookiesFromTrackers, "sitedata-block-trackers-option-recommended")

@@ +617,5 @@
> +    let keepUntilMenu = document.getElementById("keepCookiesUntil");
> +
> +    let blockCookies = (behavior != 0);
> +    let cookieBehaviorLocked = Services.prefs.prefIsLocked("network.cookie.cookieBehavior");
> +    const blockCookiesControlsDisabled = !blockCookies || cookieBehaviorLocked;

nit: we try to keep let and const usage consistent and generally prefer const for GLOBAL_CONSTANTS, so use "let" here

@@ +623,5 @@
> +
> +    let completelyBlockCookies = (behavior == 2);
> +    let privateBrowsing = Preferences.get("browser.privatebrowsing.autostart").value;
> +    let cookieExpirationLocked = Services.prefs.prefIsLocked("network.cookie.lifetimePolicy");
> +    const keepUntilControlsDisabled = privateBrowsing || completelyBlockCookies || cookieExpirationLocked;

nit: let

@@ +1088,5 @@
> +  /**
> +   * Reads the network.cookie.cookieBehavior preference value and
> +   * enables/disables the rest of the new cookie & site data UI accordingly.
> +   *
> +   * Returns "0" if cookies are accepted and "2" if they are entirely disabled.

Doesn't it return "disallow" and "allow"?

@@ +1105,5 @@
> +   * Updates the "accept third party cookies" menu based on whether the
> +   * "accept cookies" or "block cookies" radio buttons are selected.
> +   */
> +  writeBlockCookies() {
> +    var block = document.getElementById("blockCookies");

nit: let

@@ +1106,5 @@
> +   * "accept cookies" or "block cookies" radio buttons are selected.
> +   */
> +  writeBlockCookies() {
> +    var block = document.getElementById("blockCookies");
> +    var blockCookiesMenu = document.getElementById("blockCookiesMenu");

nit: let

@@ +1121,5 @@
> +  /**
> +   * Converts between network.cookie.cookieBehavior and the new third-party cookies UI
> +   */
> +  readBlockCookiesFrom() {
> +    var pref = Preferences.get("network.cookie.cookieBehavior");

nit: let

@@ +1137,5 @@
> +    }
> +  },
> +
> +  writeBlockCookiesFrom() {
> +    var block = document.getElementById("blockCookiesMenu").selectedItem;

nit: let

::: browser/components/preferences/in-content/privacy.xul
@@ +220,5 @@
>          <radio data-l10n-id="sitedata-block-cookies-option"
>                 value="2"
>                 flex="1" />
>        </radiogroup>
> +      <radiogroup id="blockCookies"

This element feels a little visually squeezed compared to the mockup, can you please add it to the selector here:

https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/browser/themes/shared/incontentprefs/preferences.inc.css#414

That will give it the same amount of top margin as the other section.
Attachment #9000089 - Flags: review?(jhofmann) → review+
Comment on attachment 9000110 [details] [diff] [review]
Part 2: Add tests for the new Cookies and Site Data UI

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

History mode should be completely independent from cookie settings now so, browser_privacypane_4.js and browser_privacypane_8.js are mostly obsolete.

There are some pieces that could still be relevant (maybe move these lines to _3.js: https://searchfox.org/mozilla-central/rev/5dbfd833bbb114afe758db4d4bdbc5b13bcc33ef/browser/components/preferences/in-content/tests/browser_privacypane_4.js#21-22) but otherwise I'd recommend just removing these tests instead.

I missed removing them when I changed this.

::: browser/components/preferences/in-content/tests/browser_privacypane_8.js
@@ +15,5 @@
>    // history mode should be initialized to remember
>    test_historymode_retention("remember", undefined),
>  
>    // history mode should remain remember; toggle acceptCookies checkbox
>    test_custom_retention("acceptCookies", "remember"),

This is completely broken now, acceptCookies is not a checkbox (hence that function should get a value parameter) and it shouldn't change the history mode (and it doesn't in my local testing).

In case anyone is interested, this isn't failing because test_custom_retention itself is setting the historyMode to "custom" here: https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js#259
Attachment #9000110 - Flags: review?(jhofmann)
> Bryan, can you please take a look at comment 3? Do you think there are any
> alternatives to using a dropdown here, e.g radio buttons? The problem with
> using radio buttons here is that it adds quite a lot of visual noise to the
> UI.

I just spoke with Ehsan, and it seems the current design will have enough room to accommodate "longer" languages. Also, if we run into issues, we have a fallback position which is to insert a carriage return after the label, moving the pull-down menu down a bit and freeing more horizontal space.
Would you like me to remove browser_privacypane_8.js too, or do something else with it?
Flags: needinfo?(bbell) → needinfo?(jhofmann)
(In reply to :Ehsan Akhgari from comment #13)
> Would you like me to remove browser_privacypane_8.js too, or do something
> else with it?

Unless you have strong objections I'd say remove it. At this point it only checks that a menulist can be changed to different values, which seems unnecessary to me.
Flags: needinfo?(jhofmann)
Comment on attachment 9001705 [details] [diff] [review]
Part 3: Enable hiding the "reject trackers" menu item based on a preference

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

Just hiding it doesn't work in this case because writeBlockCookies from your previous patch selects third party trackers by default (even if they are hidden). Could you just remove the node? Is there any harm in that?
Attachment #9001705 - Flags: review?(jhofmann) → review-
(In reply to Johann Hofmann [:johannh] from comment #15)
> Comment on attachment 9001705 [details] [diff] [review]
> Part 3: Enable hiding the "reject trackers" menu item based on a preference
> 
> Review of attachment 9001705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just hiding it doesn't work in this case because writeBlockCookies from your
> previous patch selects third party trackers by default (even if they are
> hidden). Could you just remove the node? Is there any harm in that?

Great catch!  No, there is no harm in removing it, I'll do that.
Attachment #9000089 - Attachment is obsolete: true
Attachment #9000110 - Attachment is obsolete: true
Attachment #9001705 - Attachment is obsolete: true
Attachment #9001994 - Flags: review?(jhofmann)
These tests extend the existing tests for the old UI, and ensure the
new states in the new UI work properly.
Attachment #9001995 - Flags: review?(jhofmann)
BTW, I'll be writing some other patches on top of these to remove the old UI, after discussing with Tanvi.  In order to avoid redoing work, I'd like to land these ones as they are...
Attachment #9002080 - Flags: review?(jhofmann)
Attachment #9002080 - Flags: review?(francesco.lodolo)
Attachment #9002080 - Flags: review?(francesco.lodolo) → review+
Attachment #9001994 - Flags: review?(jhofmann) → review+
Attachment #9001995 - Flags: review?(jhofmann) → review+
Comment on attachment 9001996 [details] [diff] [review]
Part 4: Enable hiding the "reject trackers" menu item based on a preference

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

r=me with the below resolved :)

::: browser/components/preferences/in-content/privacy.js
@@ +537,5 @@
> +      if (!contentBlockingCookiesAndSiteDataRejectTrackersEnabled) {
> +        blockCookiesFromTrackers.parentNode.removeChild(blockCookiesFromTrackers);
> +        // We may have removed the default element of the menulist just now, so recompute it again!
> +        let blockCookiesMenu = document.getElementById("blockCookiesMenu");
> +        blockCookiesMenu.setInitialSelection();

Do you think that this is necessary?

I'm a bit concerned that we're able to put the user in a state where they have network.cookie.cookieBehavior set to "4", but "Third Party Trackers" doesn't appear in their list anymore.

If we don't recompute the selection, the correct label will still show in the list, but the user won't be able to select it anymore, which still sounds confusing but preferable to me.

What do you think?
Attachment #9001996 - Flags: review?(jhofmann) → review+
Comment on attachment 9001996 [details] [diff] [review]
Part 4: Enable hiding the "reject trackers" menu item based on a preference

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

::: browser/components/preferences/in-content/privacy.js
@@ +534,5 @@
>          document.l10n.setAttributes(blockCookiesFromTrackers, "sitedata-block-trackers-option-recommended");
>        }
> +      // Allow hiding the Reject Trackers option based on a pref
> +      if (!contentBlockingCookiesAndSiteDataRejectTrackersEnabled) {
> +        blockCookiesFromTrackers.parentNode.removeChild(blockCookiesFromTrackers);

You should be able to just do "blockCookiesFromTrackers.remove();"
Comment on attachment 9002080 [details] [diff] [review]
Part 5: Remove the old Cookies & Site Data UI

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

r=me, note the comments. :)

Thanks!

::: browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js
@@ +53,5 @@
>    controls.forEach(function(control) {
>      ok(control, "the dependent controls should exist");
>    });
>    let independents = [
> +    win.document.getElementById("blockCookies")

Hrm, I realize that I didn't clean up these tests enough after making the cookies section independent of the history menu. I think we can leave it like this, but this whole test seems a bit nonsensical for these elements now.

Not an issue with your patch, though :)

::: browser/locales/en-US/browser/preferences/preferences.ftl
@@ -734,5 @@
>  sitedata-learn-more = Learn more
>  
> -sitedata-accept-cookies-option =
> -    .label = Accept cookies and site data from websites (recommended)
> -    .accesskey = A

I must have missed this earlier, but surely the new labels need to get those accesskeys as well.

@@ -737,5 @@
> -    .label = Accept cookies and site data from websites (recommended)
> -    .accesskey = A
> -
> -sitedata-block-cookies-option =
> -    .label = Block cookies and site data (may cause websites to break)

I understand that this is not a question for you, but we should make sure that we really want to remove (may cause websites to break) and (recommend) on the new UI.

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +410,5 @@
>    flex-direction: column;
>    justify-content: space-between;
>  }
>  
>  #acceptCookies, #blockCookies {

Please remove #acceptCookies here

(Also as a side note, we usually separate selectors in CSS rules by newlines)

@@ +414,5 @@
>  #acceptCookies, #blockCookies {
>    margin-top: 1.5em;
>  }
>  
> +#keepRow {

You can merge this one with the rule above.
Attachment #9002080 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #23)
> ::: browser/components/preferences/in-content/privacy.js
> @@ +537,5 @@
> > +      if (!contentBlockingCookiesAndSiteDataRejectTrackersEnabled) {
> > +        blockCookiesFromTrackers.parentNode.removeChild(blockCookiesFromTrackers);
> > +        // We may have removed the default element of the menulist just now, so recompute it again!
> > +        let blockCookiesMenu = document.getElementById("blockCookiesMenu");
> > +        blockCookiesMenu.setInitialSelection();
> 
> Do you think that this is necessary?
> 
> I'm a bit concerned that we're able to put the user in a state where they
> have network.cookie.cookieBehavior set to "4", but "Third Party Trackers"
> doesn't appear in their list anymore.
> 
> If we don't recompute the selection, the correct label will still show in
> the list, but the user won't be able to select it anymore, which still
> sounds confusing but preferable to me.
> 
> What do you think?

I see.  I actually added this to prevent this exact case of a label that doesn't exist in the drop-down being selected (see the screenshot) being selected, but if you think that's preferable, I'm happy to drop it.

I personally think we should never switch the browser.contentblocking.cookies-site-data.ui.reject-trackers.enabled pref from true to false for our users, which should remove the possibility of getting into this state from this UI.  There is also the possibility of getting into this state from the content blocking UI, of course, which we should pay attention to as well.

I'll remove this call before landing.
(In reply to Johann Hofmann [:johannh] from comment #25)
> ::: browser/locales/en-US/browser/preferences/preferences.ftl
> @@ -734,5 @@
> >  sitedata-learn-more = Learn more
> >  
> > -sitedata-accept-cookies-option =
> > -    .label = Accept cookies and site data from websites (recommended)
> > -    .accesskey = A
> 
> I must have missed this earlier, but surely the new labels need to get those
> accesskeys as well.

Sure, I'll add them.

> @@ -737,5 @@
> > -    .label = Accept cookies and site data from websites (recommended)
> > -    .accesskey = A
> > -
> > -sitedata-block-cookies-option =
> > -    .label = Block cookies and site data (may cause websites to break)
> 
> I understand that this is not a question for you, but we should make sure
> that we really want to remove (may cause websites to break) and (recommend)
> on the new UI.

The new UI includes "(may cause websites to break)".  It also includes "(recommended)" behind a pref...
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/330301cd20be
Part 1: Update the Cookies and Site Data UI in the Preferences window; r=johannh,flod
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bfb53d863aa
Part 2: Remove some obsolete privacy pane tests; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/8177c8bf4e5f
Part 3: Add tests for the new Cookies and Site Data UI; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/c617048d4514
Part 4: Enable hiding the "reject trackers" menu item based on a preference; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/6efdde6260d2
Part 5: Remove the old Cookies & Site Data UI; r=johannh,flod
There were also bc failures on browser/components/enterprisepolicies/tests/browser/browser_policy_cookie_settings.j
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=194857834&repo=mozilla-inbound&lineNumber=2118
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/625b9010085c
Part 1: Update the Cookies and Site Data UI in the Preferences window; r=johannh,flod
https://hg.mozilla.org/integration/mozilla-inbound/rev/26bc4e3382d4
Part 2: Remove some obsolete privacy pane tests; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/374d33355d31
Part 3: Add tests for the new Cookies and Site Data UI; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c75236893de
Part 4: Enable hiding the "reject trackers" menu item based on a preference; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/5794c6564567
Part 5: Remove the old Cookies & Site Data UI; r=johannh,flod
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7784d3275ff7
Part 1: Update the Cookies and Site Data UI in the Preferences window; r=johannh,flod
https://hg.mozilla.org/integration/mozilla-inbound/rev/edc4aecf816e
Part 2: Remove some obsolete privacy pane tests; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/547b082422b0
Part 3: Add tests for the new Cookies and Site Data UI; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b5f60a54a12
Part 4: Enable hiding the "reject trackers" menu item based on a preference; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c038572979
Part 5: Remove the old Cookies & Site Data UI; r=johannh,flod
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89e493ded984
Backed out 5 changesets for bc failures on browser_policy_cookie_settings.js. CLOSED TREE
Flags: needinfo?(ehsan)
Depends on: 1520670
You need to log in before you can comment on or make changes to this bug.