193068 - [meta] Missing accesskeys in preference dialogs

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Kyle Yuan]

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.

Comment 1

[Aaron Leventhal]

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?

Comment 2

[Kyle Yuan]

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;

Comment 3

[Kyle Yuan]

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

Comment 4

[Jessie Li]

Taking...

Comment 5

[Aaron Leventhal]

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

Comment 6

[piers]

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 :).

Comment 7

[Jessie Li]

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.

Comment 8

[Kyle Yuan]

>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 :)

Comment 9

[Jessie Li]

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

Comment 10

[piers]

>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 :(

Comment 11

[Kyle Yuan]

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 12

[Jessie Li]

Attachment: patch

Comment 13

[timeless]

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 ...

Comment 14

[Jessie Li]

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!

Comment 15

[timeless]

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 16

[Jessie Li]

Comment on attachment 114740 [details] [diff] [review]
patch

I promise to modify the patch as timeless suggested.
Jag, please sr it for me. Thanks!

Comment 17

[jag (Peter Annema)]

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

Comment 18

[Jessie Li]

Attachment: new patch

Comment 19

[jag (Peter Annema)]

Comment on attachment 115138 [details] [diff] [review]
new patch

sr=jag

Comment 20

[Pete Zha]

just checked in trunk

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 115138 [details] [diff] [review]
new patch

not 1.3 material.

Comment 22

[sairuh (rarely reading bugmail)]

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].