Opened 2 years ago

Closed 2 years ago

Last modified 10 months ago

#20244 closed defect (fixed)

Move privacy checkboxes to about:preferences#privacy (proposed)

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-usability, TorBrowserTeam201610, tbb-fingerprinting, tbb-no-uplift
Cc: gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I would like to propose moving the Privacy Settings checkboxes to the about:preferences#privacy page. That means removing them from torbutton and adding them to tor-browser.git

Here is my reasoning:

  1. The security slider can be separated from the privacy settings. The privacy checkboxes are dangerous, because the default (all checked) offers maximum privacy, and unchecking any of the checkboxes substantially reduces privacy. Whereas the security slider has low security by default, and moving the slider always increases security. So I think it's better to encourage use of the security slider and discourage use of the privacy checkboxes.
  1. We unify the privacy user interface, which hopefully simplifies it for users. There are already privacy settings for Firefox on the about:preferences#privacy page, so it seems natural to move them there. Also the "Always Use Private Browsing Mode" checkbox on that page is more or less equivalent to torbutton's "Don't record browsing history" checkbox. And then we don't have to duplicate the "restart" prompt associated with that checkbox.
  1. Hopefully we can uplift to mainline Firefox, one of our SponsorU goals, thereby exposing our great privacy features (first party isolation, fingerprinting resistance, etc.) to Firefox users in the privacy UI.

Child Tickets

Change History (17)

comment:1 Changed 2 years ago by gk

Cc: gk added

Good idea, although that might not fit into SponsorU anymore (time-wise).

comment:2 Changed 2 years ago by arthuredelstein

Status: newneeds_review

I've written patches for this, because it's going to make the problems I encountered in #18093 much easier to solve.

There are 6 patches here (in which I remove checkboxes one by one and then clean up the UI):
https://github.com/arthuredelstein/torbutton/commits/20244
and 2 patches here (where I add two checkboxes to the about:preferences#privacy page):
https://github.com/arthuredelstein/tor-browser/commits/20244

Note that the last torbutton patch adds an overlay so that the our translated checkbox labels can be temporarily applied to the about:preferences page. If Mozilla uplifts these to Firefox and translates the labels themselves, then we won't need this overlay any more.

comment:3 Changed 2 years ago by arthuredelstein

Keywords: tbb-usability TorBrowserTeam201610R added

comment:4 in reply to:  2 Changed 2 years ago by bugzilla

Replying to arthuredelstein:

I've written patches for this, because it's going to make the problems I encountered in #18093 much easier to solve.

yay :)

There are 6 patches here (in which I remove checkboxes one by one and then clean up the UI):
https://github.com/arthuredelstein/torbutton/commits/20244

And rename "Privacy and Security Settings..." menu item?

and 2 patches here (where I add two checkboxes to the about:preferences#privacy page):
https://github.com/arthuredelstein/tor-browser/commits/20244

"Restrict third party..." has "strange" relations with FF's "Third party cookies" option, so the layout should reflect the dependencies.
And what about "Custom settings" instead of "Never"? Would it be default or both would be good (after bugfixing)?

Note that the last torbutton patch adds an overlay so that the our translated checkbox labels can be temporarily applied to the about:preferences page. If Mozilla uplifts these to Firefox and translates the labels themselves, then we won't need this overlay any more.

It's good to get this upsteamed while Mozilla is in mozilla52: behind a pref as resistFingerprinting and first-party isolation, but also Mozillians could make suggestions about naming, layout, hints, etc in the FF's UI.

Last edited 2 years ago by bugzilla (previous) (diff)

comment:5 in reply to:  2 ; Changed 2 years ago by mcs

Status: needs_reviewneeds_revision

Replying to arthuredelstein:

I've written patches for this, because it's going to make the problems I encountered in #18093 much easier to solve.

Kathy and I reviewed and ran with all of the patches. Nice work! Comments below.

There are 6 patches here (in which I remove checkboxes one by one and then clean up the UI):
https://github.com/arthuredelstein/torbutton/commits/20244

  • There is a typo in the commit message for 38f3653b89ce3790d75b8652316f7381158d0af3 ("Bug Bug").
  • You sometimes treat privacy.thirdparty.isolate as a Boolean pref, which does not seem correct.
  • When I clicked the button in about:plugins to enable plugins, no warning prompt was displayed. Possibly related, there is no default value for plugins.disable which caused this to appear on the browser console:
    16:26:17.523 NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]1 torbutton.js:2323:0
    
  • Please remove the following from src/defaults/preferences/preferences.js:
    extensions.torbutton.resist_fingerprinting
    extensions.torbutton.restrict_thirdparty
    
  • Can we reduce the height of the preferences window? I cannot remember how the height is determined, but on OSX there is now a lot of extra space below the slider. Or maybe it is just remembering an old height that I set by resizing.

and 2 patches here (where I add two checkboxes to the about:preferences#privacy page):
https://github.com/arthuredelstein/tor-browser/commits/20244

Note that the last torbutton patch adds an overlay so that the our translated checkbox labels can be temporarily applied to the about:preferences page. If Mozilla uplifts these to Firefox and translates the labels themselves, then we won't need this overlay any more.

  • In the tor-browser patches, should we use the same labels as we have in src/chrome/locale/en/torbutton.dtd? Or did you intentionally make them different to verify that the overlay was working?
  • Should we include the access keys in the overlay so they can be localized?

comment:6 in reply to:  5 ; Changed 2 years ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to mcs:

Replying to arthuredelstein:

I've written patches for this, because it's going to make the problems I encountered in #18093 much easier to solve.

Kathy and I reviewed and ran with all of the patches. Nice work! Comments below.

Thanks for the review!

  • There is a typo in the commit message for 38f3653b89ce3790d75b8652316f7381158d0af3 ("Bug Bug").

Fixed.

  • You sometimes treat privacy.thirdparty.isolate as a Boolean pref, which does not seem correct.

Oops, good catch. Fixed.

  • When I clicked the button in about:plugins to enable plugins, no warning prompt was displayed. Possibly related, there is no default value for plugins.disable which caused this to appear on the browser console:
    16:26:17.523 NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]1 torbutton.js:2323:0
    

Turns out I had consistently mistyped "plugin.disable" as "plugins.disable". Fixed.

  • Please remove the following from src/defaults/preferences/preferences.js:
    extensions.torbutton.resist_fingerprinting
    extensions.torbutton.restrict_thirdparty
    

Done. I have now also removed "extensions.torbutton.block_disk" in patch 20244.1 and use the "browser.privatebrowsing.autostart" pref instead.

  • Can we reduce the height of the preferences window? I cannot remember how the height is determined, but on OSX there is now a lot of extra space below the slider. Or maybe it is just remembering an old height that I set by resizing.

I added maxwidth and maxheight properties to stop it from getting too big.

  • In the tor-browser patches, should we use the same labels as we have in src/chrome/locale/en/torbutton.dtd? Or did you intentionally make them different to verify that the overlay was working?

I did, but now I have made them the same.

  • Should we include the access keys in the overlay so they can be localized?

Added.

Here are the new versions:
https://github.com/arthuredelstein/torbutton/commits/20244+1
https://github.com/arthuredelstein/tor-browser/commits/20244+1

comment:7 in reply to:  6 ; Changed 2 years ago by mcs

Replying to arthuredelstein:

Here are the new versions:
https://github.com/arthuredelstein/torbutton/commits/20244+1
https://github.com/arthuredelstein/tor-browser/commits/20244+1

Very nice.
My only comment is that the commit message for d1d6bdee6fa52d33ac3b004af1df2d472545b37a still mentions plugins.disable.

comment:8 in reply to:  7 Changed 2 years ago by arthuredelstein

Replying to mcs:

Very nice.
My only comment is that the commit message for d1d6bdee6fa52d33ac3b004af1df2d472545b37a still mentions plugins.disable.

Ack. Here is the fix:
https://github.com/arthuredelstein/torbutton/commits/20244+2
https://github.com/arthuredelstein/tor-browser/commits/20244+1
(Nothing changed in tor-browser branch.)

comment:9 Changed 2 years ago by gk

First notes:

You did

-    if (!this.prefs.getBoolPref("extensions.torbutton.block_disk")) {
+    if (!this.prefs.getBoolPref("browser.privatebrowsing.autostart")) {

once in cookie-jar-selector.js. Any reason you left the other three extensions.torbutton.block_disk occurrences?

Nit: We don't have "Privacy and Security Settings" anymore on the preferences pane. I guess we can omit the comments now.

comment:10 Changed 2 years ago by gk

There is still one plugins.disable in the commit message of e79d9bebfb396c3ca3d48edc930e4f213c88ccc1.

comment:11 Changed 2 years ago by gk

It seems you need to remove torbutton.context_menu.networksettings.key in all non-en-US locales as well to make sure it gets the new value N assigned?

comment:12 Changed 2 years ago by gk

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201610R removed
Status: needs_reviewneeds_revision

All the other things look good to me, nice! I've not tested the patches, yet, though.

comment:13 in reply to:  12 Changed 2 years ago by arthuredelstein

Replying to gk:

All the other things look good to me, nice! I've not tested the patches, yet, though.

Thanks for the review, Georg! Here's the new version fixing the problems you found:

https://github.com/arthuredelstein/torbutton/commits/20244+3
​​https://github.com/arthuredelstein/tor-browser/commits/20244+1
(Again nothing changed in the tor-browser branch.)

comment:14 Changed 2 years ago by arthuredelstein

Status: needs_revisionneeds_review

comment:15 Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Alright, that got fixed on tor-browser-45.4.0esr-6.5-1 with commits

1b92582f7ef5bec693b01bc78672122add405834
e5c3cd25ed459765a91d1b364151c46afaca36c0

and on torbutton master with commits

6d785fa1bf95f997a95b867538ba3353d9a76a58
aa05941ee0144026fb843b6f5a2098aed5e17b46
2978978e64fbc9164185564a19d56d5fea0b25d8
ee88e783a98b22e743b943cc144cb6b79c70706e
ea0d6dc4ebc59c594162202e142549356075a981
7f16ada703d07b1b0140eaf12dd3e93864c14568

comment:16 Changed 19 months ago by arthuredelstein

Keywords: tbb-fingerprinting added

comment:17 Changed 10 months ago by arthuredelstein

Keywords: tbb-no-uplift added
Note: See TracTickets for help on using tickets.