Closed Bug 419596 Opened 16 years ago Closed 16 years ago

add third party cookie blocking option to cookie pref UI

Categories

(Firefox :: Settings UI, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta5

People

(Reporter: dwitte, Assigned: dwitte)

Details

(Keywords: late-l10n, user-doc-complete)

Attachments

(3 files, 3 obsolete files)

per bug 417800 comment 7 and bug 417800 comment 11, it'd be nice to add this feature back. it's actually quite effective at blocking unwanted cookies (including from iframes and redirects within a page); in fact, it can be too effective since it breaks some sites.

while it's never fun to have an option that breaks things, i think there are two good arguments for it:

1) users who do want the additional strictness will feel better having the option there, and this reflects positively on our product - given that IE and Safari have third party blocking by default (though their implementations are quite toothless);

2) users who care enough to set it can probably also figure out how to use whitelisting for sites they need to work. (it would be super awesome to give the whitelisting feature some UX love as a followup to this, so that it's more discoverable and usable.)
-> me for b5
Assignee: nobody → dwitte
Target Milestone: Firefox 3 → Firefox 3 beta5
Attached patch patch v1 (obsolete) — Splinter Review
alright! this implements UI like so:
[x] Accept cookies from sites
    [x] for the originating site only

most of this is copy/pasted from firefox 1.5 code, when we still had this option in the prefpanel. (mapping a tristate pref to two checkboxes is a bit hackish - see _acceptOriginatingWasChecked in the js - but is probably best UI-wise.) i like the wording we used then, so i kept it.

i went with a simple indented checkbox, rather than the indented vbox container we used in 1.5. i also left the "Show cookies..."  button in the "Keep until:" hbox (ie on the third line).

firefox 1.5 reference:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/locales/en-US/chrome/browser/preferences/privacy.dtd&rev=1.9&cvsroot=/cvsroot
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/preferences/privacy.xul&rev=1.11&cvsroot=/cvsroot&mark=221-245#206
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Fbrowser%2Fcomponents%2Fpreferences%2Fprivacy.js&rev=1.11&cvsroot=%2Fcvsroot&mark=101-135#101

i'm going to split out the string change in a separate patch, to see about landing it separately for the upcoming freeze.
Attachment #307678 - Flags: ui-review?
Attachment #307678 - Flags: review?(gavin.sharp)
Attachment #307678 - Flags: ui-review? → ui-review?(beltzner)
Attached patch patch v1, string only (obsolete) — Splinter Review
just the string change. (if i need ui-r or somesuch here too, please speak up!)
Attachment #307680 - Flags: review?(gavin.sharp)
Attached image prefpanel screenshot
by way of testing: i've verified the pref is twiddled appropriately for the various combinations, that things get disabled and enabled properly, and that _acceptOriginatingWasChecked correctly remembers the checkbox state while the panel's open.
(In reply to comment #3)
> Created an attachment (id=307680) [details]
> patch v1, string only
> 
> just the string change. (if i need ui-r or somesuch here too, please speak up!)

In principle I can ui-r sec-related stuff, but this is something that beltzner has been rotating on a great deal, and yes, introducing the strings will require ui-r (and explicit patch approval).  I know that theoretically he could just ui-r the final patch, but then if he wanted changes made, we'd be post-freeze.

Comment on attachment 307680 [details] [diff] [review]
patch v1, string only

ok - thanks for the heads up!
Attachment #307680 - Flags: ui-review?(beltzner)
Comment on attachment 307678 [details] [diff] [review]
patch v1

I'm not sure we want to keep the sentence structure thing, and to get around the fact that we don't actually actively block third-party cookies, I'd suggest that the actual UI look like this:

[x] Accept cookies from sites
    [x] Accept third-party cookies

This means a string change to your patch, as well as an inversion of the UI such that when checked, the value of network.cookie.cookieBehavior is 0, and when unchecked it's 1.

(If you make it do that, you don't need a follow-on ui-r)
Attachment #307678 - Flags: ui-review?(beltzner) → ui-review-
Attachment #307680 - Flags: ui-review?(beltzner)
Attachment #307680 - Flags: review?(gavin.sharp)
Attachment #307678 - Flags: review?(gavin.sharp)
Attached patch patch v2 (obsolete) — Splinter Review
as requested. (note, the checkbox will now default to true.)
Attachment #307678 - Attachment is obsolete: true
Attachment #307680 - Attachment is obsolete: true
Attachment #307854 - Flags: ui-review+
Attachment #307854 - Flags: review?(gavin.sharp)
string change for freeze.
Attachment #307855 - Flags: ui-review+
Attachment #307855 - Flags: review?(gavin.sharp)
see also bug 421494 for more better.
Attachment #307855 - Flags: review?(gavin.sharp) → review+
Comment on attachment 307855 [details] [diff] [review]
patch v2, string change (checked in)

requesting approval on just the string change, for landing tonight.
Attachment #307855 - Flags: approval1.9?
Comment on attachment 307855 [details] [diff] [review]
patch v2, string change (checked in)

>Index: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
>+<!ENTITY  acceptThirdParty.label        "Accept third-party cookies">
>+<!ENTITY  acceptThirdParty.accesskey    "g">
>+

'g' is not a very intuitive access key for "Accept third-party cookies" - are all the other letters taken, or is this hold-over from the old string?
(In reply to comment #13)
> 'g' is not a very intuitive access key for "Accept third-party cookies" - are
> all the other letters taken, or is this hold-over from the old string?

a holdover - indeed, with the new string, 'p' seems better, i'll do that on checkin. ('t' is unfortunately taken!)
Comment on attachment 307855 [details] [diff] [review]
patch v2, string change (checked in)

a1.9=beltzner
Attachment #307855 - Flags: approval1.9? → approval1.9+
Comment on attachment 307855 [details] [diff] [review]
patch v2, string change (checked in)

landed, with an accesskey of "p".
Attachment #307855 - Attachment description: patch v2, string change → patch v2, string change (checked in)
Keywords: late-l10n
Flags: blocking-firefox3+
Priority: -- → P1
Attached patch patch v3Splinter Review
updated per discussion with gavin - drop the variable to store 'third party' checkbox state, and just check it whenever 'accept cookies' is checked.
Attachment #307854 - Attachment is obsolete: true
Attachment #308509 - Flags: ui-review+
Attachment #308509 - Flags: review?(gavin.sharp)
Attachment #307854 - Flags: review?(gavin.sharp)
Attachment #308509 - Flags: review?(gavin.sharp) → review+
Checking in browser/components/preferences/privacy.js;
/cvsroot/mozilla/browser/components/preferences/privacy.js,v  <--  privacy.js
new revision: 1.26; previous revision: 1.25
done
Checking in browser/components/preferences/privacy.xul;
/cvsroot/mozilla/browser/components/preferences/privacy.xul,v  <--  privacy.xul
new revision: 1.27; previous revision: 1.26
done
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Thanks for the link Steffen. :-)
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050810 Minefield/3.0pre

Verified.  Other platforms need verification as well.
VERIFIED Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0
You need to log in before you can comment on or make changes to this bug.