Closed
Bug 322247
Opened 19 years ago
Closed 16 years ago
Various accesskeys missing in the main interface and prefs.
Categories
(SeaMonkey :: MailNews: Message Display, defect)
SeaMonkey
MailNews: Message Display
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)
16.98 KB,
patch
|
mnyromyr
:
review+
philor
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
* 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.
Assignee | ||
Comment 1•16 years ago
|
||
(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 2•16 years ago
|
||
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 3•16 years ago
|
||
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-
Comment 4•16 years ago
|
||
(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?
Assignee | ||
Comment 5•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #357592 -
Flags: review?(philringnalda)
Updated•16 years ago
|
Attachment #357592 -
Flags: superreview?(neil) → superreview+
Comment 6•16 years ago
|
||
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+
Updated•16 years ago
|
Attachment #357592 -
Flags: review?(philringnalda) → review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.0a3
Comment 7•16 years ago
|
||
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]
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
QA Contact: message-display
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•