Closed Bug 1031033 Opened 6 years ago Closed 6 years ago

expose privacy.trackingprotection.enabled in privacy preferences

Categories

(Firefox :: Preferences, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.2

People

(Reporter: mmc, Assigned: mmc)

References

Details

Attachments

(1 file, 10 obsolete files)

8.47 KB, patch
adw
: review+
Details | Diff | Splinter Review
This should be a checkbox in the privacy preferences.
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8446895 - Attachment is obsolete: true
Attached image newpref_first.png (obsolete) —
Potential point of confusion with this UI: It looks like the enforcement feature is coupled to the DNT feature. A spacer might be warranted to separate the two options and make sure people don't think they have to specify a DNT option to use tracking protection. Space could also probably be better used here if the radio buttons were converted to a menulist instead. This would also make it much simpler. As-is, it looks somewhat like a 4 choice option at first glance.
Attachment #8446899 - Attachment is obsolete: true
Attached image newpref.png (obsolete) —
Attachment #8446901 - Attachment is obsolete: true
(In reply to Dave Garrett from comment #4)
> Potential point of confusion with this UI: It looks like the enforcement
> feature is coupled to the DNT feature. A spacer might be warranted to
> separate the two options and make sure people don't think they have to
> specify a DNT option to use tracking protection. Space could also probably
> be better used here if the radio buttons were converted to a menulist
> instead. This would also make it much simpler. As-is, it looks somewhat like
> a 4 choice option at first glance.

Thanks for the feedback. This bug is for the new preference only. I added space and left DNT alone.
Current mockup: attachment 8459851 [details]
Points: --- → 3
Component: DOM: Security → Preferences
Flags: firefox-backlog+
Product: Core → Firefox
Duplicate of this bug: 1043779
(In reply to Dave Garrett from comment #4)
> Potential point of confusion with this UI: It looks like the enforcement
> feature is coupled to the DNT feature. A spacer might be warranted to
> separate the two options and make sure people don't think they have to
> specify a DNT option to use tracking protection. Space could also probably
> be better used here if the radio buttons were converted to a menulist
> instead. This would also make it much simpler. As-is, it looks somewhat like
> a 4 choice option at first glance.

Agreed - I thought there wasn't enough contextual separation between the two things when I first saw the mockup.

Philipp: Thoughts?
Flags: needinfo?(philipp)
FYI: we're going back to the single checkbox for the DNT pref (see bug 1042135)... mentioning it here in case it matters for design.
See Also: → 1042135
Comment on attachment 8448270 [details]
newpref.png

Obsoleting this screenshot since it will change when the DNT pref gets changed.
Attachment #8448270 - Attachment is obsolete: true
Ah! Thanks, Sid. No extra info needed from Philipp then.
Flags: needinfo?(philipp)
FWIW, This mockup shows the intended design: https://bug1029193.bugzilla.mozilla.org/attachment.cgi?id=8459849

It still shows radio buttons for DNT options, but you see how that section is separated by space and the »Learn More« link.
Attachment #8448268 - Attachment is obsolete: true
Attached image pref.png (obsolete) —
Comment on attachment 8478722 [details] [diff] [review]
Expose tracking protection pref in privacy pref pane

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

Hi Drew,

I spent an embarrassingly long time trying to get an icon to show up for the checkbox, but no go. There weren't too many models of things in prefs that show a related image, with the exception of the application menu items. Any help is welcome.

I also wasn't sure how to test non-in-content prefs from Nightly anymore, or what kind of automated tests are required for exposing a bool pref.

Thanks,
Monica
Attachment #8478722 - Flags: feedback?(adw)
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #17)
> There weren't too many models of things in prefs that show a related image
The translation prefs seems to have an image added by bug 1022856:
http://hg.mozilla.org/mozilla-central/rev/2d9116d8711b#l2.22

An existing image usage is for the Sync Prefs UI:
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/sync.xul#66
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/sync.xul#65

I don't know much about how in-content prefs are implemented though.
(In reply to Ed Lee :Mardak from comment #18)
> (In reply to [:mmc] Monica Chew (please use needinfo) from comment #17)
> > There weren't too many models of things in prefs that show a related image
> The translation prefs seems to have an image added by bug 1022856:
> http://hg.mozilla.org/mozilla-central/rev/2d9116d8711b#l2.22
You should be able to see that UI by setting browser.translation.ui.show true and opening preferences -> Content -> Languages
Attachment #8478722 - Attachment is obsolete: true
Attachment #8478722 - Flags: feedback?(adw)
Attached image pref.png (obsolete) —
Attachment #8478723 - Attachment is obsolete: true
Comment on attachment 8478733 [details] [diff] [review]
Expose tracking protection pref in privacy pref pane

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

Ed was right, as usual. Thanks!
Attachment #8478733 - Flags: review?(adw)
Comment on attachment 8478733 [details] [diff] [review]
Expose tracking protection pref in privacy pref pane

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

Thanks Monica, Ed.  I don't think this needs a test.

::: browser/components/preferences/in-content/privacy.xul
@@ +90,5 @@
> +  </hbox>
> +  <label id="trackingProtectionLearnMore"
> +         class="text-link"
> +         value="&trackingProtectionLearnMore.label;"
> +         onclick="gAdvancedPane.openTextLink(event)"/>

Is there a reason for this onclick?  I don't think you need it since you're setting the label's href.  Actually openTextLink isn't defined on the in-content gAdvancedPane, so if you right-click this link (to bypass the href but trigger the onclick), you get:

JavaScript error: about:preferences#privacy, line 1: gAdvancedPane.openTextLink is not a function

http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/advanced.js

::: browser/components/preferences/privacy.xul
@@ +96,5 @@
> +      </hbox>
> +      <label id="trackingProtectionLearnMore"
> +             class="text-link"
> +             value="&trackingProtectionLearnMore.label;"
> +             onclick="gAdvancedPane.openTextLink(event)"/>

Same here, except here when I right click I get:

JavaScript error: chrome://browser/content/preferences/preferences.xul, line 1: gAdvancedPane is not defined

Which I don't understand because advanced.js is included in preferences.xul (via advanced.xul).

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +10,5 @@
>  <!ENTITY dntTrackingNotOkay.accesskey   "n">
>  <!ENTITY dntTrackingOkay.label2         "Tell sites that I want to be tracked">
>  <!ENTITY dntTrackingOkay.accesskey      "h">
> +<!ENTITY trackingProtection.label       "Prevent sites from tracking me">
> +<!ENTITY trackingProtection.accesskey   "P">

P is already used in this file, and the Privacy pane, for privateBrowsingPermanent2.accesskey: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/privacy.dtd#76

There it's lowercase, but we should avoid using the same key at all.  T would be good but it's taken, too.  So is S.

How about "m" for "me"?  That'll cause the M in "from" to be underlined on platforms that actually draw underlines, but at least it's still the first letter of one of the words.  Plus it's me-centered which is always nice. :-)  But if you have a better idea that's not already used, feel free to choose it.
Attachment #8478733 - Flags: review?(adw) → review+
Let's hold off on checking this in, please, until you get a sign-off from me or Chad.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #24)
> Let's hold off on checking this in, please, until you get a sign-off from me
> or Chad.

OK -- can I check in the string changes only for maximum flexibility?
Flags: needinfo?(gavin.sharp)
Yes, we should check the strings in.
Flags: needinfo?(gavin.sharp)
Attachment #8478733 - Attachment is obsolete: true
Comment on attachment 8480155 [details] [diff] [review]
Expose tracking protection pref in privacy pref pane

Carrying over review from adw in comment 23

> Is there a reason for this onclick? 

Nope, removed. Also thanks for the accesskey tip, fixed :) String changes are in.
Attachment #8480155 - Flags: review+
See Also: → 1060852
This can't be enabled on fennec until https://bugzilla.mozilla.org/show_bug.cgi?id=1063831 is fixed.
Depends on: 1063831
No longer depends on: 1063831
Depends on: 1081343
This is a modified version of attachment 8480155 [details] [diff] [review] that is unbitrotted, and makes this UI conditional on privacy.trackingprotection.ui.enabled (added in bug 1081343), and Nightly-only for the moment (since bug 1081343 is for now).
Attachment #8478735 - Attachment is obsolete: true
Attachment #8480155 - Attachment is obsolete: true
Attachment #8510687 - Flags: review?(adw)
Comment on attachment 8510687 [details] [diff] [review]
patch with pref control on whether this UI appears

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

LGTM. Seems redundant to have #ifdef NIGHTLY around _initTrackingProtection since privacy.trackingprotection.ui.enabled is already #ifdef NIGHTLY'ed in bug 1081343. Thanks for unbitrotting.
Comment on attachment 8510687 [details] [diff] [review]
patch with pref control on whether this UI appears

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

You could avoid the #ifdef if you try-catch the getBoolPref("privacy.trackingprotection.ui.enabled") and return early on exception.  Not a big deal, but that would mean we wouldn't have to return to this once we add privacy.trackingprotection.ui.enabled to all builds.

::: browser/components/preferences/in-content/privacy.js
@@ +27,5 @@
> +    let link = document.getElementById("trackingProtectionLearnMore");
> +    let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "tracking-protection";
> +    if (url) {
> +      link.setAttribute("href", url);
> +    }

else, hide `link`?  Actually, `url` should never be falsey, since you're concatenating the return value with a nonempty string.
Attachment #8510687 - Flags: review?(adw) → review+
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #33)
> LGTM. Seems redundant to have #ifdef NIGHTLY around _initTrackingProtection
> since privacy.trackingprotection.ui.enabled is already #ifdef NIGHTLY'ed in
> bug 1081343. Thanks for unbitrotting.

As Drew mentions, the ifdef is needed since if the default pref doesn't exist, the getBoolPref call will throw, and I didn't want to add a try/catch.

(In reply to Drew Willcoxon :adw from comment #34)
> You could avoid the #ifdef if you try-catch the
> getBoolPref("privacy.trackingprotection.ui.enabled") and return early on
> exception.  Not a big deal, but that would mean we wouldn't have to return
> to this once we add privacy.trackingprotection.ui.enabled to all builds.

I was assuming we'd just rip out all the #ifdef NIGHTLYs at that point.

> else, hide `link`?  Actually, `url` should never be falsey, since you're
> concatenating the return value with a nonempty string.

Yep, fixed that. Thanks.
https://hg.mozilla.org/integration/fx-team/rev/42a1ca26cbf9
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → Firefox 36
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
Iteration: --- → 36.2
Flags: qe-verify?
Depends on: 1091901
Depends on: 1104730
Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5: 
- using latest Aurora 36.0a2 (07-01-2015) : no checkbox for tracking protection is displayed in the privacy preferences (because the UI for tracking protection is not uplifted yet to Aurora)
- using latest Nightly 37.0a1 (07-01-2015) : the checkbox for tracking protection is displayed in the privacy pane (if you set the privacy.trackingprotection.ui.enabled pref to true)
You need to log in before you can comment on or make changes to this bug.