Last Comment Bug 845353 - New 3rd-party cookie restriction to visited websites is default but not an option in Cookies pref pane
: New 3rd-party cookie restriction to visited websites is default but not an op...
Status: RESOLVED FIXED
: regression, relnote
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: seamonkey2.19
Assigned To: rsx11m
:
Mentors:
Depends on:
Blocks: 844823
  Show dependency treegraph
 
Reported: 2013-02-26 08:04 PST by rsx11m
Modified: 2013-05-14 12:43 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
unaffected
fixed


Attachments
Proposed patch (6.45 KB, patch)
2013-02-26 16:20 PST, rsx11m
neil: ui‑review+
Details | Diff | Review
Proposed patch (v2) (8.17 KB, patch)
2013-02-27 15:32 PST, rsx11m
rsx11m.pub: ui‑review+
Details | Diff | Review
Alternative patch (v2b) (8.21 KB, patch)
2013-02-27 15:54 PST, rsx11m
rsx11m.pub: ui‑review+
Details | Diff | Review
Proposed patch (v3) (8.21 KB, patch)
2013-02-28 06:03 PST, rsx11m
iann_bugzilla: review+
rsx11m.pub: ui‑review+
Details | Diff | Review
Final patch (v4) [checked in, comment #18] (8.52 KB, patch)
2013-03-02 13:54 PST, rsx11m
rsx11m.pub: review+
rsx11m.pub: ui‑review+
Details | Diff | Review
Follow-up patch for Help [comment #23] (1.60 KB, patch)
2013-03-21 12:05 PDT, rsx11m
iann_bugzilla: review+
Details | Diff | Review

Description rsx11m 2013-02-26 08:04:28 PST
Bug 818340 "Block cookies from sites I haven't visited" introduced a new option for network.cookie.cookieBehavior to allow 3rd-party cookies from sites only which have been previously visited as primary sites and were allowed to store cookies already.

This option has been made the default on trunk and breaks the Cookies preference pane UI, given that it isn't present yet. The radiobuttons will default to "Block Cookies" (which is wrong) when opened, but the cookie retention options remain enabled (given that their visibility is dependent on cookieBehavior != 2).

Just adding a radiobutton for cookieBehavior == 3 fixes the problem. I will also update the help files respectively.

There is currently some discussion in bug 818340 whether or not that's a good default and if there are better ones, but for now it is (and even if Firefox changes it back to some other set of defaults, it's still a good idea to make this new option available in the pref pane).
Comment 1 rsx11m 2013-02-26 16:20:31 PST
Created attachment 718759 [details] [diff] [review]
Proposed patch

This patch adds a new radiobutton for cookieBehavior == 3 with a new label acc3rdPartyVisited; in addition, it adds "(no third-party cookies)" to the old accOrgCookiesRadio label (which becomes accNo3rdPartyCookies). This ensures that the term "third-party cookie" is clear, in addition to the "originating websites" term of the old label. Given that a few (three) labels had the "Radio" at the end but most of them didn't, I've changed accAllCookiesRadio to just accAllCookies in the process. I've also reversed the order of entities in the DTD file to match the XUL file as they were upside-down.

The last paragraph of the "What Are Third-Party Cookies?" section has been extended and received a link to the preference pane. The "Privacy & Security Preferences - Cookies" section has been updated for the new option, and "foreign cookie" replaced with the term "third-party cookie" that is used elsewhere (with "foreign" retained in parentheses).
Comment 2 neil@parkwaycc.co.uk 2013-02-27 13:53:27 PST
Hmm, Page Info and Data Manager are getting out of date, they don't even support ACCESS_ALLOW_FIRST_PARTY_ONLY yet let alone ACCESS_LIMIT_THIRD_PARTY.
Comment 3 neil@parkwaycc.co.uk 2013-02-27 14:15:28 PST
Comment on attachment 718759 [details] [diff] [review]
Proposed patch

>+<!ENTITY acc3rdPartyVisited.label       "Allow third-party cookies for previously visited websites only">
>+<!ENTITY acc3rdPartyVisited.accesskey   "r">
"v" or maybe "w" please.

IanN's the help owner, so he has the last word, but I wanted to make some suggestions anyway.

>+<p>If you want, you can <a href="using_priv_help.xhtml#cookies">adjust your
>+  cookie preferences</a> so that websites can store ordinary cookies but not
>+  third-party ones, or only for those sites which you have been visiting
>+  directly before.</p>
Nit: "which you have been visiting" is bad grammar, you want "that you have visited".
Instead of "visited directly before", you could perhaps write this as "already visited" or "previously visited".

-  <li><strong>Allow all cookies</strong>: This is the default option. Select
You lost "This is the default option."

>+  <li><strong>Allow third-party cookies for previously visited websites
>+    only</strong>: Select this option if you want to accept or return
>+    third-party cookies only for websites which have stored cookies
>+    previously already when you directly visited them.</li>
Try "websites that stored cookies when you visited them previously." Or maybe just "websites that have already stored cookies." (Or that one could be "previously".)
Comment 4 rsx11m 2013-02-27 14:42:11 PST
(In reply to neil@parkwaycc.co.uk from comment #3)
> >+<!ENTITY acc3rdPartyVisited.accesskey   "r">
> "v" or maybe "w" please.

I thought "r" as in "3rd-party" but we have that term above already. I'll make it "v" as in "visited" which is a bit hidden at the end but the best metaphor.

> IanN's the help owner, so he has the last word, but I wanted to make some
> suggestions anyway.

I'm happy to take them.  :-)

> Nit: "which you have been visiting" is bad grammar, you want "that you have
> visited".

Ok.

> Instead of "visited directly before", you could perhaps write this as
> "already visited" or "previously visited".

The latter would be the best fit with the respective label.

> -  <li><strong>Allow all cookies</strong>: This is the default option. Select
> You lost "This is the default option."

On purpose. Was it Stefan or someone else, but I had in mind that we don't want the default explicitly stated as they may change, as in this case?

I'll leave it out for now unless Ian would like to see it back.

> >+    previously already when you directly visited them.</li>
> Try "websites that stored cookies when you visited them previously." Or
> maybe just "websites that have already stored cookies." (Or that one could
> be "previously".)

I'm not sure if omitting the "directly" or some other phrase is good. The point here is to distinguish between visits which are intended and those which occur simply because another website includes content or tracking widgets from that site.
Comment 5 rsx11m 2013-02-27 14:52:55 PST
(In reply to neil@parkwaycc.co.uk from comment #2)
> Hmm, Page Info and Data Manager are getting out of date, they don't even
> support ACCESS_ALLOW_FIRST_PARTY_ONLY yet let alone ACCESS_LIMIT_THIRD_PARTY.

Are they supposed to? The Firefox Page Info window shows selection only for "Allow", "Allow for Session", and "Block" too. I don't think that there are individual permissions for 3rd-party cookies, only in the global preference.
Comment 6 rsx11m 2013-02-27 15:32:50 PST
Created attachment 719211 [details] [diff] [review]
Proposed patch (v2)

All items in Neil's comment #3 addressed, carrying forward the ui-r+:

(In reply to rsx11m from comment #4)
> (In reply to neil@parkwaycc.co.uk from comment #3)
> > You lost "This is the default option."
> On purpose. Was it Stefan or someone else, but I had in mind that we don't
> want the default explicitly stated as they may change, as in this case?

There were actually two more instances where the old default was explicitly mentioned in the help text, I've removed/rephrased those. If you want to have it back, I'd suggest to only mention the default in the actual preferences description, thus in case of another change it'll only have to be changed once.

> I'm not sure if omitting the "directly" or some other phrase is good. The
> point here is to distinguish between visits which are intended and those
> which occur simply because another website includes content or tracking
> widgets from that site.

I've rephrased this part but kept the "directly" for that distinction.
Comment 7 rsx11m 2013-02-27 15:54:43 PST
Created attachment 719223 [details] [diff] [review]
Alternative patch (v2b)

Ok, so in the same file for the description of the Images preference pane, there is a sentence "(This option is selected by default.)" added at the end of the option's description. I've followed this example and added the same to the new (now default) option in the Cookies preferences.

That's the only change relative to attachment 719211 [details] [diff] [review] thus you can pick on version or the other (pending any other revisions).
Comment 8 neil@parkwaycc.co.uk 2013-02-27 16:04:24 PST
(In reply to rsx11m from comment #4)
> (In reply to comment #3)
> > >+<!ENTITY acc3rdPartyVisited.accesskey   "r">
> > "v" or maybe "w" please.
> 
> I thought "r" as in "3rd-party" but we have that term above already. I'll
> make it "v" as in "visited" which is a bit hidden at the end but the best
> metaphor.
Also, the underline on "r" is easily missed.

> > >+    previously already when you directly visited them.</li>
> > Try "websites that stored cookies when you visited them previously." Or
> > maybe just "websites that have already stored cookies." (Or that one could
> > be "previously".)
> 
> I'm not sure if omitting the "directly" or some other phrase is good. The
> point here is to distinguish between visits which are intended and those
> which occur simply because another website includes content or tracking
> widgets from that site.

I don't think "directly" is the right word for this. I suppose "specifically" would work, but my thinking was that "you visited" sounds sufficiently deliberate.

(In reply to rsx11m from comment #5)
> (In reply to from comment #2)
> > Hmm, Page Info and Data Manager are getting out of date, they don't even
> > support ACCESS_ALLOW_FIRST_PARTY_ONLY yet let alone ACCESS_LIMIT_THIRD_PARTY.
> 
> Are they supposed to? The Firefox Page Info window shows selection only for
> "Allow", "Allow for Session", and "Block" too. I don't think that there are
> individual permissions for 3rd-party cookies, only in the global preference.

They exist, that's how I discovered them ;-)
Comment 9 rsx11m 2013-02-27 16:12:44 PST
(In reply to neil@parkwaycc.co.uk from comment #8)
> Also, the underline on "r" is easily missed.

Unfortunately it's latching on the the "r" in "previously" not "visited", but it's better anyway.

> I don't think "directly" is the right word for this. I suppose
> "specifically" would work,

"explicitly"? "purposefully"? "intentionally"?

> but my thinking was that "you visited" sounds sufficiently deliberate.

Well, I'll wait for Ian's opinion whether to keep or omit it, or to replace it with something different.

> They exist, that's how I discovered them ;-)

Hmm... per-site definitions for that would be actually quite useful.  :-)
Comment 10 rsx11m 2013-02-27 16:14:00 PST
(In reply to rsx11m from comment #9)
> Unfortunately it's latching on the the "r" in "previously" not "visited",

s/"r"/"v"/ of course...
Comment 11 neil@parkwaycc.co.uk 2013-02-28 01:22:00 PST
(In reply to rsx11m from comment #9)
> (In reply to neil@parkwaycc.co.uk from comment #8)
> > I don't think "directly" is the right word for this. I suppose
> > "specifically" would work,
> 
> "explicitly"? "purposefully"? "intentionally"?

"explicitly visited websites" works for me, thanks!
Comment 12 rsx11m 2013-02-28 06:03:29 PST
Created attachment 719447 [details] [diff] [review]
Proposed patch (v3)

Sounds good to me as well. This version says now "explicitly visited" and mentions in parentheses that it is the default (if Firefox changes it again prior to the next merge it would be an easy fix in a follow-up patch).
Comment 13 rsx11m 2013-02-28 07:23:41 PST
(In reply to neil@parkwaycc.co.uk from comment #2)
> Hmm, Page Info and Data Manager are getting out of date, they don't even
> support ACCESS_ALLOW_FIRST_PARTY_ONLY yet let alone ACCESS_LIMIT_THIRD_PARTY.

Filed as bug 846326.
Comment 14 rsx11m 2013-03-02 13:25:02 PST
Comment on attachment 719447 [details] [diff] [review]
Proposed patch (v3)

Ian, I'm wondering: On one hand, we just briefly state for "Block all"

>   <li><strong>Block cookies</strong>: Select this option to refuse all
>     cookies.</li>

but then for "Allow all" more specifically
 
>+  <li><strong>Allow all cookies</strong>: Select this option to permit all
>+    websites not explicitly blocked to set cookies on your computer.</li>

Thus, shouldn't the first description read rather something like

>+  <li><strong>Block cookies</strong>: Select this option to refuse all
>+    cookies from websites not explicitly allowed to set cookies.</li>

?
Comment 15 Ian Neal 2013-03-02 13:35:48 PST
Comment on attachment 719447 [details] [diff] [review]
Proposed patch (v3)


>   <li><strong>Block cookies</strong>: Select this option to refuse all
>     cookies.</li>
>   <li><strong>Allow cookies for the originating website only</strong>: Select
>     this option if you don&apos;t want to accept or return
>+    <a href="privacy_help.xhtml#what_are_third-party_cookies">third-party
>+    (foreign) cookies</a> for any websites other than the one you are actively
>+    visiting.</li>
>+  <li><strong>Allow third-party cookies for previously visited websites
>+    only</strong>: Select this option if you want to accept or return
>+    third-party cookies only for websites that stored cookies when you
You link to what third-party cookies are above, is it worth doing it again here?

>+    explicitly visited them previously. (This option is selected by
>+    default.)</li>
Easy to say "This is the default option"?

r=me with those addressed/answered.
Comment 16 Ian Neal 2013-03-02 13:46:41 PST
(In reply to rsx11m from comment #14)
> Comment on attachment 719447 [details] [diff] [review]
> Proposed patch (v3)
> 
> Ian, I'm wondering: On one hand, we just briefly state for "Block all"
> 
> >   <li><strong>Block cookies</strong>: Select this option to refuse all
> >     cookies.</li>
> 
> but then for "Allow all" more specifically
>  
> >+  <li><strong>Allow all cookies</strong>: Select this option to permit all
> >+    websites not explicitly blocked to set cookies on your computer.</li>
> 
> Thus, shouldn't the first description read rather something like
> 
> >+  <li><strong>Block cookies</strong>: Select this option to refuse all
> >+    cookies from websites not explicitly allowed to set cookies.</li>
> 
> ?

Yes, that would be better. Thanks. r=me with that change too.
Comment 17 rsx11m 2013-03-02 13:54:35 PST
Created attachment 720330 [details] [diff] [review]
Final patch (v4) [checked in, comment #18]

(In reply to rsx11m from comment #14)
> >+  <li><strong>Block cookies</strong>: Select this option to refuse all
> >+    cookies from websites not explicitly allowed to set cookies.</li>

Done.

(In reply to Ian Neal from comment #15)
> You link to what third-party cookies are above, is it worth doing it again here?

Done.

> >+    explicitly visited them previously. (This option is selected by
> >+    default.)</li>
> Easy to say "This is the default option"?

Maybe, but I've kept the phrase consistent with the Images preferences. Thus, I'm going with the established phrase here as well (no change).
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-03-02 19:02:06 PST
https://hg.mozilla.org/comm-central/rev/cc2732963642
Comment 19 rsx11m 2013-03-02 19:16:06 PST
Thanks, I'm cancelling the tracking request as this has been fixed in time.
Comment 20 rsx11m 2013-03-15 12:35:12 PDT
Hmm, apparently it is no longer possible to set a "See also" link in a bug without forcing it to the other bug as well. Anyway, I'll keep an eye on bug 851606 handling uplifting to aurora and beta channels within the rapid-release train in case we'd need to hide the "3" option for any of those, in case it's not allowed to go to the next channel for some reason.
Comment 21 rsx11m 2013-03-21 12:05:45 PDT
Created attachment 727810 [details] [diff] [review]
Follow-up patch for Help [comment #23]

The next merge is coming up in about 10 days from now, thus it's time to think about any consequences from bug 851606. According to the discussion there, the feature shall stay enabled along with the default until FF 22.0 beta 4 is reached, to gather further feedback to which extent the new cookie default policy breaks any websites. Then, if it's decided not to ship FF 22.0 final with it, most likely just the default would be reversed to "Accept all cookies" (at least for that release).

Unless they remove the backend entirely (the bug description only says "disable" now, so that doesn't seem to be intended any more), the change in default would be the only consequence to be considered. We are fine in this regard as far as the pref-pane UI is concerned. If indeed they should backout the entire backend as well, we'd just need to remove the acc3rdPartyVisited checkbox from the XUL code (no l10n impact), but I'll cross that bridge if and when we get there (and that could be taken care of in comm-beta on a short notice).

For now, the only concern is the statement "(This option is selected by default.)" added to the "Allow third-party cookies for previously visited websites only" checkbox description in Help, as added by attachment 720330 [details] [diff] [review]. Should bug 851606 go into effect, the description would be wrong. This has l10n impact, thus any actions would need to be decided on for the upcoming merge.


As I see it, we have three options:

(1) leave the text as is, risking that we may ship a release which has a wrong statement, possibly changing it later once the topic was settled for Core which way to go;

(2) leave trunk as is but apply the patch after April 2 on comm-aurora only, thus we are safe for the 22.0 cycle and can revisit the topic in 6 weeks as necessary;

(3) apply the patch on trunk before the merge and just don't mention anything specific on which value is the default, then we don't need to worry how many times the default flips.


On a side note, no default is mentioned for the cookie retention settings below either, as far as I can tell, but we have them stated for other panes.
Comment 22 rsx11m 2013-03-24 12:01:49 PDT
Thanks Ian. Since you didn't provide any further comment, I read your r+ as going with option (3) to remove mentioning the default entirely (which was my thinking in comment #4 already in anticipation of such issues).
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-03-27 07:40:43 PDT
https://hg.mozilla.org/comm-central/rev/aa92876e67b9
Comment 24 rsx11m 2013-04-14 07:40:49 PDT
This should be added to the release notes for 2.19 beta as (at least initially, if not for the release) the default will change to the new option.
Comment 25 rsx11m 2013-05-14 12:43:49 PDT
Bug 851606 has been pushed already for comm-beta, so the change in default will *not* be effective with 2.19 beta 1 (the release notes could still mention that this option was introduced, though).

Note You need to log in before you can comment on or make changes to this bug.