Closed Bug 322247 Opened 14 years ago Closed 11 years ago

Various accesskeys missing in the main interface and prefs.

Categories

(SeaMonkey :: MailNews: Message Display, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a3

People

(Reporter: vhaarr+bmo, Assigned: wladow)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

* The entire Tools->Import dialog (convert to wizard?)
 * Preferences->Notifications/
    - "Custom .wav file" could focus the textbox.
 * Preferences->Labels/
    - Could add XUL labels with "1"-"5" and numbers as accesskeys.
 * <Account>/
     Drafts folder/
       <some message>/
         "Edit Draft..."-button.
         Attachments-pane/
           Pane itself needs a key.
           Context menu on item/
             "Detach ..." menu item (also correct ellipses; no space).
             "Delete" menu item.
         Context menu in body/
           "Print Preview" menu item.
Attached patch add accesskeys, v1 (obsolete) — Splinter Review
(In reply to comment #0)
> * The entire Tools->Import dialog (convert to wizard?)
this patch adds accesskeys for the all items except navigation buttons, those will be replaced by <wizard>'s default buttons once Import dialog is converted to use <wizard> widget - Bug 101874

>  * Preferences->Notifications/
>     - "Custom .wav file" could focus the textbox.
fixed when prefpanes migrated to <preferences>

>  * Preferences->Labels/
>     - Could add XUL labels with "1"-"5" and numbers as accesskeys.
insufficient, still no way to bring up Select Color dialog. I think we can live without accesskeys here 

>  * <Account>/
>      Drafts folder/
>        <some message>/
>          "Edit Draft..."-button.
Do we want this? Included in the patch through, it seems to be usable. I'll remove it, if we don't want it here. The Shredder guys don't use accesskeys for message header buttons.

>          Attachments-pane/
>            Pane itself needs a key.
The same question as above. Included in the patch too, works fine.
 
>            Context menu on item/
>              "Detach ..." menu item (also correct ellipses; no space).
>              "Delete" menu item.
>          Context menu in body/
>            "Print Preview" menu item.
Already fixed

Besides these the patch is adding:
- accesskeys for Back and Forward menuitems in SeaMonkey Mail -> Go menu
- accesskey for "Download messages now" option in Account Wizard

Asking Phil to review mail part and Neil the rest of the patch. thx
Assignee: mail → wladow
Status: NEW → ASSIGNED
Attachment #355331 - Flags: superreview?(neil)
Attachment #355331 - Flags: review?(philringnalda)
Comment on attachment 355331 [details] [diff] [review]
add accesskeys, v1

Looks reasonable to me but I think Mnyromyr should see the patch too.
Attachment #355331 - Flags: superreview?(neil)
Attachment #355331 - Flags: superreview+
Attachment #355331 - Flags: review?(mnyromyr)
Comment on attachment 355331 [details] [diff] [review]
add accesskeys, v1

>diff --git a/mail/locales/en-US/chrome/messenger/importDialog.dtd b/mail/locales/en-US/chrome/messenger/importDialog.dtd
>+<!ENTITY importAll.label          "Import everything">
>+<!ENTITY importAll.accesskey      "I">

Uppercase I (and lowercase l or a simple 1) are extremely bad choices for accesskeys.
'e' is a more logical choice here.

>diff --git a/mailnews/base/prefs/resources/content/AccountWizard.xul b/mailnews/base/prefs/resources/content/AccountWizard.xul
>+        <checkbox id="downloadMsgs" hidden="true" label="&downloadOnLogin.label;" checked="true"
>+                  accesskey="&downloadOnLogin.accesskey;"/>

If attributes don't fit onto a single line, each line should have just one attribute, with all attributes aligned vertically. The attributes should be roughly ordered id-label/value-accesskey-key-other_attributes-js_code_attributes. (See the menuitems in mailWindowOverlay.xul already in your patch.)
This comment holds for almost all of your XUL changes, so I won't repeat them individually.

>diff --git a/suite/locales/en-US/chrome/mailnews/importDialog.dtd b/suite/locales/en-US/chrome/mailnews/importDialog.dtd
>+<!ENTITY importAll.label          "Import everything">
>+<!ENTITY importAll.accesskey      "I">

See above, use 'e'.

>diff --git a/suite/locales/en-US/chrome/mailnews/msgHdrViewOverlay.dtd b/suite/locales/en-US/chrome/mailnews/msgHdrViewOverlay.dtd
> <!ENTITY editMessage.label "Edit Draft…">
>+<!ENTITY editMessage.accesskey "D">
> 
> <!ENTITY attachmentsTree.label             "Attachments:">
>+<!ENTITY attachmentsTree.accesskey         "A">

Vertically align the edit entities with those following.
Attachment #355331 - Flags: review?(mnyromyr) → review-
(In reply to comment #3)
>(From update of attachment 355331 [details] [diff] [review])
>>diff --git a/suite/locales/en-US/chrome/mailnews/importDialog.dtd b/suite/locales/en-US/chrome/mailnews/importDialog.dtd
>>+<!ENTITY importAll.label          "Import everything">
>>+<!ENTITY importAll.accesskey      "I">
>See above, use 'e'.
And change the case of the label to "Import Everything" perhaps?
All comments addressed.

> If attributes don't fit onto a single line, each line should have just one
> attribute, with all attributes aligned vertically. The attributes should be
> roughly ordered
> id-label/value-accesskey-key-other_attributes-js_code_attributes. (See the
> menuitems in mailWindowOverlay.xul already in your patch.)

Please, guys, to make this easier for everyone: have a session, make a mandatory decision on how to wrap these attributes and ask everyone to follow it. Because it's extremely confusing when different reviewers are asking me to wrap this in different ways. Thx.
Attachment #355331 - Attachment is obsolete: true
Attachment #357592 - Flags: superreview?(neil)
Attachment #357592 - Flags: review?(mnyromyr)
Attachment #355331 - Flags: review?(philringnalda)
Attachment #357592 - Flags: review?(philringnalda)
Attachment #357592 - Flags: superreview?(neil) → superreview+
Comment on attachment 357592 [details] [diff] [review]
add accesskeys, v2
[Checkin: Comment 7]

> Please, guys, to make this easier for everyone: have a session, 
> make a mandatory decision on how to wrap these attributes and 
> ask everyone to follow it. Because it's extremely confusing when
> different reviewers are asking me to wrap this in different ways.

Well, basically, we have just two set rules:
1. Mozilla Coding Style Guidelines, which don't help for XUL and 
   are superimposed by
2. When in Rome, do as the Romans do, iow fit into a file's prevalent
   style.

MailNews in particular has the problem of being just the outskirts
of the Empire, with Barbarians running all around, bringing along
their own special ways and such...
We try to work towards a common style, but sadly not everybody 
cares and (especially with coding style) not everybody agrees upon 
the target...

The overall principle of source code styling should be *readability* -
there's one writing it but hundreds reading it!
Attachment #357592 - Flags: review?(mnyromyr) → review+
Attachment #357592 - Flags: review?(philringnalda) → review+
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.0a3
Comment on attachment 357592 [details] [diff] [review]
add accesskeys, v2
[Checkin: Comment 7]


http://hg.mozilla.org/comm-central/rev/5667e365df4b
Attachment #357592 - Attachment description: add accesskeys, v2 → add accesskeys, v2 [Checkin: Comment 7]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
QA Contact: message-display
Resolution: --- → FIXED
Blocks: 537221
You need to log in before you can comment on or make changes to this bug.