Closed Bug 205541 Opened 17 years ago Closed 17 years ago

Selecting "Unblock Cookies..." while on a site that you'd previously blocked allows the site to set cookies rather than just removing the block

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: porter, Assigned: mconnor)

References

Details

(Whiteboard: checkwin checklinux)

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030507
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030507

It seems to me that "unblock" should not equal "allow" in the Tools > Cookie
Manager menu. However, if you're on a site for which you've blocked cookies, and
you choose "Unblock Cookies from this Site" it doesn't just remove the block
entry for that site from the Cookie Manager. Instead it changes the block entry
into an allow entry.

Maybe what is needed in the menu are boolean-state items, like the ones under
the View > Show/Hide menu, which would allow you to toggle the state of both
"Block Cookies..." and "Allow Cookies..." The true states would need to be
mutually exclusive, of course, but both could be false indicating no entry for
that site on the Cookie Sites list at all.

Reproducible: Always

Steps to Reproduce:
1. Block cookies for a site.
2. Open Cookie Manager and view site's entry in the Cookie Sites list.
3. Select "Unblock Cookies..." from the Tools > Cookie Manager menu.
4. Look at the Cookie Sites entry in the Cookie Manager again.

Actual Results:  
Cookie Sites entry is set to "site can set cookies"

Expected Results:  
Cookie Sites entry should simply be removed.
Bumped this off mvl in irc for some feedback. The main problem here is when you
individually allow or block cookies (ask everytime), and then remember this
decision. When you then unblock (or block) from the site, you usually will not
want this decision remembered, but will rather want to have it changed temporarily.
When whitelisting goes in, the current behaviour will make more sense (though
you might still want some sort of toggle like the reporter proposes), but for
now this is definitely not optimal. Either we need to 
1) change the menu entry to say "unblock and allow" to at least take away the
confusion, or, 
2) add a manu entry, so we get three: "block", "unblock" and "unblock and allow"
(maybe "unblock and remember") - which will become four when whitelisting goes
in, or,
3) (better for now IMO) we need to change the behaviour of the menu entry back
to what it was, just completely removing the listing of the site, so that it
goes back to the 'neutral' state.

In any case, to remove confusion I think it would be good if one of these three
solutions could go in before 1.4 - then afterwards with cookie whitelisting we
can figure out what would be the best solution. (The more I think about it, the
better I like the pure toggle of checkmarks before "allow" and "block", although
of course they'd need to be mutually exclusive, and things might get problematic
with different domain-levels.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
I think this is a regression. Mozilla worked this way up through version 1.3.
Verified with build 20030312 (1.3 final). I haven't tried the builds in between
to isolated when it changed but it may have occured at the same time the dialog
box was reworked.
I think the suggestion in comment 0 sounds nice. It will still work when
whitelisting becomes possible.
Or, if it is too confusing, have three options: allow, block, default. (with one
checkmark)

Sander: There currently is no such thing as "block without remembering". It will
always be saved. What is possible is to remove the host from cookperm.txt, but
that will be saved too.

jag: do you have an opinion here?
Assignee: darin → mvl
Okay, we could do this as a radio-style submenu a la Text Size:

 º Allow Cookies From This Site
   Use Default Cookie Settings
   Block Cookies From This Site

This would both show what the current permissions are, along with allowing a
clear choice to change the behaviour.  Thoughts?
-> me
Assignee: mvl → mpconnor
Attached patch patch (obsolete) — Splinter Review
First off, this little bit of UI also applies to image permissions, so I made
the same changes to that submenu as well, to preserve the consistency.	I
decided I liked the current disabling of options (based on current permission)
well enough, I simply added the "Use Default" option to remove permissions, and
changed Unblock to Allow.

And since I was in the neighbourhood, I made the alerts into proper
promptService alerts.
Comment on attachment 134231 [details] [diff] [review]
patch

neil, this should a) make this UI a little more intuitive and b) clean up the
dialogs a bit.
Attachment #134231 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #134231 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch patch with proper accesskeys (obsolete) — Splinter Review
Attachment #134231 - Attachment is obsolete: true
Attachment #134232 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 134232 [details] [diff] [review]
patch with proper accesskeys

Bah, I wish I'd never looked at cookieNavigatorOverlay.xul - I wonder who
really owns this comment:
"for some unexplainable reason, CheckForImage() keeps getting called repeatedly
as we mouse over the task menu."
Also this should never appear in an overlay:
<menupopup id="taskPopup" onpopupshowing="CheckForVisibility()">
Explanations available on request.

Anyway, they way the menus disable looks odd... I'd really appreciate it if you
could switch it to a radiobutton style (of course you'll need to check that the
user doesn't select the current setting...)

>+        case "cookieDefault":

>+        case "imagesDefault":
None of the other case names have an s in them :-/
Attached patch patch v2 (radio-style) (obsolete) — Splinter Review
Attachment #134232 - Attachment is obsolete: true
Attachment #134275 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #134232 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 134275 [details] [diff] [review]
patch v2 (radio-style)

>+      document.getElementById("UseCookiesDefault").setAttribute("checked",blocked == nsIPermissionManager.UNKNOWN_ACTION);
Space after comma.

>+          var perm = nsIPermissionManager.ALLOW_ACTION;
>+          if (perm == permissionmanager.testPermission(uri, "cookie"))
>+            break;
>+          permissionmanager.add(uri, "cookie", perm);
I wonder why my build didn't complain about the redeclaration of var perm :-/
I'm not even convinced (but open to persuasion) that it's worth having as a
separate variable.

>+          promptService.alert(window,element.getAttribute("title"),
>+                              element.getAttribute("msg"));
Space after comma.

>         case "imageAllow":
>           permissionmanager.add(uri, "image", nsIPermissionManager.ALLOW_ACTION);
You forgot to check the existing permission here.

>         case "imageBlock":
>-          permissionmanager.add(uri, "image", nsIPermissionManager.DENY_ACTION);
>+          var perm = nsIPermissionManager.DENY_ACTION;
>+          if (perm == permissionmanager.testPermission(uri, "image"))
>+            break;
A thought: if you return; here...

>+          permissionmanager.add(uri, "image", perm);
>           element = document.getElementById("BlockImages");
>-          alert(element.getAttribute("msg"));
>+          promptService.alert(window,element.getAttribute("title"),
>+                              element.getAttribute("msg"));
>           break;
Leave this as a break...

>         default:
And return here...

>       }
Then you can alert here...

>+<!ENTITY cookieCookiesDefaultCmd.label "Use Default Cookie Permissions">
>+<!ENTITY cookieCookiesDefaultCmd.accesskey "u">
"U", otherwise the u of Default will get underlined. (Double-check the others
too.)
Attachment #134275 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch v3, with suggested changes (obsolete) — Splinter Review
the perm var doesn't really accomplish much in retrospect, its gone now.  Also,
I changed the accesskeys to all uppercase, just to always force it since that
is obviously the intended behaviour.
Attachment #134275 - Attachment is obsolete: true
Attachment #134296 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 134296 [details] [diff] [review]
v3, with suggested changes

>+      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                                    .getService(Components.interfaces.nsIPromptService);
>       switch (action) {

>+          return;
>       }
Just spotted that you could have moved the promptService variable here.
>+      promptService.alert(window,element.getAttribute("title"), element.getAttribute("msg"));
Attachment #134296 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attached patch final patch (obsolete) — Splinter Review
with the promptService var moved, no other changes, carrying over r+
Attachment #134296 - Attachment is obsolete: true
Attachment #134312 - Flags: superreview?(alecf)
Attachment #134312 - Flags: review+
Comment on attachment 134312 [details] [diff] [review]
final patch

looks fine, but I like that we had a common function before (enableElement) to
do the document.getElementById() and so forth:
-      enableElement("BlockCookies", blocked !=
nsIPermissionManager.DENY_ACTION);
+      document.getElementById("UseCookiesDefault").setAttribute("checked",
blocked == nsIPermissionManager.UNKNOWN_ACTION);

sr=alecf but extra points if you merge that stuff into a new common function.

(Also, the wording for the by-default labels is a little awkward.. not that I
can think of anything better!)
Attachment #134312 - Flags: superreview?(alecf) → superreview+
Attachment #134312 - Attachment is obsolete: true
Comment on attachment 134364 [details] [diff] [review]
ooh! ooh! extra points!

so i'm ok with this as it stands, but we do need to be aware of this problem:

>+        case "imageDefault":
>+          if (permissionmanager.testPermission(uri, "image") == nsIPermissionManager.UNKNOWN_ACTION)
>+            return;
>+          permissionmanager.remove(uri.hostPort, "image");

using uri.hostPort directly here will work in most cases, but not all.
nsIPermissionManager::Add() and ::TestPermission() take the uri directly, and
do some massaging to it in some cases. Remove() will be expecting that massaged
string. unfortunately, it's hard to change Remove() to take the uri directly,
because it's handy to e.g. get an enumerator from the permissionmanager, find
an nsIPermission you want to remove, and then call Remove() using the relevant
host string obtained from said nsIPermission.

i'm not sure how we could solve it. maybe we could just provide a conversion
helper on nsIPermissionManager to convert a URI to a key. then we could enforce
the distinction between the URI and the key a little better perhaps?

mvl?
checked in

file:/// urls can't be removed this way, but I think that's a safe enough case
for now, but I'll leave this open until mvl responds or we spin that off into a
new bug
I'd suggest to file a new bug.
The UI is ok, it's the backend that needs some work. The problem is that you
can't find out what to remove. For example, if you block domain.com from setting
cookies, TestPermissions will return blocked for sub.domain.com. But calling
Remove with sub.domain.com won't do anything. We need to decide what should happen.
Bug 224327 filed.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
*** Bug 227294 has been marked as a duplicate of this bug. ***
*** Bug 230559 has been marked as a duplicate of this bug. ***
>>+          promptService.alert(window,element.getAttribute("title"),
>>+                              element.getAttribute("msg"));
>Space after comma.
Bah, this never got fixed, did it :-(
V: 
Mozilla 1.6f, Mac OS X
Status: RESOLVED → VERIFIED
QA Contact: cookieqa → benc
Whiteboard: checkwin checklinux
You need to log in before you can comment on or make changes to this bug.