Closed
Bug 205541
Opened 21 years ago
Closed 21 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)
Core
Networking: Cookies
Tracking
()
VERIFIED
FIXED
People
(Reporter: porter, Assigned: mconnor)
References
Details
(Whiteboard: checkwin checklinux)
Attachments
(1 file, 5 obsolete files)
12.61 KB,
patch
|
Details | Diff | Splinter Review |
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
Comment 2•21 years ago
|
||
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.
Comment 3•21 years ago
|
||
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
Assignee | ||
Comment 4•21 years ago
|
||
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?
Assignee | ||
Comment 6•21 years ago
|
||
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.
Assignee | ||
Comment 7•21 years ago
|
||
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)
Assignee | ||
Updated•21 years ago
|
Attachment #134231 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 8•21 years ago
|
||
Attachment #134231 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #134232 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 9•21 years ago
|
||
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 :-/
Assignee | ||
Comment 10•21 years ago
|
||
Attachment #134232 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #134275 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•21 years ago
|
Attachment #134232 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 11•21 years ago
|
||
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-
Assignee | ||
Comment 12•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #134275 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #134296 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 13•21 years ago
|
||
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+
Assignee | ||
Comment 14•21 years ago
|
||
with the promptService var moved, no other changes, carrying over r+
Attachment #134296 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #134312 -
Flags: superreview?(alecf)
Attachment #134312 -
Flags: review+
Comment 15•21 years ago
|
||
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+
Assignee | ||
Comment 16•21 years ago
|
||
Attachment #134312 -
Attachment is obsolete: true
Comment 17•21 years ago
|
||
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?
Assignee | ||
Comment 18•21 years ago
|
||
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
Comment 19•21 years ago
|
||
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.
Assignee | ||
Comment 20•21 years ago
|
||
Bug 224327 filed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 21•21 years ago
|
||
*** Bug 227294 has been marked as a duplicate of this bug. ***
Comment 22•21 years ago
|
||
*** Bug 230559 has been marked as a duplicate of this bug. ***
Comment 23•21 years ago
|
||
>>+ promptService.alert(window,element.getAttribute("title"),
>>+ element.getAttribute("msg"));
>Space after comma.
Bah, this never got fixed, did it :-(
Comment 24•21 years ago
|
||
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.
Description
•