IMO, 4 kinds of widget need accesskey: 1) textbox, accesskey in its label 2) button, accesskey in the button text 3) color button, accesskey in its label 4) menulist/combo box, accesskey in its label I'll provide a completed list for the missed accesskeys.

Hello, I don't understand the bug summary. Do you mean that there are accesskeys missing within preference dialogs? Or are you talking about the need to always have the same letter for the preference menu item's accesskey?

We need accesskeys for the following widgets: Appearance/Themes, "Uninstall Theme" button; Appearance/Languages, dup "d" of "Download More..." button; Navigator/Languages, "Add" & "Remove" button, "Default char coding" menulist; Navigator/Help Apps, "When encountered" textbox; Navigator/Smart Browsing, "Advanced" button; Composer, "Maximum number of pages" textbox; Composer/New Page, "Author" textbox, "Use custom colors" 5 buttons, "g" for "Background image" doesn't work; MailNews/Notification, "Browse" & "Preview" button; MailNews/Return Receipts, "Allow return receipts for some messages" 3 menulists; Privacy/Cookies, textbox before "days"; Privacy/Popup, 2 "Exceptions" buttons; Privacy/Master Passwrods, "Change Password" & "Reset Password" buttons, textbox before "minutes or longer"; Privacy/SSL, "Edit Ciphers" button; Privacy/Certificates, "Manage..." 2 buttons; Privacy/Validation, "Manage CRLs" button, "Response Signer" menulist, "Service URL" textbox; Chatzilla, "What do these mean" button; Chatzilla/Startup, all 9 buttons; Advanced/Cache, "Cache Folder" textbox; Advanced/Mouse Wheel, textbox before "lines" Advanced/DOM inspector, "Border Color" color button, following 3 textboxes, "Install" button;

sorry, aaron, I overwrote your changes :( I meant the first point "there are accesskeys missing within preference dialogs".

Taking...

This will help you choose what letters to use: http://www.mozilla.org/projects/ui/accessibility/accesskey.html

Kyle, re comment 2, theres a couple there that don't apply: Appearance/Languages, dup "d" of "Download More..." button: both instances of this button do exactly the same thing and share the same entity (iirc) for the label and accesskey. (this is bad imo, but not an accesskey problem:) Navigator/Languages "Add" & "Remove" button, "Default char coding" menulist: These are fixed by my patch in bug 191160, which is ready to go in when the tree reopens for 1.4a (unless someone wants to put it in for 1.3final :) Navigator/Help Apps, "When encountered" textbox: This is a read only textbox - it dosn't need an accesskey (unless i'm wrong) Advanced/Cache, "Cache Folder" textbox: As above. Chatzilla, "What do these mean" button: Look here and wonder at the appalling ui design... :( I also noticed: Chatzilla/Interface , "Limit private message tabs to" textbox Chatzilla/Sounds, All listboxes. I presume you are just attacking the main prefs panel -- there are loads of accesskeys missing from linked dialogs (like Privacy -> Certs -> Manage Certs) Hope this helps :).

Piers,what you mentioned in comment 6: accesskeys for "Chatzilla/Sounds, All listboxes." are there, which are Sou_n_d on new Private Message Tab, etc. So sum up you and kyle's comments: accesskeys are needed for the following widgets: Appearance/Themes, "Uninstall Theme" button; Navigator/Smart Browsing, "Advanced" button; Composer, "Maximum number of pages" textbox; Composer/New Page, "Author" textbox, "Use custom colors" 5 buttons, "g" for "Background image" doesn't work; MailNews/Notification, "Browse" & "Preview" button; MailNews/Return Receipts, "Allow return receipts for some messages" 3 menulists; Privacy/Cookies, textbox before "days"; Privacy/Popup, 2 "Exceptions" buttons; Privacy/Master Passwrods, "Change Password" & "Reset Password" buttons, textbox before "minutes or longer"; Privacy/SSL, "Edit Ciphers" button; Privacy/Certificates, "Manage..." 2 buttons; Privacy/Validation, "Manage CRLs" button, "Response Signer" menulist, "Service URL" textbox; Chatzilla/Startup, all 9 buttons; Chatzilla/Interface , "Limit private message tabs to" textbox; Advanced/Mouse Wheel, textbox before "lines" Advanced/DOM inspector, "Border Color" color button, following 3 textboxes, "Install" button; And there is another bug 193265 for accesskeys missing from linked dialogs to preference main dialog.

>Appearance/Languages, dup "d" of "Download More..." button: both instances of >this button do exactly the same thing and share the same entity (iirc) for the >label and accesskey. (this is bad imo, but not an accesskey problem:) This is a bad UI design, though they are doing the some thing. We either merge the two button to one, or give them different accesskeys. >Navigator/Languages "Add" & "Remove" button, "Default char coding" menulist: > These are fixed by my patch in bug 191160, which is ready to go in when the >tree reopens for 1.4a (unless someone wants to put it in for 1.3final :) Yes, I noticed that, so I marked your fix for 189496, 191160 and 191169 block this bug. We won't do the duplicate work on those :) >Navigator/Help Apps, "When encountered" textbox: This is a read only textbox - >it dosn't need an accesskey (unless i'm wrong) > >Advanced/Cache, "Cache Folder" textbox: As above. Oh, that's true. But I still wonder why not use a <description> instead of a read-only textbox? >Chatzilla, "What do these mean" button: Look here and wonder at the appalling >ui design... :( >I also noticed: >Chatzilla/Interface , "Limit private message tabs to" textbox We do not give any accesskeys for the checkbox, so, imo, we should keep this rule there. >Chatzilla/Sounds, All listboxes. there are accesskeys on the groupboxes. >I presume you are just attacking the main prefs panel -- there are loads of >accesskeys missing from linked dialogs (like Privacy -> Certs -> Manage Certs) I want to fix the *all* missing accesskeys for pref dialogs, included the linked dialogs. Jessie, could you help on this, too? >Hope this helps :). Really helpful :)

Hi Aaron, When I am fixing this bug, I found many of radiobox and checkbox have implemented accesskey and they can work well if you know which key is the accesskey. But why the accesskey is not underlined? Is this a bug or a known issue? For example, In preference main dialog, click Composer-New Page Setting: click Alt+c, is the accesskey for the "Use Custom Colors:" the xul file is like this: <radiogroup id="useCustomColors" preftype="bool" prefstring="editor.use_custom_colors"> <radio id="DefaultColorsRadio" value="false" label="&defaultColorsRadio.label;" accesskey="&defaultColors.accesskey;" oncommand="UseDefaultColors();"/> <radio id="CustomColorsRadio" value="true" label="&customColorsRadio.label;" accesskey="&customColors.accesskey;" oncommand="UseCustomColors()"/> </radiogroup> dtd file: <!ENTITY customColorsRadio.label "Use custom colors:"> <!ENTITY customColors.accesskey "c"> Thanks, Jessie

>This is a bad UI design, though they are doing the some thing. We either merge >the two button to one, or give them different accesskeys. Yep -- it may be good to use the same "web page-ish" links like on Apperance -> Themes ("Get New Themes"). >Oh, that's true. But I still wonder why not use a <description> instead of a >read-only textbox? Possibly because the text can often be longer than the width of the box (i.e. to make it obvious that you can select and drag to see the whole contents) >We do not give any accesskeys for the checkbox, so, imo, we should keep this >rule there. We do give accesskeys in some cases -- they just aren't shown for checkboxes and radio buttons (this is a major blocker to accessability imo) - bug 68841. Re: comment 0, we _should_ provide: 5) checkbox, accesskey in its label 6) radiobutton, accesskey in its label ...but there's no point in doing them yet :(

Yeah, a "Download More Language..." link is a good idea. People need a consistent experience when they clicked a button. We also noticed the problem with accesskeys for checkbox and radiobutton (I omitted those kinds of widgets when I wrote comment 0), Aaron has already had a huge patch against this, but seems nobody wants to review it. Aaron, any chance to push it got fixed? This is really a big accessibility/keynav issue.

Comment on attachment 114740 [details] [diff] [review] patch >+++ editor/ui/composer/locale/en-US/pref-composer.dtd 17 Feb 2003 08:40:30 -0000 >@@ -33,7 +33,9 @@ > <!ENTITY chooseButton.label "Choose"> > > <!ENTITY recentFiles.title "Recent Pages Menu"> >+<!ENTITY documentsInMenu.accesskey "n"> please make this line up ^ > <!ENTITY documentsInMenu "Maximum number of pages listed:"> >+ ^ don't add this line > > <!ENTITY cssEditing.label "Cascading Style Sheets (CSS) Editing"> > <!ENTITY useCSS.label "Use CSS styles instead of HTML elements and attributes"> >+++ extensions/content-packs/resources/content/pref-contentpacks.xul 17 Feb 2003 08:45:57 -0000 >@@ -240,7 +240,7 @@ > <vbox> > <button id="downloadLanguagePacks" > label="&downloadContentPacks.label;" make this ^ downloadLanguagePacks.label and clone the item in the dtd >- accesskey="&downloadContentPacks.accesskey;" >+ accesskey="&downloadLanguagePacks.accesskey;" > oncommand="DownloadRegionPacks()"/> > </vbox> > </hbox> >+++ extensions/cookie/resources/content/pref-popups.xul 17 Feb 2003 08:45:57 -0000 >@@ -155,6 +155,7 @@ > oncommand="setButtons()"/> > <spacer flex="1"/> > <button id="exceptionsAllow" label="&popupView.label;" >+ accesskey="&popupView.accesskey;" > oncommand="viewPopups();"/> > </hbox> > <hbox> >@@ -162,6 +163,7 @@ > oncommand="setButtons()"/> > <spacer flex="1"/> > <button id="exceptionsBlock" label="&popupView.label;" >+ accesskey="&popupView_2.accesskey;" > oncommand="viewPopups();"/> .. recycling labels is bad... how about popupViewAllow.* popupViewBlock.* please drop the inspector patch, there's a localization patch for inspector which should land first. split out the chatzilla patch and get rginda to approve it. again, _2 isn't a syntax i've seen in mozilla ...

Hi timeless, >+++ extensions/content-packs/resources/content/pref-contentpacks.xul 17 Feb 2003 08:45:57 -0000 >@@ -240,7 +240,7 @@ > <vbox> > <button id="downloadLanguagePacks" > label="&downloadContentPacks.label;" make this ^ downloadLanguagePacks.label and clone the item in the dtd >- accesskey="&downloadContentPacks.accesskey;" >+ accesskey="&downloadLanguagePacks.accesskey;" > oncommand="DownloadRegionPacks()"/> > </vbox> > </hbox> Like comment8 mentioned, this is a bad UI design, though they are doing the some thing. We either merge the two button to one, or give them different accesskeys. So I gave them different accesskeys. >please drop the inspector patch, there's a localization patch for >inspector which should land first. Could you tell me the bug id for inspector, I will drop this part. >split out the chatzilla patch and get rginda to approve it. Ok, I will file another bug to block this bug, and ask rginda to approve it. Thanks for your efforts!

localizable inspector is bug 109891 i'm aware of the discussion about the duplicate text. imo if you aren't going to move away from label you should split the entities for the two labels as i suggested. otherwise if some localization needs to give different strings for the two buttons, it can't, which is bad.

Comment on attachment 114740 [details] [diff] [review] patch I promise to modify the patch as timeless suggested. Jag, please sr it for me. Thanks!

Could you attach the new patch? I'll sr that one.

Comment on attachment 115138 [details] [diff] [review] new patch sr=jag

just checked in trunk

Comment on attachment 115138 [details] [diff] [review] new patch not 1.3 material.

i found a couple small issues, but rather than reopening this, i've spun off new bugs: bug 195784 and bug 195785. vrfy'd fixed on win2k and linux rh8.0 (2003.03.03.05 comm trunk), based on the fixes in attachment 115138 [details] [diff] [review].