Closed Bug 1231359 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Preferences, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 46
Iteration:
46.2 - Jan 11
Tracking Status
firefox45 --- affected
firefox46 --- verified

People

(Reporter: past, Assigned: Paolo)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Whiteboard: [fxprivacy] [strings])

Attachments

(4 files)

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.
Flags: qe-verify+
Whiteboard: [fxprivacy] → [fxprivacy] [strings]
Assignee: nobody → past
Status: NEW → ASSIGNED
Iteration: --- → 46.1 - Dec 28
Priority: P2 → P1
Summary: Add a checkbox to enable TP in normal mode → [experiment] Add a checkbox to enable TP in normal mode
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.
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
Summary: [experiment] Add a checkbox to enable TP in normal mode → [experiment] Simplify the hidden preferences that control whether Tracking Protection is enabled in non-private windows
Blocks: 1235565
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.
Assignee: past → paolo.mozmail
Iteration: 46.1 - Dec 28 → 46.2 - Jan 11
Attachment #8702580 - Flags: review?(past)
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?
Attachment #8702580 - Flags: review?(past) → review+
(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.
(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.
This is how the change to the Privacy Preferences looks like.
Attachment #8705693 - Attachment description: privacy-updated.png → Screenshot of Privacy Preferences with the Tracking Protection UI enabled manually
https://hg.mozilla.org/mozilla-central/rev/70b34c15015c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
QA Contact: paul.silaghi
Depends on: 1238924
(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.
Flags: needinfo?(paolo.mozmail)
The privacy.donottrackheader.enabled preference should not be affected by changes in the radio button.
Flags: needinfo?(paolo.mozmail)
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
Attached image 1231359_wrong.png
The caption in the wrong place.
Attached image 1231359_right.png
Caption in the right place
(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.
Verified fixed FF 46.0a1 (2016-01-13) Win 7, Ubuntu 14.04, OS X 10.9.5.
Status: RESOLVED → VERIFIED
Right now tracking.label ("Tracking") is not visible at all on OS X and it looks plainly wrong.
(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?
Flags: needinfo?(paolo.mozmail)
(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.
Flags: needinfo?(paolo.mozmail)
(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.
(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.
(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.
Indeed, so make the XUL more complex to simplify the UI, resulting in workarounds in l10n and theming...
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.
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.
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.
relnote-firefox: --- → ?
Flags: needinfo?(rkothari)
s/ritu/liz Sorry :/
Flags: needinfo?(rkothari) → needinfo?(lhenry)
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.
Flags: needinfo?(lhenry)
OK, sorry.
relnote-firefox: ? → ---
You need to log in before you can comment on or make changes to this bug.