Closed Bug 495937 Opened 15 years ago Closed 14 years ago

New Smart Folder dialog doesn't use a verb for the confirm button

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
Linux
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a1

People

(Reporter: andreasn, Assigned: ewong)

Details

Attachments

(3 files, 5 obsolete files)

Attached image screenshot of the issue
According to the Interface guidelines for both Vista, OS X and GNOME, it's preferred to use a verb instead of a standard button labeled OK. 

I would suggest something like "Create", "Create folder", "Create new saved search" (a bit too long maybe).
Bryan, what would you suggest here?
I tend to go shorter when the item is complex, so I'd suggest 'Create'
Assignee: nisses.mail → nobody
Severity: normal → trivial
Component: General → Mail Window Front End
QA Contact: general → front-end
philor, have time for this?
Attachment #425663 - Flags: review?(philringnalda)
Edmund, maybe you want to take this one over as well?  The patch needs some work to make an accesskey happen
(In reply to comment #4)
> Edmund, maybe you want to take this one over as well?  The patch needs some
> work to make an accesskey happen


Sure.
Assignee: nobody → edmund
Status: NEW → ASSIGNED
(In reply to comment #4)
> Edmund, maybe you want to take this one over as well?  The patch needs some
> work to make an accesskey happen

Btw, I'm using "R" as the accesskey.  It looks ok, though confirmation
from you or anyone would be appreciated.
Comment on attachment 425663 [details] [diff] [review]
changes accept button to use the verb "Create"

removing this review for now since Edmund is working on a new patch
Attachment #425663 - Flags: review?(philringnalda)
(In reply to comment #6)
> (In reply to comment #4)
> > Edmund, maybe you want to take this one over as well?  The patch needs some
> > work to make an accesskey happen
> 
> Btw, I'm using "R" as the accesskey.  It looks ok, though confirmation
> from you or anyone would be appreciated.

R sounds good, I'm assuming C will already be taken by the Cancel
Attachment #432106 - Attachment description: Changed Ok button to Create, with R as access key. → Changed Ok button to Create, with R as access key and added License block to virtualFolderProperties.dtd.
Comment on attachment 432106 [details] [diff] [review]
Changed Ok button to Create, with R as access key and added License block to virtualFolderProperties.dtd.

I had a quick discussion with Bryan about this over irc.

The unfortunate side-effect of this patch is that for editing saved searches, the button to accept will also be called "Create". Which, of course, isn't quite right for editing.

Bryan and I suggest using "Update" for the edit case.
Attachment #432106 - Flags: review?(bugzilla) → review-
(In reply to comment #10)
> (From update of attachment 432106 [details] [diff] [review])
> I had a quick discussion with Bryan about this over irc.
> 
> The unfortunate side-effect of this patch is that for editing saved searches,
> the button to accept will also be called "Create". Which, of course, isn't
> quite right for editing.
> 
> Bryan and I suggest using "Update" for the edit case.

Makes sense.   I'll have the patch done soon.  

While I'm at it, what about the part about the license block?  Should I
also include it or leave it out?
This patch is without the license block addition.
Attachment #432106 - Attachment is obsolete: true
Attachment #435347 - Flags: review?(bugzilla)
Comment on attachment 435347 [details] [diff] [review]
Changed Ok button to Update, with U as access key.

Unfortunately I don't think this is quite what we intended. I think this dialog needs to be "Create" when creating a new folder, and "Update" when editing an existing one. Hence you need two labels and to switch between them according to how the dialog is opened.

We do do this in other places.
Attachment #435347 - Flags: review?(bugzilla) → review-
The button becomes Create if it's from a "New" instruction.  It becomes
"Update" if it is from a "Properties" instruction.
Attachment #425663 - Attachment is obsolete: true
Attachment #435347 - Attachment is obsolete: true
Comment on attachment 444109 [details] [diff] [review]
Changed the Smart Folder button to reflect its function depending on how the Smart Folder is opened.

(In reply to comment #14)
> Created an attachment (id=444109) [details]
> Changed the Smart Folder button to reflect its function depending on how the
> Smart Folder is opened.
> 
> The button becomes Create if it's from a "New" instruction.  It becomes
> "Update" if it is from a "Properties" instruction.

This looks pretty close!  I think you just need to make those strings localizable.

Take a look at the code in this file:
http://mxr.mozilla.org/comm-central/source/mail/extensions/mailviews/content/mailViewList.js#89

You need a string bundle like this:

  var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"].
      getService(Components.interfaces.nsIStringBundleService);
  var bundle = strBundleService.createBundle("chrome://messenger/locale/messenger.properties");

Then you could set the label like this:

document.documentElement.getButton("accept").label = bundle.GetStringFromName("editExistingFolderAcceptLabel")

Which allows for labels of different languages inside the javascript code.
(In reply to comment #15)

> This looks pretty close!  I think you just need to make those strings
> localizable.
> 
> You need a string bundle like this:
> 
>   var strBundleService =
> Components.classes["@mozilla.org/intl/stringbundle;1"].
>       getService(Components.interfaces.nsIStringBundleService);
>   var bundle =
> strBundleService.createBundle("chrome://messenger/locale/messenger.properties");
> 
> Then you could set the label like this:
> 
> document.documentElement.getButton("accept").label =
> bundle.GetStringFromName("editExistingFolderAcceptLabel")
> 
> Which allows for labels of different languages inside the javascript code.

Just wondering.  What about the accesskey?  Do they need some sort
of localizable setting too?
Actually what we tend to do in situations like this is to define the labels and access keys in the dtd file and reference them in attributes of the dialog.

So something like

existingAcceptLabel="&editExistingFolderAccept.label;"
newAcceptLabel="&newFolderAccept.label;"

and then in the js just use getButton("accept").label = dialog.newAcceptLabel

(the code isn't quite right, but hopefully you get the idea).
Attachment #444109 - Flags: feedback?(bugspam.Callek)
Attachment #444411 - Flags: review?(iann_bugzilla)
Attachment #444411 - Flags: feedback?(bugspam.Callek)
Attachment #444109 - Flags: feedback?(bugspam.Callek)
Attachment #444411 - Flags: ui-review?(clarkbw)
Attachment #444411 - Flags: superreview?(bugzilla)
Comment on attachment 444411 [details] [diff] [review]
 Changed the Smart Folder button to reflect its function depending on how the Smart Folder is opened.  (v2)

Code looks good, one (very minor nit: set the accept label to Update from the start, so that something more appropriate than "OK" shows in the case of a slow computer not running onload faster than the human eye scans; -- don't change this unless a "real" reviewer wants it though)
Attachment #444411 - Flags: feedback?(bugspam.Callek) → feedback+
Comment on attachment 444411 [details] [diff] [review]
 Changed the Smart Folder button to reflect its function depending on how the Smart Folder is opened.  (v2)

>+++ b/mailnews/base/content/virtualFolderProperties.js
I'd prefer setting a variable to document.documentElement.getButton("accept") then using that in the if statement.
>   if (arguments.editExistingFolder)
>+  {
>+    document.documentElement.getButton("accept").label = 
>+        document.documentElement.getAttribute("editFolderAcceptButtonLabel");
>+    document.documentElement.getButton("accept").accesskey = 
>+        document.documentElement.getAttribute("editFolderAcceptButtonAccessKey");
>     InitDialogWithVirtualFolder(arguments.folder);
>+  }
>   else // we are creating a new virtual folder
>   {
>+    document.documentElement.getButton("accept").label = 
>+        document.documentElement.getAttribute("newFolderAcceptButtonLabel");
>+    document.documentElement.getButton("accept").accesskey = 
>+        document.documentElement.getAttribute("newFolderAcceptButtonAccessKey");

so r=me with the above addressed.
Attachment #444411 - Flags: review?(iann_bugzilla) → review+
r+IanN, f+Callek
Attachment #444411 - Attachment is obsolete: true
Attachment #445953 - Flags: review+
Attachment #445953 - Flags: feedback+
Attachment #444411 - Flags: ui-review?(clarkbw)
Attachment #444411 - Flags: superreview?(bugzilla)
Attachment #445953 - Flags: ui-review?(clarkbw)
Attachment #445953 - Flags: superreview?(bugzilla)
Comment on attachment 445953 [details] [diff] [review]
 Changed the Smart Folder button to reflect its function depending on how the Smart Folder is opened.  (v3)

Looks great, thanks.
Attachment #445953 - Flags: superreview?(bugzilla) → superreview+
Attachment #445953 - Flags: ui-review?(clarkbw) → ui-review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/b5afb60824f3

Thanks Edmund.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: New Smart Folder dialog don't use a verb for the confirm button → New Smart Folder dialog doesn't use a verb for the confirm button
Target Milestone: --- → Thunderbird 3.2a1
You need to log in before you can comment on or make changes to this bug.