1231359 - [experiment] Simplify the hidden preferences that control whether Tracking Protection is enabled in non-private windows

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

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

We need a new checkbox in the privacy section of the preferences tab for enabling TP in normal browsing mode. It will be followed by two radio buttons for selecting whether TP will be used in private windows (Default), "Always", or never use TP in case the checkbox is unchecked. Choosing “Always” (use TP) will enable DNT by flipping the privacy.donottrackheader.enabled pref and send the DNT signal to all sites.

Comment 1

[:Paolo Amadini]

If I understand correctly, this UI will still be displayed only when the privacy.trackingprotection.ui.enabled preference is set and the channel is Nightly or Developer Edition, so that we can complete the other features like the UI tour before this option is available for wider testing. Is this correct?

Also, I think the latest thinking is that the privacy.donottrackheader.enabled preference will not be modified by these preferences, but controlled separately. The platform will send the DNT header to tracking sites that are known to honor it, and as such are not blocked.

Comment 2

[:Paolo Amadini]

After discussing at the team standups and hearing from Philipp, we've decided to implement a slightly revised version of the interface:

Use Tracking Protection
( ) Always
(*) Only in Private Browsing
( ) Never

I've updated this bug to reflect that, and I've created a patch to implement this without changing how the privacy.trackingprotection.ui.enabled preference works and in which channels it is enabled. We can integrate those changes at a later time.

Here is a tryserver build to see if there are any test failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f62afe096c

Comment 3

[:Paolo Amadini]

I've filed bug 1235565 for the changes that are not included in the current patch. Panos, you may want to use that bug number for the changes you originally implemented here that are still relevant.

Comment 4

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1231359 - Simplify the hidden preferences that control whether Tracking Protection is enabled in non-private windows.

Review commit: https://reviewboard.mozilla.org/r/29131/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29131/

Comment 5

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

Comment on attachment 8702580 [details]
MozReview Request: Bug 1231359 - Simplify the hidden preferences that control whether Tracking Protection is enabled in non-private windows.

https://reviewboard.mozilla.org/r/29131/#review26361

Just a few nits, looks good!

::: browser/components/preferences/in-content/privacy.js:143
(Diff revision 1)
> +    var radiogroup = document.getElementById("trackingProtectionRadioGroup");

Nit: s/var/let/g here and in the function below.

::: browser/components/preferences/in-content/privacy.js:155
(Diff revision 1)
> +  {

Style nit: opening curly in the same line as the function keyword?

I know this file has both styles, but since we are trying to use eslint for this kind of thing going forward, we might as well not introduce new deviations from the norm.

::: browser/components/preferences/in-content/privacy.xul:91
(Diff revision 1)
> -                accesskey="&trackingProtection5.accesskey;"
> +          <label id="trackingProtectionLearnMore" class="text-link"

I might have missed last week's updates, but wasn't our previous agreement to go with the question mark icon (the standard one) instead of a text link as indicated in the spec?

Comment 6

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #5)
> I might have missed last week's updates, but wasn't our previous agreement
> to go with the question mark icon (the standard one) instead of a text link
> as indicated in the spec?

I think we decided for the text link, but if we want the question mark we'll need someone from UX to provide the right icon.

This bug also needs a copy review before landing.

Comment 7

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

(In reply to :Paolo Amadini from comment #6)
> (In reply to Panos Astithas [:past] from comment #5)
> > I might have missed last week's updates, but wasn't our previous agreement
> > to go with the question mark icon (the standard one) instead of a text link
> > as indicated in the spec?
> 
> I think we decided for the text link, but if we want the question mark we'll
> need someone from UX to provide the right icon.

That's why I mentioned "the standard one", by which I was referring to the same icon we use in section headers:

https://dxr.mozilla.org/mozilla-central/rev/0771c5eab32f0cee4f7d12bc382298a81e0eabb2/browser/components/preferences/in-content/privacy.xul#84-85

Let's discuss this in the meeting today.

Comment 8

[:Paolo Amadini]

Attachment: Screenshot of Privacy Preferences with the Tracking Protection UI enabled manually

This is how the change to the Privacy Preferences looks like.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/70b34c15015c

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/70b34c15015c

Comment 11

[Paul Silaghi, QA [:pauly]]

(In reply to Panos Astithas [:past] from comment #0)
> Choosing “Always” (use TP) will enable DNT by flipping the
> privacy.donottrackheader.enabled pref
This is not happening, and I'm not sure if it's expected, based on comments 1, 2.

Comment 12

[:Paolo Amadini]

The privacy.donottrackheader.enabled preference should not be affected by changes in the radio button.

Comment 13

[Alfred Kayser]

Why is the <caption> moved to inside all these boxes?
It must be the first element in the groupbox, in order to be styled correctly (in other themes).
See attachments

Comment 14

[Alfred Kayser]

Attachment: 1231359_wrong.png

The caption in the wrong place.

Comment 15

[Alfred Kayser]

Attachment: 1231359_right.png

Caption in the right place

Comment 16

[:Paolo Amadini]

(In reply to Alfred Kayser from comment #13)
> Why is the <caption> moved to inside all these boxes?
> It must be the first element in the groupbox, in order to be styled
> correctly (in other themes).

With the default theme, this makes the buttons on the right aligned in a better way. The difference however is minor enough that I wouldn't mind reviewing a patch in a new bug for moving the <caption> out of the hbox.

Comment 17

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 46.0a1 (2016-01-13) Win 7, Ubuntu 14.04, OS X 10.9.5.

Comment 18

[Stefan Plewako [:stef]]

Right now tracking.label ("Tracking") is not visible at all on OS X and it looks plainly wrong.

Comment 19

[Stefan Plewako [:stef]]

(In reply to Stefan Plewako [:stef] from comment #18)
> Right now tracking.label ("Tracking") is not visible at all on OS X and it
> looks plainly wrong.

Is this expected?

Comment 20

[:Paolo Amadini]

(In reply to Stefan Plewako [:stef] from comment #18)
> Right now tracking.label ("Tracking") is not visible at all on OS X and it
> looks plainly wrong.

What looks wrong in particular? If you have the "privacy.trackingprotection.ui.enabled" preference set to false, then you should see "tracking.label" in the same way as the current Aurora branch. If the preference is set to true, then you should see the new "trackingProtectionHeader.label" header instead.

Comment 21

[Stefan Plewako [:stef]]

(In reply to :Paolo Amadini from comment #20)
> What looks wrong in particular? If you have the
> "privacy.trackingprotection.ui.enabled" preference set to false, then you
> should see "tracking.label" in the same way as the current Aurora branch. If
> the preference is set to true, then you should see the new
> "trackingProtectionHeader.label" header instead.

It shouldn't replace header/caption (in bold) and should be displayed below - see location bar section below if in doubt.

Comment 22

[:Paolo Amadini]

(In reply to Stefan Plewako [:stef] from comment #21)
> It shouldn't replace header/caption (in bold) and should be displayed below
> - see location bar section below if in doubt.

This is expected, it's an intentional design decision to keep the UI simple, despite being a slightly different solution from what we usually do in the preferences dialog.

Comment 23

[Stefan Plewako [:stef]]

(In reply to :Paolo Amadini from comment #22)
> This is expected, it's an intentional design decision to keep the UI simple,
> despite being a slightly different solution from what we usually do in the
> preferences dialog.

Thankfully this aspect of incontent preferences mess is workaroundable in l10n.

Comment 24

[Alfred Kayser]

Indeed, so make the XUL more complex to simplify the UI, resulting in workarounds in l10n and theming...

Comment 25

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

In this case the complexity is imposed by external constraints that require keeping the old UI around along with the new. If anyone wants to help, Paolo's offer in comment 16 still stands.

Comment 26

[Stefan Plewako [:stef]]

Tested this in latest nightly by adding back the old caption, replacing trackingprotectionbox caption with its content (rm caption tags), removing trackingprotectionpbmbox caption entirely with devtools inpector and…

(In reply to Panos Astithas [:past] from comment #25)
> In this case the complexity is imposed by external constraints that require
> keeping the old UI around along with the new.

Could you explain that in more detail?
I don't see how repurposing caption has anything to do with alternative version living behind the pref.

> If anyone wants to help,
> Paolo's offer in comment 16 still stands.

(In reply to :Paolo Amadini from comment #16)
> With the default theme, this makes the buttons on the right aligned in a
> better way. The difference however is minor enough that I wouldn't mind
> reviewing a patch in a new bug for moving the <caption> out of the hbox.

Current implementation seems to make buttons on the right aligned in a better way only if the decision was made to repurpose caption - when left untouched buttons align equally well in the context where they belong and not to some undocumented atheistic author decision about what captions/headers are for and how they should display in relation to other elements.

I also think that it was proven that the change in not that minor at all and explanations provided do not meet reality really.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Highlight the feature
[Suggested wording]: Tracking Protection can be enabled in non-private windows 
[Links (documentation, blog post, etc)]:

Ritu, maybe we want to add this in the release notes.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

s/ritu/liz Sorry :/

Comment 29

[Javaun Moradi [:javaun]]

Folks, we are not shipping in 46. There was some under-the-hood work here to help facilitate future experimentation. This work would allow us to more easily launch experiments to test Tracking Protection for breakage, collect user data and feedback, etc. But this work is just that foundational work. We haven't done the experiments. Tracking Protection cannot be enabled any more easily outside Private Windows, the only method is still via about:config. 

We should not issue a release note here.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

OK, sorry.