1177085 - Add a preferences panel for changing the pre-loaded TP blocklist

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Screenshot

The privacy pane in preferences has a toggle (hidden by default) to "Prevent sites for tracking me". It should be changed to "Prevent sites from tracking my online activity in Private Windows" and a button should appear next to it that brings up a dialog to switch to a different blocklist.

I believe the code for the popup blocking exceptions dialog can be reused here and the patch should also turn on the privacy.trackingprotection.ui.enabled pref to make the feature visible. Unless of course we want to flip all related prefs at a later point in the cycle.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I can work on this as I already have some experience with the preferences code.

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: pref-panel.jpeg

Screenshot of the preferences panel

Comment 3

[Matthew N. [:MattN]]

We're not going to implement the "Restore Defaults" and "Remove" buttons for 42.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Add a preferences panel for changing the pre-loaded TP blocklist

Parking the patch for now, until the required platform work is done. Apart from blocklists.js, everything else should be ready for review.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v2

Rebased on top of latest m-c and fixed a couple of conflicts.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v3

Here is an approach that tries to work around the missing platform API for changing blocklists by directly modifying the two prefs, gethashURL and updateURL. The dialog pref-switching code works, but it doesn't seem to affect the platform code even after a browser restart (i.e. even a bogus URL still results in content blocking).

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

François, do you know if a bogus blocklist URL should break TP or not? Is the blocklist perhaps cached somewhere? If it is, can I purge it somehow?

Comment 8

[François Marier [:francois]]

If the list can't be fetched at the given updateURL, then the browser assumes the server is down and it will try again later.

Old entries are never removed from the cache. They are not supposed to be honored after 45 minutes (they are considered stale) but that's currently broken (see bug 1032393) and so right now, stale entries are honored forever.

If you want to purge the list, you need to delete the files on disk. See https://wiki.mozilla.org/Security/Tracking_protection#List for their location.

Comment 9

[François Marier [:francois]]

Assuming we are serving the other lists ourselves (and we have to until bug 1037560 is fixed), then you could take a different approach:

- leave updateURL and gethashURL as they are
- add a list with a different name (e.g. disconnect-track-digest256)
- set urlclassifier.trackingTable to "test-track-digest256,disconnect-track-digest256"

Note that the last two parts of the list name are fixed ("-track-digest256") and you can only change the first part.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v4

Thanks, that worked! I'm now using 2 prefs per list that provide a name and the id of the list that is to be added to urlclassifier.trackingTable. I've used mozpub2-track-digest256 as the name of the strict list and lo and behold TP stops working! The change also seems to take effect immediately, which is what we wanted.

I probably have to update the urlclassifier.trackingWhitelistTable as well with something like mozpub2-trackwhite-digest256, and the list names will presumably need to be localizable (unless we use brand names?) so a bit more work is still needed. Also, using an icon for the list as in the mockup will need an additional pref per list and I'm not sure that's better from a maintenance POV.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Here is a try push that will produce binaries to play with the patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d667e4a006e7

Once ready the binaries will be available here:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d667e4a006e7

Comment 12

[François Marier [:francois]]

Comment on attachment 8650397 [details] [diff] [review]
WIP v4

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

::: browser/app/profile/firefox.js
@@ +1894,5 @@
>  pref("privacy.trackingprotection.introURL", "https://support.mozilla.org/kb/tracking-protection-firefox");
>  #endif
>  pref("privacy.trackingprotection.introCount", 0);
>  pref("privacy.trackingprotection.introURL", "https://support.mozilla.org/kb/tracking-protection-firefox");
> +// Block lists for tracking protection.

What gcp and I talked about (but isn't reflected in the bug I linked to, sorry!) is that instead of using numerical indices for the prefs (lists.1.* and list.2.*), we could use the first part of the list name.

So your prefs would look like:
	
privacy.trackingprotection.lists.mozpub.name = "Standard (recommended)"
privacy.trackingprotection.lists.mozpub2.name = "Strict"

and then later we could have:

privacy.trackingprotection.lists.mozpub.name.updateURL = ...
privacy.trackingprotection.lists.mozpub2.name.updateURL = ...
privacy.trackingprotection.lists.mozpub.name.gethashURL = ...
privacy.trackingprotection.lists.mozpub2.name.gethashURL = ...
browser.safebrowsing.lists.goog.updateURL = ...
browser.safebrowsing.lists.goog.gethashURL = ...

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to François Marier [:francois] from comment #12)
> and then later we could have:
> 
> privacy.trackingprotection.lists.mozpub.name.updateURL = ...
> privacy.trackingprotection.lists.mozpub2.name.updateURL = ...
> privacy.trackingprotection.lists.mozpub.name.gethashURL = ...
> privacy.trackingprotection.lists.mozpub2.name.gethashURL = ...

I guess you meant to hang the updateURL and gethashURL properties off the actual name (e.g. "mozpub"), not the name property, right? Just like below:

> browser.safebrowsing.lists.goog.updateURL = ...
> browser.safebrowsing.lists.goog.gethashURL = ...

I will update the patch to use this scheme.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

I'm working on bug 1107372 right now and currently it looks like:

pref("browser.safebrowsing.provider.google.lists", "goog-badbinurl-shavar,goog-downloadwhite-digest256,goog-phish-shav
pref("browser.safebrowsing.provider.google.updateURL", "https://safebrowsing.google.com/safebrowsing/downloads?client=
pref("browser.safebrowsing.provider.google.gethashURL", "https://safebrowsing.google.com/safebrowsing/gethash?client=S
pref("browser.safebrowsing.provider.google.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client
pref("browser.safebrowsing.provider.google.appRepURL", "https://sb-ssl.google.com/safebrowsing/clientreport/download?k

pref("browser.safebrowsing.provider.mozilla.lists", "mozpub-track-digest256,mozpub-trackwhite-digest256");
pref("browser.safebrowsing.provider.mozilla.updateURL", "https://tracking.services.mozilla.com/downloads?client=SAFEBR
pref("browser.safebrowsing.provider.mozilla.gethashURL", "https://tracking.services.mozilla.com/gethash?client=SAFEBRO

Comment 15

[Gian-Carlo Pascutto [:gcp]]

The TP lists being out of browser.safebrowsing just adds special cases to the code, which is why I'd prefer to get rid of it. You can still find out who the TP lists are from the names, i.e. *-track-* or *-trackwhite-*.

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v5

This version updates the whitelist along with the blocklist, localizes the list names and uses the preference name suggested by francois and gcp:

pref("browser.safebrowsing.provider.mozilla.lists.mozpub.name", "mozpubName");
pref("browser.safebrowsing.provider.mozilla.lists.mozpub2.name", "mozpub2Name");

What these contain are keys for the localized names in the property file. So this approach is more scalable as it needs one pref per list instead of two with the previous one.

The extra l10n value can only be avoided if we decide that the list names will not be localizable (which seems unlikely).

I am not clear on what browser.safebrowsing.provider.mozilla.lists is supposed to contain: all lists or active lists? If the former, we may omit the browser.safebrowsing.provider.mozilla.lists.XXX.name prefs altogether and enumerate on this pref instead (plus similar lists from other providers once bug 1037560 is fixed). If the latter, I assume I will have to update the patch to modify this pref instead of urlclassifier.trackingTable and urlclassifier.trackingWhitelistTable.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Dialog screenshot

This is what it looks like.

Comment 18

[Francesco Lodolo [:flod]]

Comment on attachment 8652265 [details] [diff] [review]
Patch v5

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

::: browser/locales/en-US/chrome/browser/preferences/blocklists.dtd
@@ +4,5 @@
> +
> +<!ENTITY window.title                 "Block List Providers">
> +<!ENTITY window.width                 "45em">
> +
> +<!ENTITY treehead.selection.label     "">

Why would you need an empty localizable string here?

Comment 19

[Gian-Carlo Pascutto [:gcp]]

(In reply to Panos Astithas [:past] from comment #16)

> I am not clear on what browser.safebrowsing.provider.mozilla.lists is
> supposed to contain: all lists or active lists? 

The lists on which the other things below that pref apply to. These should include the active lists but may include anything for which the preferences are relevant.

pref("browser.safebrowsing.provider.google.lists", "goog-badbinurl-shavar,goog-downloadwhite-digest256,goog-phish-shavar,goog-malware-shavar,goog-unwanted-shavar");
pref("browser.safebrowsing.provider.google.updateURL", "http://%(server)s/safebrowsing-dummy/update");
pref("browser.safebrowsing.provider.google.appRepURL", "http://%(server)s/safebrowsing-dummy/update");

pref("browser.safebrowsing.provider.mozilla.lists", "mozpub-track-digest256,mozpub-trackwhite-digest256");
pref("browser.safebrowsing.provider.mozilla.gethashURL", "http://%(server)s/safebrowsing-dummy/gethash");
pref("browser.safebrowsing.provider.mozilla.updateURL", "http://%(server)s/safebrowsing-dummy/update");

urlclassifier.trackingTable and urlclassifier.trackingWhitelistTable are unchanged - they identify the type of the lists (action to take on a hit), which is unrelated to the provider. They also identify which lists are actually used.

>If the former, we may omit the browser.safebrowsing.provider.mozilla.lists.XXX.name prefs altogether and enumerate on this pref instead (plus similar lists from other providers once bug 1037560 is fixed).

Not sure what you are planning here, but I don't think (for example) it's a good idea to make the order of lists in that thing significant. If there's a generic description for a provider, it can just go like:

pref("browser.safebrowsing.provider.google.name", "Google SafeBrowsing");

If you need names per list instead of per provider, then indeed what you propose is fine. Maybe I'll want to rework my patch to not make .lists a comma-seperated list though, but actually enumerate the keys that sit below .lists.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Francesco Lodolo [:flod] from comment #18)
> Why would you need an empty localizable string here?

Oops, leftover! Fixed locally, thanks.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #19)
> (In reply to Panos Astithas [:past] from comment #16)
> urlclassifier.trackingTable and urlclassifier.trackingWhitelistTable are
> unchanged - they identify the type of the lists (action to take on a hit),
> which is unrelated to the provider. They also identify which lists are
> actually used.

Got it, thanks.

> >If the former, we may omit the browser.safebrowsing.provider.mozilla.lists.XXX.name prefs altogether and enumerate on this pref instead (plus similar lists from other providers once bug 1037560 is fixed).
> 
> Not sure what you are planning here, but I don't think (for example) it's a
> good idea to make the order of lists in that thing significant. If there's a
> generic description for a provider, it can just go like:
> 
> pref("browser.safebrowsing.provider.google.name", "Google SafeBrowsing");
> 
> If you need names per list instead of per provider, then indeed what you
> propose is fine. Maybe I'll want to rework my patch to not make .lists a
> comma-seperated list though, but actually enumerate the keys that sit below
> .lists.

Indeed, we need a name per list to display in the "Change Block List" dialog. It seems to me that enumerating the child keys is slightly easier, but enumerating the strings in the comma-separated list is fine, too.

In the above, I was trying to minimize the amount of prefs needed for each list, which in my current patch is 1 (the key to the localized name). I am using the list name as the actual key in the property file, so the only real need for having an actual property called XXX.lists.mozpub.name is so that I can enumerate all available lists from the children of XXX.lists. I thought that since XXX.mozilla.lists already contains the available lists, I might just extract the l10n key from that and ditch XXX.lists.mozpub.name.

However, if we are going to specify more attributes per list in the future (different URLs? icons? vendors?), then I think the more future-proof approach is the one I've already used.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v6

This version fixes a couple of style issues in the tree pointed out by Aislinn, updates urlclassifier.disallow_completion with the additional list and fixes the l10n issue mentioned by Francesco.

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Updated screenshot

Comment 24

[Tim Nguyen :ntim]

Comment on attachment 8652792 [details] [diff] [review]
Patch v6

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

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +33,5 @@
> +
> +treechildren::-moz-tree-cell-text(checkmark,selected) {
> +  color: var(--in-content-link-color-active);
> +}
> +

Can you use the same checkmark as the search panel instead of using the unicode character ?
See https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/incontentprefs/search.css?offset=100#22

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v7

Updated to prompt the user to restart after the list switch, since francois suggested that this is the only way to guarantee a list update from the server.

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v8

(In reply to Tim Nguyen [:ntim] from comment #24)
> Can you use the same checkmark as the search panel instead of using the
> unicode character ?

Good idea, done.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8654077 [details] [diff] [review]
Patch v8

I think this is ready for review. It won't land until the production servers are updated with the second list, but it should work in the meantime by changing the domain names of the following 2 prefs to tracking.stage.mozaws.net:

browser.trackingprotection.updateURL, browser.trackingprotection.gethashUR

Comment 28

[François Marier [:francois]]

Comment on attachment 8654077 [details] [diff] [review]
Patch v8

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

I like what you've done and I would love to see it used for more than just the TP list. Do you think it would make sense to turn the pref (urlclassifier.trackingTable) into a parameter for the UI and perhaps rename it from blocklistmanager to listmanager?

That way, we could use the listmanager with a parameter like "urlclassifier.malwareTable" to allow users to select the malware list(s) they want to use.

Another thing that would be great to have is a parameter that would turn on multiple selection. In the case of the malware lists, we'll most likely want to have multiple lists selected at once (i.e. adding more than just one list name to the pref).

Of course, it's outside the scope of this bug, so if you'd rather not do it now, that's fine too.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +34,5 @@
> +blockliststitle=Block List Providers
> +# LOCALIZATION NOTE (mozpubName, etc.): These labels appear in the tracking
> +# protection block lists dialog.
> +mozpubName=Standard (recommended)
> +mozpub2Name=Strict

If we're going to ask translators to translate these strings, do you think it makes sense to give better shortnames to these lists?

Right now, they're not very good. Something like "disconnectstandard" and "disconnectfull" might be better.

(Obviously, I'm not criticizing your patch, the names would have to change on the shavar server too.)

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to François Marier [:francois] from comment #28)
> I like what you've done and I would love to see it used for more than just
> the TP list. Do you think it would make sense to turn the pref
> (urlclassifier.trackingTable) into a parameter for the UI and perhaps rename
> it from blocklistmanager to listmanager?

I'm not sure it would be easier to make the dialog generic compared to using it as a basis for the new one, which what I've done with the popup permissions dialog. The strings will certainly be different, but perhaps also functionality will be different (e.g. multiple selection, icons, available buttons, button actions). I'd be happy to work on this and even factor out a common dialog, but in a separate bug where the other dialog's requirements will be clearly laid out.

> > +mozpubName=Standard (recommended)
> > +mozpub2Name=Strict
> 
> If we're going to ask translators to translate these strings, do you think
> it makes sense to give better shortnames to these lists?
> 
> Right now, they're not very good. Something like "disconnectstandard" and
> "disconnectfull" might be better.

Perhaps, if we are comfortable with tying the list name to the vendor/product. What if we decide to switch vendors at some point? Do we want to have to update both Firefox and the cloud service for that?

One implication in that case would be that we wouldn't be able to keep serving old versions of Firefox with the same server API. If we use mozpub as indicative of "the standard Mozilla-served list", then we can change its contents at will without affecting clients.

Comment 30

[François Marier [:francois]]

(In reply to Panos Astithas [:past] from comment #29)
> I'd be happy to work on this and even
> factor out a common dialog, but in a separate bug where the other dialog's
> requirements will be clearly laid out.

Sounds good to me.

> > > +mozpubName=Standard (recommended)
> > > +mozpub2Name=Strict
> > 
> > If we're going to ask translators to translate these strings, do you think
> > it makes sense to give better shortnames to these lists?
> > 
> > Right now, they're not very good. Something like "disconnectstandard" and
> > "disconnectfull" might be better.
> 
> Perhaps, if we are comfortable with tying the list name to the
> vendor/product. What if we decide to switch vendors at some point? Do we
> want to have to update both Firefox and the cloud service for that.

You're right, it may be better to keep the name of the vendor out.

> One implication in that case would be that we wouldn't be able to keep
> serving old versions of Firefox with the same server API. If we use mozpub
> as indicative of "the standard Mozilla-served list", then we can change its
> contents at will without affecting clients.

Good point about backwards compatibility. I guess my only concern is that the name "mozpub2" is not very descriptive and that if we're going to use a second list for something else than testing, we should think about a better name for it. Maybe "mozfull-track-digest256"?

Anyways, we can track this in a separate bug.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to François Marier [:francois] from comment #30)
> Maybe "mozfull-track-digest256"?

I like that.

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8654077 [details] [diff] [review]
Patch v8

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

::: browser/components/preferences/in-content/privacy.js
@@ +372,5 @@
> +   */
> +  showBlockLists: function ()
> +  {
> +    var bundlePreferences = document.getElementById("bundlePreferences");
> +    let brandName = document.getElementById("bundleBrand").getString("brandShortName");

nit, please wrap this line

::: browser/components/preferences/in-content/privacy.xul
@@ +83,5 @@
>    <label class="header-name">&panePrivacy.title;</label>
>  </hbox>
>  
>  <!-- Tracking -->
> +<groupbox id="trackingGroup" data-category="panePrivacy" hidden="true">

Why did you remove the align=start here? Does bug 883777 no longer occur?

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +29,5 @@
>  invalidURITitle=Invalid Hostname Entered
>  
> +#### Block List Manager
> +
> +blockliststext=You can specify which provider(s) list you want to use to block tracking elements on websites. You can add additional lists by visiting the Firefox add-ons library.

With the patch I can only specify one provider list to use at a time. This text implies that I can use multiple lists. Also, this text has "Firefox" hard-coded in it and I couldn't find any mention of "add-ons library" elsewhere in MXR.

Can you please change this text to:
A block list determines which tracking elements on a website are blocked from loading. You can specify which provider's list you want to use by clicking on the name of the list below. Additional lists are available in the Add-ons Manager.

@@ +34,5 @@
> +blockliststitle=Block List Providers
> +# LOCALIZATION NOTE (mozpubName, etc.): These labels appear in the tracking
> +# protection block lists dialog.
> +mozpubName=Standard (recommended)
> +mozpub2Name=Strict

I agree, we should change these names from mozpubName and mozpub2Name to something much more identifiable before releasing this.

@@ +36,5 @@
> +# protection block lists dialog.
> +mozpubName=Standard (recommended)
> +mozpub2Name=Strict
> +# LOCALIZATION NOTE (blocklistChangeRequiresRestart, restartTitle): %S = brandShortName
> +blocklistChangeRequiresRestart=%S must restart in order to update block lists.

%S must restart to change block lists.

I removed "in order" as that is implied, and I changed "update" to "change" because I don't want this to get confused with downloading an "update" for the current block list.

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +25,5 @@
> +treechildren::-moz-tree-cell-text {
> +  font-size: 1.25rem;
> +}
> +
> +#blocklistsTree treechildren::-moz-tree-image(selectionCol, checked) {

Can you please merge the search.css lines that do this same thing with the ones here? We shouldn't duplicate these pretty generic rules. The only thing different between the search.css ones and these is the ID at the beginning.

Comment 33

[François Marier [:francois]]

(In reply to Panos Astithas [:past] from comment #31)
> (In reply to François Marier [:francois] from comment #30)
> > Maybe "mozfull-track-digest256"?
> 
> I like that.

I created bug 1201741 for this.

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v9

Addressed comments, thanks!

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #32)
> Why did you remove the align=start here? Does bug 883777 no longer occur?

It no longer occurs in my testing, but the reason I removed it was to follow the spec that has the button far away from the checkbox label.

> I agree, we should change these names from mozpubName and mozpub2Name to
> something much more identifiable before releasing this.

I am waiting for bug 1201741 to settle on a name and then I will update this patch. We expect this to land shortly before the next branch date anyway.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

I deliberated asking this during the review, and the question still lingers with me post-review.

Why do we need to add this to the preferences? How likely is someone going to want to change their list? This seems *very* heavy add-ons territory here, and something that we shouldn't be complicating our preferences with.

Comment 36

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #35)
> I deliberated asking this during the review, and the question still lingers
> with me post-review.
> 
> Why do we need to add this to the preferences? How likely is someone going
> to want to change their list? This seems *very* heavy add-ons territory
> here, and something that we shouldn't be complicating our preferences with.

I think the only reason is a legal one.
If we can get away with not exposing that at all, we should.

Ash probably knows more about where we are on this with legal…

Comment 37

[Jared Wein [:jaws] (please needinfo? me)]

Is the legal reason that we "need a way to disable this"?, because that is already provided. Otherwise about:config should provide enough of an interface to satisfy a legal requirement that people can change their list.

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 1174999 contains the user story that prompted this work. Providing more than one list is an important feature as far as I know, partly because tracking protection is still somewhat contentious, but also to alleviate potential concerns about not having any other options. Javaun will probably have more context.

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

We don't provide the ability to change where the safe-browsing list is sourced from, nor the safe-downloads list. I don't think we should be incorporating design just to deflect potential questions. We can answer questions through our support docs, and users who feel compelled to change the list will still have the ability to use about:config.

Comment 40

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #39)
> We don't provide the ability to change where the safe-browsing list is
> sourced from, nor the safe-downloads list.

It sounds like the argument you are making is that normal users should be happy with whatever list we pick as the default and only experts (who have heard of about:config) will want to ever switch lists.

The part about TP being contentious above meant to indicate that different lists will provide a different usability/protection trade-off. Sorry if that wasn't clear. That is, some websites will not be usable with the strict list, but will be with the standard list. This sounds to me like a choice we should provide users with. It doesn't seem to me that safe-browsing is in the same category as TP in regards to potential content breakage.

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Yes, I believe normal users should be happy with whatever list we pick as the default. We need to be confident about our choice as the default, and we don't always need to provide built-in UI for overriding the default choice. It's convenient that about:config exists, and we can use that to provide an expert-focused UI for overriding it, but I am highly doubtful that 20% of users will ever think of changing their tracking-protection list.

Some sites may not be usable with tracking protection, but we provide a per-site switch in the location bar to disable the feature. If we think that the strict list breaks so many sites where using the location bar UI to override on a per-site basis becomes too annoying (annoying to the point that we ship a toned-down non-strict list), then that sounds like a pretty bad mark against the strict list.

Comment 42

[Javaun Moradi [:javaun]]

Everything cited above is correct. There are legal and policy concerns that multiple lists mitigates immensely. It's also a choice we want to offer all users. The two lists we have now offer very different tradeoffs, without middle ground. The stricter list offers maximum protection but a lot more breakage. The standard list allows some tracking domains to avoid too much breakage. It's a choice we do want to offer to everyone, though I expect the actual number who do it is less than 1%.

Multiple blocklist support in the UI would be the one additional thing we'd ship in 42 if we could.

Right now, we only have the ability to switch between two lists from Disconnect. At this point, it's not even decided which of two will ship. The diary study will help decide that. We'd like to get more curated from more partner providers served by our Shavar service in the near future. Beyond that, there may be choices available via Add-on.  We have a default choice, but we also want to be a platform for anyone to build/bundle their own list.

Comment 43

[Javaun Moradi [:javaun]]

I have list names from Disconnect, to be used in the UI:

For list A (the current default), the string should be:
"Disconnect Protection (Default)" 

For list B (the strict list), the string should be: 
"Disconnect Aggressive Protection"

Comment 44

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Javaun Moradi [:javaun] from comment #42)
> Right now, we only have the ability to switch between two lists from
> Disconnect. At this point, it's not even decided which of two will ship.

Are you saying that there is a possibility that we may only ship one list?

> Beyond that, there may be choices available via Add-on.  We have a
> default choice, but we also want to be a platform for anyone to
> build/bundle their own list.

Why not provide a Mozilla-created add-on hosted on AMO that can do all of this?

Comment 45

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v10

Updated the list names per comment 43 and also updated the patch to use the new list names mozstd and mozfull, since I expect this to land after all the server-side changes are deployed.

Comment 46

[Panos Astithas (he/him) [:past] (please ni?)]

Oh and another minor thing, I moved the list name prefs from firefox.js to all.js as I expect other products to use them in the future, but also to be close to their parent prefs.

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8658785 [details] [diff] [review]
Patch v10

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

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +33,5 @@
> +blockliststext=A block list determines which tracking elements on a website are blocked from loading. You can specify which provider's list you want to use by clicking on the name of the list below. Additional lists are available in the Add-ons Manager.
> +blockliststitle=Block List Providers
> +# LOCALIZATION NOTE (mozstdName, etc.): These labels appear in the tracking
> +# protection block lists dialog. They are the names of the block lists.
> +mozstdName=Disconnect Protection (Default)

The way that we do our capitalization, this can be confusing and interpreted as to "remove protection" where "remove" is a synonym for "disconnect".

Is there any other way that we can reword this to disambiguate?

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

(I guess it has nothing to do with our capitalization, but my question still stands)

Comment 49

[Javaun Moradi [:javaun]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #44)
> Are you saying that there is a possibility that we may only ship one list?

Sorry, to be clear, we will ship TP in PBM with only single list support in Fx42. At this time, we haven't decided which of the two Disconnect lists will be the one that ships. A user study is underway to help figure that out. This work for multiple lists is for Fx43, and at that time the second list will be offered as an alternate choice.

> Why not provide a Mozilla-created add-on hosted on AMO that can do all of
> this?

We want to offer more options in-product. There isn't a perfect list for all users, there are big tradeoffs for either.

Comment 50

[Javaun Moradi [:javaun]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #47)
> The way that we do our capitalization, this can be confusing and interpreted
> as to "remove protection" where "remove" is a synonym for "disconnect".
> 
> Is there any other way that we can reword this to disambiguate?

These names came from the vendor, Disconnect. Let me talk to UX and copy and see what they say.

Comment 51

[Jared Wein [:jaws] (please needinfo? me)]

How about "Normal Protection by Disconnect" and "Strict Protection by Disconnect"?

Comment 52

[Javaun Moradi [:javaun]]

Final copy changes as seen by vendor (Disconnect), creative team, and legal review.
===

// HEADER IN MODAL WINDOW
Block Lists

// WINDOW DESCRIPTOR TEXT

You can choose which list Firefox will use to block Web elements that may track your browsing activity.

// FOR THE CURRENT, LESS STRICT LIST / A LIST

Disconnect.me basic protection (Recommended). Allows some trackers so websites function properly.

// FOR THE NEW, STRICTER B LIST
Disconnect.me strict protection. Blocks known trackers. Some sites may not function properly.

Comment 53

[Francesco Lodolo [:flod]]

Quick note, at risk of stating the obvious: if we want this in Firefox 43, it needs to land in m-c before Monday. Or, worst case scenario, we need at least the strings in.

Comment 54

[Javaun Moradi [:javaun]]

Flod: absolutely. Panos and I spoke this morning, I told him I'd have final strings by end of day today, so he could start the work first thing tomorrow (NI Panos).

Comment 55

[François Marier [:francois]]

While the new list (mozfull-track-digest256) is not yet in production, you can go ahead and land this with the new name. The client will start pulling it as soon as it's available on the shavar server.

Comment 56

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Latest screenshot

This is what the dialog looks like with the new strings.

Comment 57

[Axel Hecht [:Pike]]

(In reply to Panos Astithas [:past] from comment #56)
> Created attachment 8662336 [details]
> Latest screenshot
> 
> This is what the dialog looks like with the new strings.

How would this look with translations are 1.5-2 times as long as English?

Comment 58

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Axel Hecht [:Pike] from comment #57)
> How would this look with translations are 1.5-2 times as long as English?

The list will contain as much text as possible with an ellipsis at the end, but the entire string will be visible in the tooltip.

Comment 59

[Francesco Lodolo [:flod]]

(In reply to Panos Astithas [:past] from comment #58)
> The list will contain as much text as possible with an ellipsis at the end,
> but the entire string will be visible in the tooltip.

Is there any chance to make the text wrap on two lines instead? 

Consider that most languages have a +20/30% inflation, meaning for 100 characters in English, you'll end up with more than 120/130 characters in German, French, etc. 

Exceptions are languages like Japanese and Chinese, but you have exceptions in the other direction too (Scottish Gaelic, Irish).

The window's width is localizable, but having a 1200px wide dialog is not great UI either, especially when the buttons are in one corner.

Comment 60

[Axel Hecht [:Pike]]

Also, if we stick to the wording, the text that gets cut off is "website functions properly", i.e., the actual consequence and informing piece to base ones decision on.

Comment 61

[Javaun Moradi [:javaun]]

Release Note Request (optional, but appreciated)
[Why is this notable]: This is multiple blocklist support for the Tracking Protection Feature in PBM
[Suggested wording]: User selectable second block list for Private Browsing's Tracking Protection
[Links (documentation, blog post, etc)]: TBD...

Comment 62

[Liz Henry (:lizzard) (relman/hg->git project)]

It looks like this might be aimed at release notes for 43 later when it's in beta (or release) but since the work hasn't landed yet I don't think it needs a note for 43 aurora.

Comment 63

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Francesco Lodolo [:flod] from comment #59)
> Is there any chance to make the text wrap on two lines instead? 

We discussed this in the team meeting yesterday and there was consensus that this would look weird. In order to get this in for 43 I made the minimal adjustment of reverting the tree cell font inflation, which gives us some extra breathing room (around 30% for the longer string and 50% for the shorter one). While not perfect, we feel this is a good compromise for v1 and there are plans to improve this dialog in the near future.

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0ea13f8b1e77

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ea13f8b1e77

Comment 66

[Francesco Lodolo [:flod]]

I think you just introduced an accesskey conflict using "c" (sadly, good luck in finding one available in that chaos).

<!ENTITY changeBlockList.label          "Change Block List">
<!ENTITY changeBlockList.accesskey      "C">

<!ENTITY  acceptThirdParty.pre.label      "Accept third-party cookies:">
<!ENTITY  acceptThirdParty.pre.accesskey  "c">

Comment 67

[Panos Astithas (he/him) [:past] (please ni?)]

Oops! I missed the conflict because the cookie accesskey is not present in the default configuration. I'll see what I can come up with.

Comment 68

[Paul Silaghi, QA [:pauly]]

I'm not sure I completely understand the implications of this feature.
In private browsing, on cnn.com with "disconnect.me basic protection" => control center says: "nightly is blocking parts of the page that may track your browsing", TP shield displayed.
With "disconnect.me strict protection" => control center says: "no tracking elements detected on this page", TP shield is not displayed.
44.0a1 (2015-10-12), Win 7 x64
Can you please clarify what are the expected results here, and how to test this further?

Comment 69

[Panos Astithas (he/him) [:past] (please ni?)]

This is weird, I can reproduce this on fx-team. It seems like updating the urlclassifier.trackingTable pref no longer makes the platform respect the change. Here is the extra log output from a test run after the browser was restarted following the change from the standard to the strict list:

[...]
37834752[124f14500]: Active table: goog-badbinurl-shavar
37834752[124f14500]: Active table: goog-malware-shavar
37834752[124f14500]: Active table: goog-phish-shavar
37834752[124f14500]: Active table: goog-unwanted-shavar
37834752[124f14500]: Active table: mozstd-track-digest256
37834752[124f14500]: Active table: mozstd-trackwhite-digest256
37834752[124f14500]: Active table: test-malware-simple
37834752[124f14500]: Active table: test-phish-simple
37834752[124f14500]: Active table: test-track-simple
37834752[124f14500]: Active table: test-trackwhite-simple
37834752[124f14500]: Active table: test-unwanted-simple
37834752[124f14500]: Cleaning up backups.
37834752[124f14500]: Done applying updates.
37834752[124f14500]: update took 91ms
[...]
mozstd-track-digest256;a:1442352906
mozstd-trackwhite-digest256;a:1442372824
test-malware-simple;a:1
test-phish-simple;a:1
test-track-simple;a:1-2
test-trackwhite-simple;a:1
test-unwanted-simple;a:1
37834752[124f14500]: Marking table goog-phish-shavar as last updated on 1563661745
37834752[124f14500]: Marking table goog-malware-shavar as last updated on 1563661745
37834752[124f14500]: Marking table goog-unwanted-shavar as last updated on 1563661745
37834752[124f14500]: Marking table goog-badbinurl-shavar as last updated on 1563661745
37834752[124f14500]: Marking table mozfull-track-digest256 as last updated on 1563663882
37834752[124f14500]: Marking table mozstd-trackwhite-digest256 as last updated on 1563663882
1965899776[101236540]: nsChannelClassifier[125a7bd20]: Classifying principal https://self-repair.mozilla.org/en-US/repair on channel with uri https://self-repair.mozilla.org/en-US/repair
1965899776[101236540]: nsChannelClassifier[125a7bd20]: suspended channel 125a78e50
37834752[124f14500]: Checking fragment self-repair.mozilla.org/en-US/
37834752[124f14500]: Checking fragment self-repair.mozilla.org/en-US/repair
37834752[124f14500]: Checking fragment self-repair.mozilla.org/
[...]


It appears that the standard list is still active somehow after the restart. François, has anything changed in this area recently?

Comment 70

[Gian-Carlo Pascutto [:gcp]]

(In reply to Panos Astithas [:past] from comment #69)
> 37834752[124f14500]: Active table: mozstd-track-digest256
> [...]
> mozstd-track-digest256;a:1442352906
> mozstd-trackwhite-digest256;a:1442372824
> [...]
> 37834752[124f14500]: Marking table mozfull-track-digest256 as last updated
> on 1563663882
> 37834752[124f14500]: Marking table mozstd-trackwhite-digest256 as last
> updated on 1563663882

An interaction between bug 1175562 remembering when a provider was last updated, and the current design only partially updating a provider?

Comment 71

[François Marier [:francois]]

I filed bug 1214454 which describes the problem and suggests a fix for this.

Comment 72

[Paul Silaghi, QA [:pauly]]

Could you please clarify what are the differences between the two blocklists and how to test them?

Comment 73

[François Marier [:francois]]

(In reply to Paul Silaghi, QA [:pauly] from comment #72)
> Could you please clarify what are the differences between the two blocklists
> and how to test them?

The strict list is the same as the standard list, except it also includes the sites in the "Content" category of https://services.disconnect.me/disconnect.json

This test page should show a happy fox using both the strict and the standard lists:

  https://people.mozilla.org/~fmarier/tracking-test/

because it tries to load an iframe from extremetracking.com which is on both lists.

This one however will only show a happy fox when using the strict list (and a sad cat using the standard list):

  https://people.mozilla.org/~fmarier/tracking-test/full.html

because it tries to load an iframe from gravatar.com which is only on the strict list.

Comment 74

[Paul Silaghi, QA [:pauly]]

Verified fixed on 43.0a2 (2015-10-19), 44.0a1 (2015-10-19) Win 7.

Comment 75

[Liz Henry (:lizzard) (relman/hg->git project)]

Noting for 43 beta.  Is there something we can link to, an example or documentation? Thanks.

Comment 76

[Panos Astithas (he/him) [:past] (please ni?)]

I'm not sure, Javaun might now: do we mention the multiple blocklist support in a blog post or SUMO article?

Comment 77

[Javaun Moradi [:javaun]]

Great point, we don't mention it in either. I'll start a thread with Joni. Even though 42 just launched I think it would be great to put a "coming soon in 43" section on the sumo page that shows how to use multiple blocklist support. A lot of users are looking at that, so it would be great to put it right there.

https://support.mozilla.org/en-US/kb/tracking-protection-pbm?as=u&utm_source=inproduct

Comment 78

[Liz Henry (:lizzard) (relman/hg->git project)]

Joni or Javaun, I'm going over a draft of release notes for 43 now. Should I link to the existing SUMO page?

Comment 79

[Joni Chan]

Hi Lizzard, Javaun and I have updated the SUMO page. Here's the direct link to the section. Feel free to use it if Javaun's okay with that: https://support.mozilla.org/kb/tracking-protection-pbm#w_to-change-your-blocklist