Closed Bug 23508 Opened 25 years ago Closed 22 years ago

Cookie confirmation dialog should show all data

Categories

(Core :: Networking: Cookies, enhancement, P3)

enhancement

Tracking

()

VERIFIED FIXED

People

(Reporter: bugzilla, Assigned: mvl)

References

Details

(Keywords: helpwanted, Whiteboard: [p-ie/mac] [p-icab] [p-opera])

Attachments

(12 files, 10 obsolete files)

58.65 KB, image/jpeg
Details
4.69 KB, image/png
Details
8.28 KB, image/png
Details
1.82 KB, text/plain
Details
22.84 KB, patch
Details | Diff | Splinter Review
19.73 KB, image/png
Details
20.71 KB, image/png
Details
20.44 KB, image/png
Details
24.21 KB, patch
Details | Diff | Splinter Review
19.46 KB, image/png
Details
26.41 KB, patch
Details | Diff | Splinter Review
32.99 KB, patch
morse
: review+
Details | Diff | Splinter Review
When you go to a site that send a cookie and you have enabled "warn me about cookies" you'll get a Confirm box that warns you about the cookie. The Confirm box should include the cookie name, machine set, path, date, etc. Attached is a image with the three cookie dialogs from Netscape, Mozilla and IE. I really like some of the stuff from the IE dialog.
Severity: normal → enhancement
I agree. Most people have no idea what a 'cookie' means, and IE's dialog teaches its meaning to the user via their dialog. [changing severity to 'enhancement']
Assignee: shuang → german
reassign it to german for final UI clean up. Thanks for the screen shots.
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → INVALID
This issue was discussed at length about two years ago when the new cookie nag box was designed. It was decided at that time (jar was involved in the decision so I am copying him on this report) that there was too much "noise" in this box for the novice user. That is why I removed all that and simplified the nag box. Therefore closing this out as invalid.
Are you serious about this? Then what about a pref to decide whether the user wants the basic or the advanced cookie dialog? Or what about just a "Show more details" dialog in the current cookie dialog? Over 60% of the internet users use IE and I havn't heard a single complain about the cookie dialog! Most people always just leave the default setting ("accept all cookies") on. So when you decide to change this setting to "ask me every time a cookie is being set" then you're normally a pro!
This feature is very different from the old "show me each time a cookie is set." The old feature was an endless nag, that no one could IMO endure (novice, or expert!). This dialog is meant to be usable by a relative novice. The design is such that you can typically say "No" only once to each cookie-setting-site, and not be bothered very often at all. If the feature becomes an "expert only feature," then it will be interesting, but will have missed the mass market it was trying to help. To reach that audience, the basic dialog must be very simple. If folks insist on a more complex dialog, I'd argue for having a pref hidden deep within some advanced list of options (which is among the suggestions just made above). IMO, such a feature is nice... but is indeed another feature... while our list is and can grow beyond bounds. If someone wants to champion this "extra info via pref" feature from beyond the walls of Netscape... because they feel it would be cool... then I would be glad to see such things arrive in the tree. The critical point is that it must be possible to have a relatively simple dialog that won't scare or confuse folks, and it is certainly the case that the Netscape branded variant of Mozilla will be interested in supporting that variant as a default (it is currently the only sub-option for this feature). Note that the default setting for cookie acceptance will continue to be "accept all cookies." However, if folks are concerned about cookies, then this option should be a nice compromise (far from decline all).
Maybe I'm not imaginative enough, but why would you want to see the contents of the cookie anyway? 99 out of 100 times its uncomprehensible gibberish. As you say, most people have this set to "Accept all cookies" and Mozilla provides a dialog already that displays the value and other information about cookies already stored on the computer, as well as giving the user the option to remove those cookies. I seriously doubt that anyone except the most masochistic geek would be able to glean enough information from the cookie data attached to the IE dialog to form a decision on whether or not they want to reject or accept the cookie. As Jim says, this is not saying that someone cannot develop such a feature and contribute it. Its just that we will not be using our own resources to develop it when we believe the current system to be satisfactory.
Ok, then what about just marking the bug as RFE?
Would it be possible to expand the dialog to take advantage of XUL?
Status: RESOLVED → REOPENED
Status: REOPENED → RESOLVED
Closed: 25 years ago25 years ago
Resolution: INVALID → WONTFIX
gemal, you can re-open the bug as an RFE --- but, someone ultimately has to be convinced to do the actual work to make it happen. If you code, go for it. I'm re-opening and resolving this as WONTFIX --- there's nothing invalid about this request besides the fact that three people have said that they don't personally care for it.
[altering resolution to WONTFIX]
Status: RESOLVED → VERIFIED
[verified]
OS: Windows 98 → All
Hardware: PC → All
Are you allowed to do that, Eli? Verify your own resolutions?? IMO, this should be reopened and assigned to nobody@mozilla.org, with HELP WANTED. I, for one, regularly accept or deny individual cookies based on their content. If necessary, the cookie data could be shown by clicking a (persistent) disclosure triangle in the dialog, so the dialog in its default form wouldn't scare novice users.
Status: VERIFIED → REOPENED
Reopening as requested by Eli.
Assignee: german → nobody
Status: REOPENED → NEW
Summary: Cookie dialog should show cookie name, path, date, etc. → [HELP WANTED] Cookie confirmation dialog should show all data
ObComment [this annoyance is currently under discussion elsewhere]
Resolution: WONTFIX → ---
Keywords: helpwanted
Assigning all open "nobody@mozilla.org" bugs to "leger@netscape.com" to weed thru.
Assignee: nobody → leger
*** Bug 26705 has been marked as a duplicate of this bug. ***
Summary: [HELP WANTED] Cookie confirmation dialog should show all data → Cookie confirmation dialog should show all data
Moving UE/UI component bugs over to new User Interface: Design Feedback component. UE/UI component will be deleted.
Component: UE/UI → User Interface: Design Feedback
Assigning to nobody@mozilla.org, help wanted to work on.
Assignee: leger → nobody
QA Contact: elig → nobody
*** Bug 34202 has been marked as a duplicate of this bug. ***
If there was a question before, there definately *IS* a demand for this: 8. Cookies - Netscape no longer tells me the contents of a cookie! Horrors! I want to be able to pick and choose the cookies I save based on their content, eg. ad cookies vs username/password cookies. Netscape 4.x had this wonderful feature. IE5 has a similar feature, but it doesn't show as much content or in as intuitive a manner. Do I have to go to the cookie manager to remove the cookies? Why can't I do this when the cookies come in? (this was from a user's initial review of Netscape PR1 on macfixit: http:// www.macfixit.com/ultimate/Forum21/HTML/000010.html)
*** Bug 47970 has been marked as a duplicate of this bug. ***
It would be nicer if the information was in a table, but this is better than nothing. Could someone review and check in? Jim?
Assignee: nobody → jar
Component: User Interface: Design Feedback → Cookies
Keywords: patch, review
QA Contact: nobody → mpt
From the above comments (Gemal and Roskind), it was agreed that if this feature went in it would have to be controlled by a pref. However the submitted patch puts in the added information unconditionally, without testing any pref.
Reassigning to get help from non-netscape contributors. If there is a complete patch created, please re-assign to the module owner, or get permission, and land the fix. It sure seems like the folks that want this would be happy with an "invisible pref" that was set by manually editing the prefs.js. This would avoid the UI/UE issues arround the pref, and make the UI presentation of the data less critical to usability for an average user.
Assignee: jar → nobody
Better than an invisible pref would be a disclosure triangle in the dialog. This is a preliminary design: +--------------------------------------------------------+ |[] Cookie request ::::::::::::::::::::::::::::::::::::::| +--------------------------------------------------------+ | Mozilla has been asked to store a cookie (( Allow )) | | while opening the address ( Deny ) | | [http://webmail.com.foo/$INBOX ]. | | Do you want to allow this cookie? ( Stop ) | | | | [/] Remember this decision | | | | |> About this cookie | +--------------------------------------------------------+ +--------------------------------------------------------+ |[] Cookie request ::::::::::::::::::::::::::::::::::::::| +--------------------------------------------------------+ | Mozilla has been asked to store a cookie (( Allow )) | | while opening the address ( Deny ) | | [http://webmail.com.foo/$INBOX ]. | | Do you want to allow this cookie? ( Stop ) | | | | [/] Remember this decision | | __ | | \/ About this cookie | | | | Server: webmail.com.foo | | Name: user-id | | Value: morse | | Could be read by: any server in the domain .com.foo | | Would expire: 2001-12-31 | | | +--------------------------------------------------------+ If you opened/closed the `About this cookie' part for one cookie, that state would be remembered for future cookie dialogs.
Keywords: patch, review
My geek side loves the suggestion. I agree, for folks that want to see the info, the above is the nicest way to expose it mentioned so far. My fear of complications for an inocent user is fretting about any of this. Again, if we could get outside contributors to land all this stuff, that would be great. Because we're so crunched, Netscape folks won't have time to mess with UI smoothing etc at this point, so we'd still have to have a pref and let marketing and Netscape UI make a in/out call for Nav 6 after it landed in mozilla. My *guess* is that we'll branch for Nav 6 in late September, so if external contributors are hoping it will hitch along into Navigator, it needs to land RSN.
Depends on: 52778
mpt: I think the "could be read by" (or "for") field should be moved up into the always-visible portion of the dialog, and the "url requested" field should be moved down. The domain for the cookie is shorter and more useful in deciding whether to accept a cookie (for example, moderately paranoid users don't want foo.dhs.org to set *.dhs.org cookies, but won't mind foo.dhs.org cookies). "While opening the address" (or "URL requested") is vague btw... what address am I opening when I surf to altavista and get an engage or doubleclick cookie? Any ideas on how to fix that?
*** Bug 62424 has been marked as a duplicate of this bug. ***
Netscape nav triage team: based on Steve Morse's pre-triage recommendation, this is not a beta stopper.
Keywords: nsbeta1-
Another thing that could be designed would be the ability to choose whether accepting/rejecting any cookie from a site/domain, or to just accept/deny ONE cookie (as identified by name) from that site/domain. From the UI point of view, it would be just a matter of adding another button. From the infrastructure POV, I'm not sure (I'm not a coder).
The accept cookie dialog is not very useful right now, because it doesn't give the user enough information to make an informed decision. If nothing else, it should at least mention when the cookie expires. Right now the dialog makes no distinction between a cookie that lasts for one browser session and a cookie that tracks you until 2038. It would also be nice if the cookie options had an option to automatically accept only cookies that expire during this browser session.
An option to automatically accept only session cookies is bug 64336.
Not having name & value information for a cookie is a show stopper for me. I always review cookies to decide wether I want to allow/deny, the "remember this decision" is great for the doubleclicks of this world, but in some cases you want better granularity of control. E.g: site example.com wants to set a cookie for user/password, and then later on wants to set another for some arbitrary value which you don't wish to store. In this case I want to permanently remember the user/password cookie and permanently deny the arbitrary value. Doing it from the cookie alert box is how it should ahppen, with an option in the cookie dialog box to turn on expert cookie control to allow site/time/name granularity control on each cookie. You may refer to it as "masochistic geek" behaviour but internet privacy is a very high priority for a lot of people. Personally I'm going back to Communicator until it's resolved.
Lack of this feature is one of the primary reasons I don't use mozilla. (and wanting to submit an RFE for it was my sole motivation for creating this bugzilla account) I am a web developer, so not only do I not use mozilla for my surfing, the lack of good cookie information is a major impediment to the use of mozilla when developing websites. This leads to my websites not being mozilla compatible, which can only be a bad thing. There have been some good UI suggestions in this thread, that seem to address the "novice users will be overwhelmed" concerns. Also the current design creates no path for users to move from novice to expert which would be a tragedy. Thanks.
*** Bug 80649 has been marked as a duplicate of this bug. ***
My report has been marked a duplicate of this bug.
*** Bug 83761 has been marked as a duplicate of this bug. ***
*** Bug 93738 has been marked as a duplicate of this bug. ***
*** Bug 96890 has been marked as a duplicate of this bug. ***
The lack of cookie information is one of the reasons I continue to use iCab when surfing sites I'm not familiar with. Just knowing if a site is setting cookies or not doesn't me absolutely no good whatsoever. I need to know what kind of cookies they're setting before I can make an informed decision. There are ways to show this information without overwhelming a novice. (See lots of good comments above)
Blocks: 100573
once <expander/> gets into the tree, will take a look at this.
So, folks... I turned cookie warning on and what a dissapointment - i see only a nothing-containing dialog the cookie... Is it too much to add a button ("Details >>") which allows to see complete information about the cookie? And the details are show every next warning until i click "<< No details" some other time...
Depends on: 85045
I support all those people that demand more information about cookie in confirmation dialog. Do it as you want: - hidden pref - UI pref - "more details" button But people who create websites really need it to debug ther work. If you care so much about novice users hide this feature , but please *enable* it anyway.
*** Bug 115821 has been marked as a duplicate of this bug. ***
Keywords: mozilla1.0
*** Bug 116614 has been marked as a duplicate of this bug. ***
Attached image Opera 6's Cookie Dialog
Note all of the wonderful information it contains. Also not the options that Opera gives for EACH INDIVIDUAL COOKIE, IN ADDITION TO BY SITE/SERVER!
CookiePal's UI is better, IMO. See bug 102198 for screenshot.
iCab's (see attachment) is still better and offers the most possibilities.
It must be possible to accept/refuse all cookies for this domain or from this server. PS. iCab's one doesn't show server which wants to set cookie.
Attachment #10756 - Attachment is obsolete: true
Well, it sort of worked, with 0.9.8 and 0.9.9 under Linux, but breaks again with build #2002022613, also under Linux. Can we please have some non-JoJo-behaviour of such bugs, and please up the severity to major (See also comment #43 and possibly other comments relating to privacy)? Thanks...
*** Bug 131603 has been marked as a duplicate of this bug. ***
That makes 11 duplicates of this bug. It's getting close to making the Most Frequently Reported Bugs list at http://bugzilla.mozilla.org/duplicates.cgi.
Whiteboard: [p-ie/mac] [p-icab] [p-opera]
hello, it appears to be broken again with build id 2002040306 under linux :( please up the severity to major. thanks best. --toni++
adding self to cc list
*** Bug 136179 has been marked as a duplicate of this bug. ***
Hello, want to confirm that the bug is still there (build # 2202042021, ie, post-1.0rc1) Please also see comment #57. Why is this bug unowned? Sounds like if nobody wants to work on it :( Thanks!
now it's owned. please hurry up and fix it. if you have no intention, please reassign it to nobody. really, no one is working on this. Doron said he would after he lands expander.
Assignee: nobody → a_geek
a_geek, note that this is not a bug, but an enhancement. also note that it depends on a bug that hasn't been fixed yet, so this one can't be fixed either. but feel free to fix bug 85045 and this one yourself. otherwise please shut up. thanks.
Wrt. comment #62, I'd like to say that a bug, err, "enhancment" with 49 votes and many dupes (and counting) is not an enhancement, but a real bug. Without this bug fixed you often can't make an informed decision whether you want to accept or refuse a cookie. Please also note that it worked at some point in the past, so breaking it again definitely is a bug, and that I consider the dependency on a GUI element to expand and collapse the details (bug 85045) is invalid, or at least _much_ less important than just to show the data. _That_ would be an "enhancement". And no, I'm not going to shut up.
a_geek, you don't need to shut up, this bug is asssigned to you. We're all waiting for a patch.
Hello, below are a few ideas on what and how to do, as well as a few questions. Enjoy ;) - At first, the cookie dialog should just show the complete cookie data. - For enhancements to the UI, likely forcing a rewrite, I have different ideas on how to do it: For one, I guess most users have their preferences about the cookie dialog and don't intend to switch from one cookie to the next. Hence there should be some controls in the appropriate preferences dialog where the user can select which fields to view by default. Next, some cookies are really big and thus ugly to handle. I'd like to see a cookie dialog box on the screen that's at most as wide as the screen, but to accomodate huge cookies, they should show the contents in a "window" that can be panned right and left, using a scroll bar if necessary. For the other details, it would imho be "well enough" if the cookie dialog shows one item at a time, eg. expiry or path. There could be one control like a drop-down menu where the user can select the attribute of the cookie he wants to see, with the value display flipping accordingly (would save screen real estate, and the neccessary controls are already there, at least in other parts of the UI). If someone would strive for the "default attributes" idea outlined above, then these could be just statically displayed w/o needing such a fancy control. (timeless@mac.com suggested that the cookie dialog should present the cookie data in a fashion that allows cut&paste as well: "multiline wrapped, scrollable, copyable.")
*** Bug 146029 has been marked as a duplicate of this bug. ***
*** Bug 148870 has been marked as a duplicate of this bug. ***
Keywords: 4xp
Apologies for the recent duplicate, guess I just didn't try all the possible search terms for this. As there hasn't been an update for a while does anyone know of the status of this? My comments: The idea that fits well with me is the one where the normal dialogue is presented with a "More info" button that shows the content of the cookie (maybe an enhancement could be to have pref.js options to control excatly what that button reveals, default would be everything) Anyhow, something along the lines of the way IE does it is a good start, extra functionality can come later.
*** Bug 151951 has been marked as a duplicate of this bug. ***
*** Bug 156602 has been marked as a duplicate of this bug. ***
*** Bug 160313 has been marked as a duplicate of this bug. ***
So, is this still a debate on whether or not to enhance or how to program the enhancement?
re-assinging to self, as I have a patch.
Assignee: a_geek → mvl
Attached patch patch v1Splinter Review
A first attempt for a patch. It doesn't follow mpt's spec's. What I did was just immitate the current dialog box, and add a disclosure button. That button shows the content of the cookie. Further improvements can be discussed in others bug's I think. All comments are welcome, especially about the c++ part. I'm not sure if my way of opening the dialog is the correct way. As far as I know, I can not use the prompt service, because that doesn't allow custom xul.
Could you please attach a screen shot of your dialog. Also make sure you get module owner's permission before checking in such a patch.
Attached image screenshot of the dialogs (obsolete) —
The screenshots. Only modern.
Screenshots of the dialogs as in the patch.
Attachment #95538 - Attachment is obsolete: true
Attachment #95538 - Attachment is patch: false
Attachment #95538 - Attachment mime type: text/plain → image/png
the "More Info" button should change its label to "Less Info" when clicked (and on click hide the additional info)
the icab dialog http://bugzilla.mozilla.org/attachment.cgi?id=34985&action=view is really cool. perhaps we could make it look more like that than ie
See comment #52, we need an option to accept/allow all cookies from a specific server or domain.
screen shot looks EXACTLY like what is needed! Good Job! Will there be a way to set a preference so that all future cookie notification stays in "More Info" mode?
When the cookie details are shown, I think the checkbox should move to the bottom of the dialogue along with the buttons. It looks a bit lost staying at the top. I think the label of the 'More Info' button is a bit vague. If I hadn't already known the purpose of the button I would have assumed that it was a help button. I suggest renaming it to 'Cookie Details' or something. With the current dialogue, the wording makes it clear that checking the checkbox will accept/reject all cookies from the site in question. However, when the details are shown, some users may get confused and think that the box applies only to that specific cookie. IE gets round this by labelling its checkbox 'Apply my decision to all cookies from this Web site'. Similiar labelling could be used for the Mozilla dialogue (possibly without the word 'Web' as it's not used in the question at the top of the dialogue).
What is the difference between attachment id=95538 and attachment id=95539? Are the fields describing the cookie editable or are the read only? If the former, why?
On layour: - The labels should be right-aligned, not left-aligned. - When clicking on More Info, this design has the buttons moving to the bottom of the window, which will force the user to move the mouse if he wants to hide details. This will also make it less convenient for him to click on Accept or Decline. For this reason, I recommend having a layout where the buttons stay where they are, and the contents added to the bottom of the window (not unlike mpt's design). The checkbox should stay at the same position, additionally. - To acheive this, it may be that the accept/decline buttons will be better off vertically-aligned to the right, and the ( More Info ) button left-aligned at the bottom. This would have the effect of presenting a similar layout to mpt's suggestion, sans the disclosure triangle. About wording: - Instead of "Remember this decision", I suggest "Use this choice for all cookies from this site".
Thanks for al the feedback so far. The biggset problem with the placing of the buttons is that when using a <dialog> you can't place anything under the buttons. So when the buttons are moved, I need to use <window>, and create the buttons myself. About the vertical placing of the buttons: that is possbile, but used nowhere else in mozilla, as far as I know. Is it a good idea then? The 'More Info' Button currently works as a toggle, and it remebers it's state. The textboxes are read-only. The labels in other places in mozilla are also aligned left. It seems inconsistent to change that. The 2 screenshots are excatly the same. I just made a mistake while uploading. And who is the module owner? There is no 'cookie' component on http://www.mozilla.org/owners.html
> And who is the module owner? I am.
Attached image New screenshot
I changed some wording and the location of the buttons. The patch for that needs some work. I will upload that tomorrow.
Comments on the latest screenshot: - I suggest using "Contents" instead of "Information" as it is a bit more descriptive. - I think the question "Do you want to allow it?" isn't necessary, since Accept/Decline already matches the intention in "wants". This would make the dialog less wide and easier to scan. - "Cookie info" could become "Show details" and "Hide details" (depending on state), and I would suggest aligning it to the left so the layout would become: /|\ The site .foo.bar wants to modify an existing cookie. / * \ [ ] Use this choice for all cookies from this site. ( Show details ) ( Accept ) (( Decline )) +-- About this cookie -------------------------------------+ | | I'd also like to say thanks for working on this; this has been a top request for *very* long.
I think 'Allow' and 'Deny' would be better wording than 'Accept' and 'Decline'. 'Accept' and 'Decline' work well when a site is setting a cookie but they don't make as much sense when a site is asking to modify a cookie.
I was initially against this bug because showing too much information in the cookie nag box is confusing to users. It was not be accident but by deliberate design that the nag box in mozilla shows less information than the nag box in 4.x. However I like the work that was done here and I think that the added information would be useful (to the .1% of users who want it) provided it has no negative impact on the users who don't want it. That means that the current nag box should not be changed other than having the one added button to get more information. That said, I would be willing to approve this patch provided the appearance and wording in the current nag box is retained. If you want to change the wording, then open a separate bug on that. That means that the text would remain as "The site X wants to set a cookie. Do you want to allow it?", that the checkbox text remains as "Remember this decision", and that the buttons remain as "Yes" and "No", with an added button to get the cookie information.
I agree with morse, except for two nits: More then 1 in 1000 users want this. Closer to 1 in 10. Remember. You don't get the box in the first place unless you've entered prefs, expanded the privacy group, and modified your default cookie acceptance policy. 1 in 10 of THOSE users want this. I'm assuming this is why there's no help button in the dialog. That said, you're still right. It's very important it has "no negative impact on the users who don't want it". This is the second nit: "and that the buttons remain as "Yes" and "No"". While other wording changes would need to be debated by UI people, can't we all at least agree Yes/No are bad dialog button options?
That particular issue (yes/no) has been debated previously regarding this dialog and the agreement was yes and no (it's the answer to the question posed). I have no objection if you want to open that debate again, but doing so here would jeopardize the fine work that Leeuwan has done. I recomment we get his work checked in now using the original wording, and then debate changing the wording in a separate bug report. (This bug report is about adding new information, not about changing the labelling of buttons.)
I can provide a patch with the old wording. But is agree with comment 82 that the checkbox question is a bit vague when the cookie details are displayed. We should change that line. And where do you think the buttons should be placed? Like in the last screenshot?
I have no strong feelings about the placement of the buttons in the expanded view. My only concern is that the normal view not be changed (except for the addition of one extra button). That includes the wording on the checkbox. If there is disagreement on that wording, that should be the topic of a separate bug report.
Agree with Morse that wording should be a separate bug to avoid conflicts of the religious order on this overburdened bug. I do advise for the button layout to be as I suggested in the last mockup, and the use of `Contents' as label for the cookie data item. Filed bug 163350 (with Alex's suggested button wording).
> I do advise for the button layout to be as I suggested in the last mockup, As I said before, I have no strong feelings either way about the button placement > and the use of `Contents' as label for the cookie data item. That would make the wording in this dialog be inconsistent with the wording in the cookie-manager dialog. If you want to change the wording, it needs to be changed in both places. Again, open a separate bug report if that's what you want.
> About the vertical placing of the buttons: that is possbile, but used nowhere > else in mozilla, as far as I know. Is it a good idea then? It's compulsory. There are two simple requirements you need to meet. (A) The buttons must not change position between states, otherwise you get the where's-the-off-switch problem Kiko described. (B) The distance between a button and the edge of the window must not change between states, otherwise the window edge cannot be used as a visual guide to find the button (like it can in every other alert or dialog). That's why the buttons need to be at the top right, as I showed in attachment 63889 [details], not the bottom right. Another example of this arrangement is the Search Messages window in mail/news. Other problems with attachment 95538 [details]: 2. Without the individual cookie info shown, `Remember this decision' is merely highly misleading. With the individual cookie info shown, however, the checkbox is Just Plain Wrong and must be changed. The checkbox does *not* remember this decision, i.e. whether the cookie shown should be accepted; rather, it determines whether *any* cookie should be accepted or declined for the same host. That can be shown more truthfully, more obviously, with less clutter, and with fewer clicks required, if using a pair of buttons rather than a checkbox (as I showed in attachment 63889 [details]). 3. The individual cookie info should be indented the same amount as the initial question, rather than wrapping under the /!\ icon. 4. The field labels should be right-aligned, not left-aligned. 5. In any row of adjacent pushbuttons, either all of them should open/close windows, or none of them should. So you should not have `More Info', which does not open/close a window, right next to `Accept' and `Decline' which do.
Attached image Screenshot v3
This is my newest screenshot. It has the labels aligned to the right. Is has the information-box aligned with the text above. The text on the 'more info' button also changes on pressing it. It has the same wording as the current box, execpt for the checkbox-question. That would be to vague when using the current text. And the new wording won't have effect on existing users. I suggest to move the discussion about moving the buttons to the right and vertically align them to the wording bug.
Attached patch patch v3Splinter Review
The patch to get the previous screenshot.
I like this one. While silent here, I have been keeping a close eye on this bug. The last version seems to fit the bill just right.
> (B) The distance between a button and the edge of the window must not change > between states, otherwise the window edge cannot be used as a visual guide > to find the button (like it can in every other alert or dialog). In the layout Michiel has implemented, it is highly unlikely that the distance to the top and right edges of the window will change. Compounded to the fact that dialogs in Mozilla usually have horizontal layout for the buttons, I'd say let this stay for now. We've been ages discussing this bug and we finally have a reasonable UI for it, IMO. We can avoid further religious warfare, by postponing this change till we can do some user testing; this can be done together with rewording for bug 163350. Which brings me to ask. Michiel, what happens when the cookie contents are very long? Is the dialog fixed width? I've filed bug 163380 on the change from Information to Contents (and lets not let that fall into limbo please). (Michiel, again, reassign to nobody if you don't feel like doing them) I'm removed dependency on the disclosure element; someone can file another bug to block this when the time comes and disclosure comes into being. I've added the bugspill I filed as blocked bugs. I'm also assigning to the patch provider. Michiel, once again, thanks for the work. Since this seems to have some consensus going, can we move on to getting reviews on this patch? Morse?
Blocks: 163350, 163380
Status: NEW → ASSIGNED
No longer depends on: 85045
Comment on attachment 95771 [details] [diff] [review] patch v3 >+++ mozilla/extensions/cookie/resources/content/cookieAcceptDialog.xul 18 Aug 2002 19:54:55 -0000 [snip] >+ >+<!DOCTYPE window SYSTEM "chrome://cookie/locale/cookieAcceptDialog.dtd"> Change this to: <!DOCTYPE dialog SYSTEM "chrome://cookie/locale/cookieAcceptDialog.dtd"> (I've already got a huge patch in another bug to fix other occurrences of this; I don't need any more =) >+ >+<dialog >+ id="cookieperm-window" >+ buttons=""
comments on the patch up to comment 100 (iow there will be a bit of duplication from the later comments, sorry) i agree w/ mpt's spec (attachment 63889 [details]), but while you're ignoring it please change 'site' to 'domain': +<!ENTITY button.rememberdecision.label "Use this choice for all cookies from this site"> why this matters: 1. load http://www.washingtonpost.com 2. accept a bunch of cookies from the domain www.washingtonpost.com 3. you'll one other cookie (perhaps) from domain questionmarket.com the 'site' is 'http://www.washingtonpost.com', but your choice *must* affect the cookie domain (www.washingtonpost.com), not the site. if it affects the site then it would mean that you automatically accepted the third party cookie (questionmarket.com). Also note that IE doesn't let you hide the extra information. The logic is pretty simple: a. you asked for it b. your request for more information is not a persistent value c. your request was about a single cookie (the next dialog you get will require you to ask for more info too) anyway, since you persist the state between dialogs (!b), ie's approach doesn't work. I suppose i might as well do a partial code review. random nits (note that the nits generally apply to the whole patch, not just the random instances i mention): 1. your use of whitespace should be consistent with the file a. - if (!dialog) { - *checkValue = 0; - return PR_FALSE; + if(cookie_s) { + nsCOMPtr<nsIWindowWatcher> wwatch(do_GetService("@mozilla.org/embedcomp/window-watcher;1")); + if (!wwatch) { a' you just added two different styles |if(| and |if (| to replace a single style |if (| b. + res = block->SetInt(1, *checkValue); + + res = block->SetString(2, NS_ConvertASCIItoUCS2(cookie_s->name).get() ); b' |foo(...)| v. |foo(... )| c. + gDateService = Components.classes[nsScriptableDateFormat_CONTRACTID] + .getService(nsIScriptableDateFormat); c' the second line should line up with something useful, most of us align it with .classes d. + document.getElementById('ifl_isSecure').setAttribute("value", + params.GetInt(6) ? d' like c', i'd align it with "value" 2. directly calling QueryInterface is generally frowned upon: + active->QueryInterface(NS_GET_IID(nsIDOMWindowInternal), getter_AddRefs(activeParent)); 2' something like this is prefered: (this code isn't tested) + nsCOMPtr<nsIDOMWindow> active; + wwatch->GetActiveWindow(getter_AddRefs(active)); + nsCOMPtr<nsIDOMWindowInternal> activeParent(active); 3. it's nice if the original code line is meaningful (something talking about the cookie acceptance dialog or at least cookie management) + * The Original Code is Mozilla.org code. 3~ if you were to write mozilla.org, you would spell it 'mozilla.org', not 'Mozilla.org' @see http://www.mozilla.org/README-style.html 4. you need verify stuff exists and is non null before using it + params = window.arguments[0].QueryInterface(nsIDialogParamBlock); a. "arguments" in window b. window.arguments.length c. QueryInterface will throw an exception, so instead of null checking a/b you should probably wrap it in a try block. 5. please use meaningful constants instead of magic numbers: + var messageText = params.GetString(0); + params.SetInt(0, 0); // say that ok was pressed 6. (this is a personal nit, you or morse may disagree in which case it's discardable, well all nits are discardable, but you might have trouble getting approval for your code if some of them aren't addressed) you're explicitly comparing against true/false: + if (document.getElementById('infobox').hidden == false) { 6' i'd simply write: + if (!document.getElementById('infobox').hidden) { 7. variables used with magic numbers should have meaningful names: +function GetExpiresString(expires) { + if (expires) { + var date = new Date(1000*expires); perhaps |secondsUntilExpiring| [for readers: Date() accepts GMTmilliseconds - well maybe that's UTCmilliseconds] 8. doctype should match the root element +<!DOCTYPE window SYSTEM "chrome://cookie/locale/cookieAcceptDialog.dtd"> + +<dialog 8' <!DOCTYPE dialog SYSTEM ...> 9. please put the id on the same line as the node: +<dialog + id="cookieperm-window" not doing so makes grep/lxr output useless. 10. i think you're encouraged to include a type attribute: + <stylesheet src="chrome://global/skin/dialog.css"/> + <script src="cookieAcceptDialog.js"/> (correct types for the lines above would be text/css and application/x-javascript respectively) 11. use <?xml-stylesheet ?> instead of <style> in xul files for example: /mailnews/extensions/smime/resources/content/msgReadSecurityInfo.xul, line 39 -- <?xml-stylesheet href="chrome://messenger/skin/smime/msgReadSecurityInfo.css" type="text/css"?> 12. <dialog><hbox/></dialog> is silly <dialog orient="horizontal"/> should work. 13. localization/skinability warning. inline styles in packages are generally frowned upon. + <vbox flex="1" style="max-width: 45em;"> 14. err. + <checkbox id="checkbox" oncommand="onCheckboxClick(this);" label="&button.rememberdecision.label;"/> a. a generic checkbox handler is useless b. you do not need to pass this to oncommand c. passing this may be expensive a'b'c' perhaps onToggleAlwaysAcceptFromDomain() d. please don't use generic names for ids. d' id="persistDomainAcceptance" might be better 15. cache strings + document.getElementById('disclosurebutton').setAttribute("label",cookieBundle.getString('showdetails')); every second time you click the button it will reretrieve this string. that's silly. 16. don't confuse localizers or reviewers by inserting inconsistent whitespace +no=no +AtEndOfSession = at end of session 16' please remove the spaces around '=' unless they are meaningful, in which case you should provide a localizer note explaining why they are there. 17. don't change your case convention for identifiers +no=no +AtEndOfSession = at end of session + +showdetails=Show details 17' atEndOfSession showDetails nb. you may not need a .properties file, it's possible to rely on xul/dtd instead.
The latest screenshot uses 'Show details' and 'Hide details'. Please change those to 'Show Details' and 'Hide Details' (note the capital 'D's) to be consistent with the case on the other buttons in Mozilla.
Please do not make any changes to the wording. This bug is about adding more information about the cooke and not about changing existing wording. Any requests for such changes should go into separate bug reports. That includes the checkbox wording of "remember this decision". And it includes the changing of "site" to "domain". That latter issue, by the way, was evaluated quite heavily by the tech pubs department, and they had very sound reasons for going with "sites" even though it wasn't technical correct. Also please restore the alignment of the labels to be left justified so that they will be consistent with the cookie-manager dialog. If you want to change that, then you will need to change them both at the same time, and again that needs to be a separate bug report. I think it's strange that you would intentionally waste real-estate by having the "about this cookie" box be aligned with the text above it rather than coming to the left margin. But if that's what you really want, I won't object. Finally, in regards to the question about what happens if the text is too wide to fit in the field, I would hope that the field is scrollable in the same manner that the corresponding fields in the cookie-manager dialog are. Is that indeed the case. (Of course, if the box came up to the left margin, you would have more information in the field before it would need to be scrolled. But that's your call.)
Attached patch Patch v4 (obsolete) — Splinter Review
This patch uses the original wording everywhere. The only extra text is for the details-button, which is labeled |Show Details| or |Hide Details| depending on the state. The alignment of the labels is back to the left. Most of the remarks of timeless are fixed. Thanks. I only had a few problems: > 12. <dialog><hbox/></dialog> is silly > <dialog orient="horizontal"/> should work. somehow that doesn't work. Don't know why. > nb. you may not need a .properties file, it's possible to rely on xul/dtd > instead. How do I do that? Could not find any info on that.
>> nb. you may not need a .properties file, it's possible to rely on xul/dtd >> instead. > How do I do that? Could not find any info on that. I've cleverly done that in the past by creating a dummy attribute in a xul tag, having that attribute get its value from the dtd file, and then having the js file read the value of that attribute. But at times I've had super-reviewers reject that by saying it was a "hack" and insisting that I use a separate .properties file. So you're dammed if you do and dammed if you don't. :-( Please post a screenshot showing what your latest patch looks like.
:-( mozilla crashed 1. i agree with morse that you shouldn't indent the details, ie doesn't either and it fixes my complaint about a horizontal dialog :) 2. re constants, if you stick a constant into an idl file then in c++ you can do |<interface>::<constant>| and in js you can do |Components.interfaces.<interface>.<constant>|. if you don't do that then please prefix 'const ' to your constant definitions in the js file. 3. check return values: + res = wwatch->OpenWindow(active, "chrome://cookie/content/cookieAcceptDialog.xul", "_blank", + "centerscreen,chrome,modal,titlebar", block, + getter_AddRefs(dialogwin)); + + /* get back output parameters */ openWindow can fail, hence the result which you should check (which per jst nit should be called |rv|, but we'll ignore that unless morse/jst demand it) before proceding. keep up the good work :-).
Attached patch patch v5 (obsolete) — Splinter Review
While creating the requested screenshot I noticed that the expires-date wasn't displayed. Now it is. And while I was doing that, I also corrected the latest remarks.
Attachment #95904 - Attachment is obsolete: true
Attached image screenshot v5
The accompanying screenshot
That looks fine to me, providing you change the spelling of "remeber" to "remember". ;-) as module owner, a=morse timeless seems well along on doing the review, so I'll leave the r= to him. Then all you'll need in order to check in is an sr. Let me know if you don't have checkin privileges, in which case I'll check it in for you.
in the cookie manager the Expires is shown in locale date, like: 13 august 2012 15:03:46 for me on my english win2k localized to Danish. is this also thought into the patch?
hg: this patch uses the same code as the cookieviewer or whatever, so it should do the right thing. mvl: + if (!this_cookie) { + PR_FREEIF(path_from_header); + PR_FREEIF(host_from_header); + PR_FREEIF(name_from_header); + PR_FREEIF(cookie_from_header); + nsCRT::free(setCookieHeaderInternal); + return; + } PR_FREEIF is really PR_DELETEIF, since you're about to return, you shouldn't use it (it's a waste of a cycle to assign 0 to the pointers) -- as usual if morse wants to trump this request you may ignore it (to be consistent with the file) -- instead please use if (foo) PR_Free(foo); + nsCOMPtr<nsIDialogParamBlock> block(do_CreateInstance("@mozilla.org/embedcomp/dialogparam;1")); please add a check for |if (!block)| + block->SetString(eMessageText, szMessage); if you're using both of these, please make their strings consistent: +<!ENTITY button.showdetails.label "Show details"> +<!ENTITY button.hidedetails.label "Hide details"> +showDetails=Show Details +hideDetails=Hide Details i've decided to waste time in the spellchecker, can you change "Int's" and "Strings" to "integers" (or "numbers") and "strings"? or at least make the case/word choice consistent in these lines: +// Continue at a lower _number_, as > 7 gives problems (max _int number_ is 8), +// And you the next items are _Int's_ and not _Strings_. +// _Int's_ and _Strings_ are separate, thay can have the same index (from email) "thay" perhaps: +// Numbers and strings are handled separately so they can share the same indices. persistance should be persistence (thanks n7p1) morse, can we change: + PRBool isDomain; /* is it a domain instead of an absolute host? */ to + PRBool isDomain; /* it i a domain instead of a[n absolute] host */ ? everytime i see that question mark i ask myself if the coder is confused/concerned and then i stop and answer, no it's just a comment... I think [n absolute] should be dropped: absolute in dns has a specific meaning <q src="http://docsrv.caldera.com/NET_tcpip/dnsC.domname.html"> Each name can be relative or absolute (fully qualified). A fully qualified domain name ends at the root domain, indicated by a trailing dot (example: volga.mynet.com.). A domain name minus the dot may be seen as a relative domain name (example: volga.mynet). Again, parallels exist with the UNIX filesystem, in which the file /usr/bin/hack may be indicated as /usr/bin/hack (its absolute name) or simply hack (its relative name if the directory is known to be /usr/bin).</q> I presume that this flag is talking about "discovercard.com" vs. ".discovercard.com" and not "discovercard.com" vs "discovercard.com." (note that i actually do load both sites, and I am always disappointed to see that i get the wrong site when i add the trailing dot. I think i'm done (don't forget to fix the spelling error morse caught).
Attached patch patch v6 (obsolete) — Splinter Review
Spelling changed. I should not try to write decent English late in the night anymore. Just go to bed. :-) I will leve the PR_FREEIF to morse. I hope he will check this in for me, as I don't have permissions. So maybe he can do it at the same time. (No screenshot this time. That didn't change)
Attachment #95920 - Attachment is obsolete: true
timeless wrote: > PR_FREEIF is really PR_DELETEIF, since you're about to return, you shouldn't use > it (it's a waste of a cycle to assign 0 to the pointers) -- as usual if morse > wants to trump this request you may ignore it (to be consistent with the file) Leeuwen wrote: > I will leve the PR_FREEIF to morse. I have no strong feelings one way or the other regarding changing those occurences of PR_FREEIF to PR_DELETEIF. If you'd like to do so, please provide a revised patch containing it. That will make my eventual check-in easier. Have you requested a super-review of these changes? If not, you should do so. Timeless, are you prepared to add your r= to this patch?
I checked wat PR_DELETEIF does, and according to lxr, it doesn't exist :-) So I wan't change it.... http://lxr.mozilla.org/seamonkey/ident?i=PR_DELETEIF Who can I ask for super-review?
> Who can I ask for super-review? See the list at http://www.mozilla.org/hacking/reviewers.html.
yeah i'll mark an r= the change is from PR_FREEIF(foo) to if (foo) PR_Free(foo); what i meant is that the meaning of PR_FREEIF is actually PR_DELETEIF PR_FREEIF(foo) => if (foo) PR_DELETE(foo) PR_DELETE(foo) => { PR_Free(foo); foo=NULL; } anyway, i'm sick of explaining this. bug 163696 filed. i did try to explain what to do in comment 113: -- instead please use if (foo) PR_Free(foo);
I had the following errors trying to compile with v6. (Win2k) nsCookies.h(83) error C2146: syntax error: missing ';' before identifier 'expires' nsCookies.h(83) error C2501: 'time_t' : missing storage-class or type specifiers nsCookies.h(83) error C2501: 'expires' : missing storage-class or type specifiers nsCookies.h(84) error C2146: syntax error: missing ';' before identifier 'lastAccessed' nsCookies.h(84) error C2501: 'time_t' : missing storage-class or type specifiers nsCookies.h(84) error C2501: 'lastAccessed' : missing storage-class or type specifiers
Attached patch patch v6b (obsolete) — Splinter Review
Missed #include <time.h> That broke the build. Thanks for testing.
Attachment #96043 - Attachment is obsolete: true
A couple JavaScript strict warnings: cookieAcceptDialog.js line 72: cookieBundle undeclared cookieAcceptDialog.js line 90: params undeclared And a JavaScript error: cookieAcceptDialog.js line 143: eCheckboxState not defined I'm being prompted for all cookies, regardless of telling the confirmation to "Remember this decision".
Attached patch patch v6c (obsolete) — Splinter Review
Fixed the JS errors. The not remembering error was alsa a JS error. I should learn to turn on strict js errors.
Attachment #96067 - Attachment is obsolete: true
Comment on attachment 96194 [details] [diff] [review] patch v6c >Index: mozilla/extensions/cookie/nsPermissions.cpp >+ block->SetString(eCookieName, NS_ConvertASCIItoUCS2(cookie_s->name).get()); >+ block->SetString(eCookieValue, NS_ConvertASCIItoUCS2(cookie_s->cookie).get()); >+ block->SetString(eCookieHost, NS_ConvertASCIItoUCS2(cookie_s->host).get()); >+ block->SetString(eCookiePath, NS_ConvertASCIItoUCS2(cookie_s->path).get()); Those pointers (from get() ) are dangling. SetString does not copy, so you either need to hold a reference to the strings until after the dialog closes or copy the PRUnichar*s (the former sounds cheaper =).
Attached patch patch v7Splinter Review
New Patch. Pointer issue should be fixed now.
Attachment #96194 - Attachment is obsolete: true
+yes=yes +no=no +atEndOfSession=at end of session You dislike uppercase? +// XXX Can I share these with nsIPermissions.cpp (or .h) somehow? if it's an .idl file, you can. else, you can't. :) that's just what I spotted right now with my being tired...
Christian, "yes", "no", and "at end of session" are currently lower-cased in the cookie-manager dialog. So if you want to change that behavior, then open a new bug on that issue. For this bug, we need to be consistent with the current implementation.
Isn't this patch ready for review yet, I have been running it without problems for some time. Great work!
Keywords: patch, review
Yes, I think so. timeless or morse, can one of you mark the last patch r=?
timeless, since you've already reviewed this, please add your r=.
I applied the patch to the 1.1 branch, and compiled it on FreeBSD 4.5/i386, on which it is working fine (it's my normal browser now for a day). I struggled through getting a Win32 version compiled, last night. Patches also seem to work on Win32 (I see the cookie dialog with the [Show Details] button, details are correct, show details sticky), although I have a long way to go before I have a usable Win32 browser. (let's here it for FreeBSD ports) The only problem with the patch is a failure on the extensions/cookie/nsPermissions.cpp file, which I had to patch by hand. Nice work, Michiel!
Attached patch patch v8 (obsolete) — Splinter Review
A patch with the constants moved to a (new) .idl file, as requested.
Blocks: 166925
Attached patch path v8.2 (obsolete) — Splinter Review
I screwed up, and forgot the mac build system. Sorry for the spam.
Attachment #97999 - Attachment is obsolete: true
Attached patch Yet Another Patch (obsolete) — Splinter Review
change |(true)| to |PR_TRUE|
Attachment #98005 - Attachment is obsolete: true
Comment on attachment 98024 [details] [diff] [review] Yet Another Patch r=timeless (note to people examining the makefile with concerns about consistency, the prececding lines used tabs tssk tssk). great work
Attachment #98024 - Flags: review+
Blocks: majorbugs
Comment on attachment 98024 [details] [diff] [review] Yet Another Patch the comment /* copy */ isn't enough - I think you need to indicate the ownership of the strings like name_from_header, etc other than that, this looks ok. sr=alecf with the comment fix.
Attachment #98024 - Flags: superreview+
added requested comments. // put the cookie information into the temporary struct. // just copy pointers to the char * strings (cookie_from_header etc). // we still own the strings, and we must free them if the cookie is // rejected.
Comment on attachment 98630 [details] [diff] [review] Yet Another Patch v2 bringing reviews forward from previous patch
Attachment #98630 - Flags: superreview+
Attachment #98630 - Flags: review+
Attachment #98024 - Attachment is obsolete: true
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago22 years ago
Resolution: --- → FIXED
See bug 170237 for error introduced with this checkin.
*** Bug 173952 has been marked as a duplicate of this bug. ***
This feature is being disabled because it caused problems for embedders. See bug 179798. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 179798
Marking resolved, per checkin for bug 179798... fixes the embedding problem. Note that this patch was never backed out, so nothing more needs to be done.
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
VERIFIED/FIXED: I've run UI functional tests on several milestones right now. There are minor issues, but they all seem to have open bugs documenting them.
Status: RESOLVED → VERIFIED
QA Contact: mpt → cookieqa
No longer blocks: majorbugs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: