Closed Bug 163350 Opened 22 years ago Closed 21 years ago

Cookie confirmation dialog could use better wording.

Categories

(Core :: Networking: Cookies, defect)

Product:
Core
Component:
Networking: Cookies
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: kiko, Assigned: mvl)

References

Details

Attachments

(8 files, 3 obsolete files)

too wide dialog
13.63 KB, image/png
Details
Suggestion for dialog
11.30 KB, image/png
Details
updated suggestion
11.70 KB, image/png
Details
new screenshot
10.46 KB, image/png
Details
Patch v2
13.39 KB, patch
timeless
: review-
Details | Diff | Splinter Review
screenshot that goes with previous patch
14.73 KB, image/png
Details
Fix skin work
17.00 KB, patch
timeless
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review
Also fix js error
1.65 KB, patch
skasinathan
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review 
This is a carry-over from bug 23508.

The current cookie dialog uses the following wording:

   /|\ The site X wants to set/modify a/an existing cookie. Do you want to allow it?
    [ ] Remember this decision

    (( Yes )) ( No )

This wording is long (even for an ascii mockup) and prompts the user with a
Yes/No question. I provide a small mockup to suggest new wording:

    /|\  The site .foo.bar wants to set a/modify an existing cookie.  
   / * \ [ ] Use this choice for all cookies from this site.
     
  ( Show details )                   (  Allow  ) ((  Deny  ))

Rationale:

- I think the question "Do you want to allow it?" isn't necessary, since
Allow/Deny already matches the intention in "wants". 
- The change would make the dialog less wide and easier to scan, which is a good
thing for any dialog.

Removing the question and offering a choice (i.e. no Yes/No buttons) also makes
the dialog slighly less intrusive, but that's not the main issue here. Michiel
has already coded the change for 23508 so I'm assigning to him in hope he wants
to take this as soon as bug 23508 is checked in. If not, reassign to nobody, as
always.
also, the word 'contents' is a bit more descriptive than 'information' when 
describing the value of the cookie. to be consistent, same should be changed in 
cookie manager dialog.
Depends on: 23508
Attachment 63889 [details] in bug 23508 suggest tu use vertical oriented buttons. That
seems a good idea to me. 
It also suggest not to use a checkbox, but use 'accept all' and 'decline all'
buttons. Seems good too.

(Should we use Decline or Deny? I am no native english speaker. Is there any
difference?)
Status: NEW → ASSIGNED
Attached image too wide dialog — 
It can happen that the dialog is much too wide. See screenshot. That should be
preventend with new wording. Some newlines won't hurt.
Attached image Suggestion for dialog — 
Attaching a suggestion. I changed some wording, but alsa some of the layout. If
that should nog happen in this bug, let me know. We can open another bug.
If your concern is about the dialog being too wide, you can always add
line-breaks to the text so that it appears on several narrow lines instead of
one wide one.
That is not my only concern.
I think accept/reject is more clear then yes/no. That way, you only have to
reaad the first line to decide which button to click.
That is also why I made the first line bold. That is all you need to know. The
rest is extra information.

Also, it seems to mee that the current wording is not consitent when you have
mote than 2 cookies from one site. "The site foo.bar wants to set a second
cookie. " and "The site foo.bar wants permission to set another cookie".

And the twisty just looks better.
The problem with the twisty is that it's not as obvious as the button, but since
it was in the original spec, it's acceptable as long as clicking on the "About
this label" cookie also triggers it.

I would really recommend removing "Do you want to remove it?" -- accept and
reject  already complement the "wants to modify" statement (and wants already
denotes intention).

I also think "Remember decision" is being unclear at best, and false at worse.
What the checkbox does is "Use this choice for all cookies from this site",
though you can use "Remember this decision..." if you strongly disagree with that.
> I think accept/reject is more clear then yes/no. That way, you only have to
> reaad the first line to decide which button to click.

I agree but I think that 'Allow' and 'Deny' would be better than 'Accept' 
and 'Reject'. The reason for this is that 'Accept' sounds like physically 
accepting an item (and 'Reject' like physically rejecting something). This is 
fine for setting cookies but not so good for modifying cookies. If it was "The 
site www.test.localdomain has offered to modify an existing cookie.", 'Accept' 
(accepting the offer) and 'Reject' (rejecting the offer) would be fine. But the 
site isn't offering, it's asking your (the user's) permission. In light of 
this, I think 'Allow' and 'Deny' would be better.

> That is also why I made the first line bold. That is all you need to know. The
> rest is extra information.

Nice touch. :-)

> The problem with the twisty is that it's not as obvious as the button, but
> since it was in the original spec, it's acceptable as long as clicking
> on the "About this label" cookie also triggers it.

I understand that twisties ('disclosure triangles') like this are more common 
on Macs than on Windows and Linux. Presumably Mac users will have no problem 
with the twisty but Windows and Linux users may do. Therefore I think a button 
would be better.

> I would really recommend removing "Do you want to remove it?" -- accept and
> reject  already complement the "wants to modify" statement (and wants already
> denotes intention).

I agree. Less text is good (as long as it doesn't reduce the meaning). If that 
happens though, the remaining text should not be bold.

> I also think "Remember decision" is being unclear at best, and false at worse.
> What the checkbox does is "Use this choice for all cookies from this site",
> though you can use "Remember this decision..." if you strongly disagree with 
> that.

Just to be clear, am I right in thinking that checking the box and:

a) Clicking 'Accept' allows the site to set and modify cookies at will?
b) Clicking 'Reject' does not allow the site to set any new cookies nor modify 
any existing ones?

If so, then perhaps 'Allow All' and 'Deny All' buttons would be better. Or 
maybe the checkbox should use wording closer to IE's ("Apply my decision to all 
cookies from this Web site").
> Just to be clear, am I right in thinking that checking the box and:
>
> a) Clicking 'Accept' allows the site to set and modify cookies at will?
> b) Clicking 'Reject' does not allow the site to set any new cookies nor modify 
> any existing ones?

mpt's original spec called for Accept All/Reject All buttons with Accept/Reject
buttons in the disclosed part. I think it *might* be the best solution, but I'm
not sure if 4 buttons (5 if you have a disclosure button) is going to be confusing.

- Apply my decision to all cookies from this Web site
- Use this decision for all cookies from this site
- Use my choice for all cookies from this site

I think the last and shortest option is the best for a checkbox. "Web site" and
"site" are interchangeable, as "decision" and "choice". I think "use" is better
than apply.

I also suggest Allow/Deny, as the mockup I opened the bug with indicates.
> Just to be clear, am I right in thinking that checking the box and:

Sorry. Yes. :-)
I agree about Allow and Deny.

> If so, then perhaps 'Allow All' and 'Deny All' buttons would be better. Or 
> maybe the checkbox should use wording closer to IE's ("Apply my decision to all 
> cookies from this Web site").

My problem with 'Allow All' is it is not clear to me (as user) if it means
'Allow All cookies form this site' or 'Allow All cookies from all sites'. A
checkbox can have a much longer label then a button, so no problem in that respect.
"Use my choice for all cookies from this site" sounds good to me.

In the screenshot, I used vertical buttons. That looks better to me, and is what
is in mpt's spec. I that allright? It is not used much in the rest of mozilla.
Attached image updated suggestion — 
My updated suggestion. It has:
- allow / deny buttons
- a more window-ish more-info button instead of a twisty.
- a short header with the problem.
- a little more info, with an actual question. without a question, what do you
answer?
- a change in the checkbox text

I don't have a patch yet, as there is still have a problem. The width of the
more/less info button changes, and so the width of the dialog changes too. That
is ugly. But in the meantime, comments are welcome.
Comment on attachment 102853 [details]
updated suggestion

on windows readonly fields do not look like editable textareas. only disabled
textareas should appear the way you see the fields in your picture.

Note: This is a skin issue, so for win classic they shouldn't look like
disabled editable textareas, but if some other skin wanted to make it look the
way you showed it in your picture, that would be a decision for the skin.
Attached patch change wording and layout (obsolete) — — Splinter Review 
I am uplocaing my opatch anyway. I have not found a solution for my problem.
But it only shows when pressing the moreinfo button. I think most people just
leave it  in one state. But ideas for fixing it are welcome.

Timeless: it are readonly textboxes. So linux and windows differ. If windows
displays them like disabled editable textareas, it is a bug in windows classic.
Component: User Interface Design → Cookies
*** Bug 142277 has been marked as a duplicate of this bug. ***
I've discussed this a bit with Michiel and I think he's going to work out
another (final?) layout so we can get this reviewed.
Attached image new screenshot — 
A new suggestion.

Horizontal buttons to solve previous problems. Right aligned labels.
Yes, this is a nice, compact layout and the wording is clear, much better than
my "solution" from bug 142277.
Adding suresh, for comments on the screenshots.
In comment 2, moshe proposed using "Content" instead of "Information" and I
think it makes a lot of sense. mvl?

My only other reservation is towards having the text with the number of cookies.
Does anybody actually care how many cookies they have from that site? My mom
sure doesn't, and I never have either. I'd rather have less text on a dialog
than more.

Anyway, this is my only comment. I think this dialog is *MUCH* nicer than the
original one, so let's hope we can get some review and checkin.
*** Bug 163380 has been marked as a duplicate of this bug. ***
> In comment 2, moshe proposed using "Content" instead of "Information" and I
> think it makes a lot of sense. mvl?
Yes, that makes sense. I will change it.

> My only other reservation is towards having the text with the number of 
> cookies. Does anybody actually care how many cookies they have from that site? 
> My mom sure doesn't, and I never have either. I'd rather have less text on a 
> dialog than more.
Your mom doesn't use the dialog at all :)
The text is there because it always was. I don't use it. I think we can remove it.
Do you want the difference between setting a first and setting another cookie to
go to?

Adding Jatin to cc list.

Jatin, can you review the wordings/strings posted in the screenshot? Thanks!
I would not remove the number of existing cookies. It does not take much space,
and it may be useful to some people. If you really do not like it then at least
put it into the part that appears with the details.
[And if you completely remove it, then why would you want the first line in bold
;-) ?]
Just putting things in the UI (or leaving them) because they may be usefull to
somebody isn't going to work. That way, the UI will be unusable because of all
things that might be usefull to someone.
So we need a better reason then maybe usefull for someone. At least /why/ it
might be usefull.
As I said, it does not take much space and it doesn't clutter the UI at all. And
it is really bad to remove a feature that has been there for so long.

Why? Hmm, for me it is sometimes useful, because of the hundreds of sites
wanting to set cookies I forget which cookies I really need and if I see that I
already have cookies from a site I am reminded that I may need it from this one
and after looking at the details I can decide...
> Just putting things in the UI (or leaving them) because they may be usefull to
> somebody isn't going to work. That way, the UI will be unusable because of all
> things that might be usefull to someone.

Michiel, you're a swell guy. This sums up the point precisely. And I would add
on: "just because you think you can come up with use case for something doesn't
mean it's a *real* use case." We're not even typical users.

Anyway, I would *recommend* all of this cookie counting were removed. The simple
plain message works fine for all cases, I'm absolutely positive of it, in fact. 

However, this is a delicate point: arguments in Bugzilla get started over
nothing, and I honestly don't want this bug to be held down because people start
complaining about us cleaning up the UI to make it saner. So if people are going
to start screaming (as they did when we removed toolbar grippies) that Mozilla
is going to suck because it doesn't tell you how many cookies you have set in a
prompt dialog that 0.3% of the end-users will see, I would rather we stopped
here, checked in what people have seemed to agree upon, and count ourselves
lucky this time. The lack of UI authority just makes working on it too scary,
we're lucky this bug has but a few people CC:ed on it.

And yes, grippies were turned back on.
Attached patch Patch v2 — — Splinter Review 
The patch that gives the previous screenshot.
Only changed information -> content.
With the number of cookies. We can file a seperate bug on that, to prevent long
discussions here. This part is ready in my opinion.
Attachment #104321 - Attachment is obsolete: true
Attachment #114122 - Flags: superreview?(darin)
Attachment #114122 - Flags: review?(suresh)
Comment on attachment 114122 [details] [diff] [review]
Patch v2

> +        <image id="info.icon" class="spaced" src="chrome://global/skin/icons/alert-exclam.gif"/>

do *not* hard code skin paths into xul.

dbaron doesn't particularly like the idea of '.' in an id, given how it'd
interact with css i'd have to agree.

i believe you ignored my comment about readonly fields v. selectable labels,
but that's not a showstopper.
Attachment #114122 - Flags: superreview?(darin)
Attachment #114122 - Flags: review?(suresh)
Attachment #114122 - Flags: review-
Attached patch Use class=plain for textboxes (obsolete) — — Splinter Review 
Using class="plain" now for hte textboxes, to remove the border. To be
consistent, also update the cookieviewer.
And changed the id="info.box". btw, we should also fix
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/commonDialog.xul#26
but that is beyond the scope of this bug.
Attached patch Fix skin work — — Splinter Review 
I forgot the skin stuff. Done now by using <image id="infoicon" class="spaced
alert-icon"/>
Attachment #114254 - Attachment is obsolete: true
Attachment #114354 - Flags: review+
Attachment #114354 - Flags: superreview?(darin)
Comment on attachment 114354 [details] [diff] [review]
Fix skin work

deferring UI superreview to jag ;-)
Attachment #114354 - Flags: superreview?(darin) → superreview?(jaggernaut)
Comment on attachment 114354 [details] [diff] [review]
Fix skin work

-  dialog.getButton("accept").label = dialog.getAttribute("acceptLabel");
-  dialog.getButton("cancel").label = dialog.getAttribute("cancelLabel");
+  document.getElementById("ok").label = dialog.getAttribute("acceptLabel");
+  document.getElementById("cancel").label =
dialog.getAttribute("cancelLabel");

Why this change?

I'll take another look after jatin comments on the new wording.
Looks good to me except the sentence, "View and Remove Cookies that are
stored..." should be rewritten to say, "View and remove cookies that are stored..."
One minor detail too: we are using Hostname on the cookie confirmation dialog,
versus Domain in the cookie details dialog. Is this intentional? I thought both
represented domain, but let me check... 

Oh! We currently use Domain: in the confirmation dialog. I don't think it should
be changed to hostname because, after all, it's a domain :-)
>-  dialog.getButton("accept").label = dialog.getAttribute("acceptLabel");
>-  dialog.getButton("cancel").label = dialog.getAttribute("cancelLabel");
>+  document.getElementById("ok").label = dialog.getAttribute("acceptLabel");
>+  document.getElementById("cancel").label =
>dialog.getAttribute("cancelLabel");
>
>Why this change?

I had to use on overlay to be able to put the button not at the bottom of the
window. But now the getButton() function doesn't work anymore, maybe because the
overlay doesn't use dlgtype to define the buttons. 


>Looks good to me except the sentence, "View and Remove Cookies that are
>stored..." should be rewritten to say, "View and remove cookies that are
>stored..."

I didn't touch that, but can fix it while I am there.


>One minor detail too: we are using Hostname on the cookie confirmation dialog,
>versus Domain in the cookie details dialog. Is this intentional? I thought both
>represented domain, but let me check... 

No, we use both in both places :)
Domain when the cookie is set for the whole domain, so .mozilla.org as domain
atribute. That cookie will be sent to every host in the mozilla.org domain.
Host when a cookie has no domain attribute, which means it wil only be sent to
the host that set it. And in the screenshot, there is one host cookie, and one
domain cookie.
> I had to use on overlay to be able to put the button not at the bottom of 
> the window. But now the getButton() function doesn't work anymore, maybe 
> because the overlay doesn't use dlgtype to define the buttons.

Hmmm... Either dialogOverlay.xul (and its platform versions) should be made to
work together with the dialog binding (add dlgtype, make oncommand handlers work
with dialog binding's command dispatch mechanism), or we need to add support to
the dialog binding to move the buttons around.

For now, sr=jag on your patch.
Attachment #114354 - Flags: superreview?(jaggernaut) → superreview+
Checking in extensions/cookie/resources/content/cookieAcceptDialog.js;
/cvsroot/mozilla/extensions/cookie/resources/content/cookieAcceptDialog.js,v 
<--  cookieAcceptDialog.js
new revision: 1.5; previous revision: 1.4
done
Checking in extensions/cookie/resources/content/cookieAcceptDialog.xul;
/cvsroot/mozilla/extensions/cookie/resources/content/cookieAcceptDialog.xul,v 
<--  cookieAcceptDialog.xul
new revision: 1.4; previous revision: 1.3
done
Checking in extensions/cookie/resources/locale/en-US/cookieAcceptDialog.dtd;
/cvsroot/mozilla/extensions/cookie/resources/locale/en-US/cookieAcceptDialog.dtd,v
 <--  cookieAcceptDialog.dtd
new revision: 1.2; previous revision: 1.1
done
Checking in extensions/cookie/resources/locale/en-US/cookieAcceptDialog.properties;
/cvsroot/mozilla/extensions/cookie/resources/locale/en-US/cookieAcceptDialog.properties,v
 <--  cookieAcceptDialog.properties
new revision: 1.3; previous revision: 1.2
done
Checking in extensions/wallet/cookieviewer/CookieViewer.dtd;
/cvsroot/mozilla/extensions/wallet/cookieviewer/CookieViewer.dtd,v  <-- 
CookieViewer.dtd
new revision: 1.13; previous revision: 1.12
done
Checking in extensions/wallet/cookieviewer/CookieViewer.xul;
/cvsroot/mozilla/extensions/wallet/cookieviewer/CookieViewer.xul,v  <-- 
CookieViewer.xul
new revision: 1.63; previous revision: 1.62
done

I checked the patch in for mvl, fixed.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
>>Looks good to me except the sentence, "View and Remove Cookies that are
>>stored..." should be rewritten to say, "View and remove cookies that are
>>stored..."
>
> I didn't touch that, but can fix it while I am there.

Hmm, did you take that into account Robert?
Attached patch Change last sentence (obsolete) — — Splinter Review 
Ok, I suck. I forgot to update that. Attaching patch that changes it.
There is an JS error on the console too. This patch fixes that. (And the
centence I forgot)
Attachment #115243 - Attachment is obsolete: true
Reopening for the js error and missed sentence
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #115255 - Flags: superreview?(darin)
Attachment #115255 - Flags: review?(suresh)
Comment on attachment 115255 [details] [diff] [review]
Also fix js error

r-suresh
Attachment #115255 - Flags: superreview?(jaggernaut)
Attachment #115255 - Flags: superreview?(darin)
Attachment #115255 - Flags: review?(suresh)
Attachment #115255 - Flags: review+
Nice use of readonly="true" class="plain"
Attachment #115255 - Flags: superreview?(jaggernaut) → superreview+
Checking in cookie/resources/content/cookieAcceptDialog.js;
/cvsroot/mozilla/extensions/cookie/resources/content/cookieAcceptDialog.js,v 
<--  cookieAcceptDialog.js
new revision: 1.6; previous revision: 1.5
done
Checking in wallet/cookieviewer/CookieViewer.dtd;
/cvsroot/mozilla/extensions/wallet/cookieviewer/CookieViewer.dtd,v  <-- 
CookieViewer.dtd
new revision: 1.14; previous revision: 1.13
done
Status: REOPENED → RESOLVED
Closed: 22 years ago → 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: