191642 - Lots of missing keyboard accelerators (accesskeys) in Options window

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[David Tenser [:djst]]

The Options (formerly Preferences) window is missing lots of keyboard
accelerators (mnemonic shortcuts) for the controls. For example, "Open Links in
the Background" could use "O" as a keyboard accelerator.

This problem is appearent in all Options panels and is a fairly important
accessibility issue.

Comment 1

[Sébastien Delahaye]

blocking bug 191524

Comment 2

[alanjstr]

The keyboard accelerator O is now used for the Options panel (formerly known as
preferences).

Comment 3

[David Tenser [:djst]]

Alan, this bug report is not about a keyboard accelerator for the "Options"
entry in the main menu, it's for the controls in the actual Options window.

Comment 4

[Chu Alan "sprite"]

Attachment: my first trial to add accesskey to General pane (pref-navigator.xul)

i have noticed several problems when creating this patch :-

1. accesskey for checkboxes and radios are not underlined. (bug 68841)
2. i can use accesskey to access the browse button even if it is disabled.
3. the browse button is not disabled on start up even if 'Ask me where...' is
enabled.

should i file a new bug for issue 2 and 3?

Comment 5

[Chu Alan "sprite"]

p.s. does those big button on the left hand side needs accesskey?

Comment 6

[David Tenser [:djst]]

> p.s. does those big button on the left hand side needs accesskey?
I'd say it's not needed, although it would be very nice to be able to cycle
through the panels using [Shift]+Ctrl+Tab, as if they were tabs. But that's a
totally separate RFE.

Comment 7

[Chu Alan "sprite"]

are there any problem with my patch?
if no, i will continue to patch other files.

Comment 8

[Simon Paquet [:sipaq]]

Chu, the patch looks good. Have you asked Ben, if he could check it in?

Comment 9

[Chu Alan "sprite"]

Attachment: add accesskey to General pane (pref-navigator.xul)

same patch, updated to current CVS.

Comment 10

[Chu Alan "sprite"]

Comment on attachment 119161 [details] [diff] [review]
add accesskey to General pane (pref-navigator.xul)

Simon: I just need to ask ben to review it?

Comment 11

[Chu Alan "sprite"]

ben:    I will continue to work on other panes after you have commented on the
patch.
others: sorry for the spam.

Comment 12

[piers]

1 note about your patch: 

>  <!ENTITY  background.label          "Open Links in the background">
> +<!ENTITY  background.accesskey      "p">

Try to avoid using letters with decenders (like p, g, q, or y) because it makes 
the underlining hard to see. Have a look at: 
http://www.mozilla.org/projects/ui/accessibility/accesskey.html
if you haven't already :)

Otherwise, patch looks fine.

Comment 13

[Chu Alan "sprite"]

i am surprised to know the patch still applies without problem, I am not sure if
I should change the access key 'p' as stated in comment 12, since I think p is a
good candidate. I will let the developer to decide it :)

BTW are there reviewer available for review?

Comment 14

[Mike Connor [:mconnor]]

taking QA contact, sorry about the bugspam

Comment 15

[Michael Lefevre]

Comment on attachment 119161 [details] [diff] [review]
add accesskey to General pane (pref-navigator.xul)

moving some easy reviews to mconnor who should be getting to them faster than
Ben

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 119161 [details] [diff] [review]
add accesskey to General pane (pref-navigator.xul)

bitrotted pretty badly

will attach a fix for this later today.

Comment 17

[Brendan Eich [:brendan]]

Sounds like mconnor's back on this.  Worth getting for PR1?

/be

Comment 18

[chris hofmann]

mconnor, any update?  getting lat for PR

Comment 19

[Mike Connor [:mconnor]]

So here's the short, ugly, bad version.  XUL accesskeys on radio/checkbox
elements  don't show up.  We actually do have accesskeys in place for most/all
of these elements, but we'd have to refactor a ton of stuff to get it to work,
short of fixing bug 68841.

I'm sort of torn on this.  While we're not making stuff as accessible as we
could, tab navigation still works fine, and button accesskeys work fine too.  If
we refactor the bindings and/or XUL to get this working, we'll have to test
really well to ensure we haven't regressed the world for the sake of making 1.0
more conveniently accessible.  I have plans to refactor a lot of code to make
instant-apply work on GNOME/OS X, but I'm not at all comfortable with changing
things at this juncture.

What I'm thinking at this point is that we've been broken on this subject since
before Mozilla 1.0, so pushing this past Firefox 1.0 isn't going to kill us. 
I'd love to see some traction on bug 68841, since it basically makes XUL look
broken if we define accesskeys that don't show up.  Its also a barrier to
developing more accessible apps, since there's no practical reason to define
invisible accesskeys.

I will try to take a stab at this when I do the big refactoring, but knocking
off the 1.0 list as I don't think the fix for 68841, even if it were available
tomorrow, is necessarily going to be "safe" enough.

Comment 20

[Aaron Leventhal]

I have a new patch posted for bug 68841 that uses XBL to underlined accesskeys
in XUL checkboxes and radio buttons. It's relatively safe compared to other
approaches (like hacking core layout).

Comment 21

[Nickolay_Ponomarev]

*** Bug 259183 has been marked as a duplicate of this bug. ***

Comment 22

[Mike Connor [:mconnor]]

Are you nominating based on the new prefwindow or the old?

Comment 23

[Aaron Leventhal]

(In reply to comment #22)
> Are you nominating based on the new prefwindow or the old?

The new one is obviously the only one we care about. If it gets checked in and
this problem is solved, then we can just mark this FIXED then.

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

I added a bunch of accesskeys but there still may be shortfalls. Can someone
come up with a list of missing ones?

Comment 25

[Nickolay_Ponomarev]

There are no acesskeys for Allowed Sites / Exceptions buttons in the Content
tab, and on the Tabs and Downloads. Maybe more, I didn't check in subdialogs.

Some existing accesskeys violate
http://www.mozilla.org/access/keyboard/accesskey , the "Make it easy to see"
section, p.3

Also the Privacy tab suffers from bug 143065.

Comment 26

[Nickolay_Ponomarev]

from bug 284310:
>Accelerator keys are also missing for the Sanitize dialog box and also for the
>buttons in all security dialog boxes (Certificate Manager, View CRLs, etc).

Comment 27

[Nickolay_Ponomarev]

*** Bug 284310 has been marked as a duplicate of this bug. ***

Comment 28

[Jo Hermans]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050629
Firefox/1.0+

Privacy & Advanced Tabs don't have shortcuts
Privacy->Remember doesn't work
Content->Fonts->Advanced doesn't have shortcuts for proprotional & sizes
Tabs doesn't have shortcuts at all
Downloads doesn't have shortcuts at all (except the view button)
Advanced->General->Accesibility shortcuts don't work (isn't that ironic ?)
Advanced->General->Browsing->Autoscrolling doesn't work

Comment 29

[Chris Beard]

Do we really need to get this in before we branch for 1.8?  Is anyone working on
it?  If not, who should?

/cb

Comment 30

[Benjamin Smedberg]

cb, it affects l10n significantly, and that was a criterion for branching

Comment 31

[:Gavin Sharp [email: [e-mail]]]

*** Bug 300697 has been marked as a duplicate of this bug. ***

Comment 32

[Bryan]

Attachment: Comprehensive patch to address additional accesskey issues in Tools-> Options

Comprehensive patch to address additional accesskey issues in Tools-> Options. 
I've left out my fixes for Tools-> Options-> Advanced-> Security per Gavin. 
I'll attach those at a later date.

Comment 33

[Aaron Leventhal]

Comment on attachment 190287 [details] [diff] [review]
Comprehensive patch to address additional accesskey issues in Tools-> Options

I'm going to go ahead and minus this for mconnor because I already see that it
doesn't follow the guidelines at
http://www.mozilla.org/access/keyboard/accesskey -- which you may not have
known about.

Comment 34

[Bryan]

(In reply to comment #33)
> (From update of attachment 190287 [details] [diff] [review] [edit])
> I'm going to go ahead and minus this for mconnor because I already see that it
> doesn't follow the guidelines at
> http://www.mozilla.org/access/keyboard/accesskey -- which you may not have
> known about.
> 
Aaron, I made every attempt to follow those guidelines but due to BUG 143065 I
found myself following the guidelines but having accesskeys that didn't work.  A
great example is where I used letters that have descenders or where I used
letters that are only one pixel wide.  I had no choice, again because of BUG
143065.  I suppose I should have asked about deviating away from the guidelines
before completing the patch.  I would be happy to correct those "deviations" if
we can get BUG 143065 fixed for 1.5.  Please advise!

Comment 35

[Aaron Leventhal]

Comment on attachment 190287 [details] [diff] [review]
Comprehensive patch to address additional accesskey issues in Tools-> Options

Okay. We'll have to circle back and fix these in a later version. Thanks for
doing the work despite bug 143065.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 190287 [details] [diff] [review]
Comprehensive patch to address additional accesskey issues in Tools-> Options

>Index: browser/base/content/sanitize.xul
>     <groupbox orient="vertical" align="start">
>       <checkbox id="promptOnSanitize" label="&promptOnSanitize.label;"
>                 preference="privacy.sanitize.promptOnSanitize"/>

>Index: browser/locales/en-US/chrome/browser/preferences/sanitize.dtd
>+<!ENTITY promptOnSanitize.accesskey   "k">

The accesskey was added to the dtd, but not referenced. I've fixed it in my
tree.

>Index: browser/components/preferences/fonts.xul
>Index: browser/locales/en-US/chrome/browser/preferences/fonts.dtd

I've tweaked a few here too, to better follow the general accesskey guidelines.

>Index: browser/locales/en-US/chrome/browser/preferences/permissions.dtd

> <!ENTITY removeallpermissions.label   "Remove All Sites">
>+<!ENTITY removeallpermissions.accesskey "A">

> <!ENTITY allow.label                  "Allow">
>+<!ENTITY allow.accesskey              "l">

I switched these two ("A" for "Allow") since that's probably used more often.

>Index: browser/components/preferences/tabs.xul
>Index: browser/locales/en-US/chrome/browser/preferences/tabs.dtd

>+<!ENTITY hideTabBar.label                    "Hide the tab bar when only one web site is open">
>+<!ENTITY hideTabBar.accesskey                "i">

Changed the "i" to "d".

>Index: browser/locales/en-US/chrome/browser/preferences/connection.dtd

> <!ENTITY  reload.label                  "Reload">
> <!ENTITY  reload.accesskey              "l">

Changed "l" to "e".

>Index: browser/locales/en-US/chrome/browser/preferences/languages.dtd

>-<!ENTITY languages.customize.selectLanguage.accesskey   "S">
>+<!ENTITY languages.customize.selectLanguage.accesskey   "n">

Why make this change? (The key doesn't work anyways, since it's used on a
dropdown, but that's another bug). I've reverted it locally.

Bryan, thanks again for doing the hard work here. If you see anything wrong
with the changes above, let me know. I'm going to post an updated patch.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

Attachment: Updated

Comment 38

[Bryan]

(In reply to comment #36)
> >Index: browser/locales/en-US/chrome/browser/preferences/permissions.dtd
> 
> > <!ENTITY removeallpermissions.label   "Remove All Sites">
> >+<!ENTITY removeallpermissions.accesskey "A">
> 
> > <!ENTITY allow.label                  "Allow">
> >+<!ENTITY allow.accesskey              "l">
> 
> I switched these two ("A" for "Allow") since that's probably used more often.

If I recall correctly this won't work (due to bug
https://bugzilla.mozilla.org/show_bug.cgi?id=143065) because "A" is already used
in that dialog.  I'll test these changes later today.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Attachment: Updated again

Forgot one file, and synced diff against current trunk.

Comment 40

[Bryan]

(In reply to comment #39)
> Created an attachment (id=190293) [edit]
> Updated again
> 
> Forgot one file, and synced diff against current trunk.

Everything looks great!  Let's get this in!!!  :-)

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Checking in browser/base/content/sanitize.xul;
/cvsroot/mozilla/browser/base/content/sanitize.xul,v  <--  sanitize.xul
new revision: 1.8; previous revision: 1.7
done
Checking in browser/components/preferences/content.xul;
/cvsroot/mozilla/browser/components/preferences/content.xul,v  <--  content.xul
new revision: 1.10; previous revision: 1.9
done
Checking in browser/components/preferences/downloads.xul;
/cvsroot/mozilla/browser/components/preferences/downloads.xul,v  <--  downloads.xul
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/preferences/fonts.xul;
/cvsroot/mozilla/browser/components/preferences/fonts.xul,v  <--  fonts.xul
new revision: 1.11; previous revision: 1.10
done
Checking in browser/components/preferences/permissions.xul;
/cvsroot/mozilla/browser/components/preferences/permissions.xul,v  <-- 
permissions.xul
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/preferences/sanitize.xul;
/cvsroot/mozilla/browser/components/preferences/sanitize.xul,v  <--  sanitize.xul
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/preferences/tabs.xul;
/cvsroot/mozilla/browser/components/preferences/tabs.xul,v  <--  tabs.xul
new revision: 1.7; previous revision: 1.6
done
Checking in browser/locales/en-US/chrome/browser/sanitize.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/sanitize.dtd,v  <-- 
sanitize.dtd
new revision: 1.3; previous revision: 1.2
done
Checking in browser/locales/en-US/chrome/browser/preferences/advanced.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/advanced.dtd,v
 <--  advanced.dtd
new revision: 1.6; previous revision: 1.5
done
Checking in browser/locales/en-US/chrome/browser/preferences/colors.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/colors.dtd,v
<--  colors.dtd
new revision: 1.4; previous revision: 1.3
done
Checking in browser/locales/en-US/chrome/browser/preferences/connection.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/connection.dtd,v
 <--  connection.dtd
new revision: 1.4; previous revision: 1.3
done
Checking in browser/locales/en-US/chrome/browser/preferences/content.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/content.dtd,v
<--  content.dtd
new revision: 1.3; previous revision: 1.2
done
Checking in browser/locales/en-US/chrome/browser/preferences/downloadactions.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/downloadactions.dtd,v
 <--  downloadactions.dtd
new revision: 1.4; previous revision: 1.3
done
Checking in browser/locales/en-US/chrome/browser/preferences/downloads.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/downloads.dtd,v
 <--  downloads.dtd
new revision: 1.3; previous revision: 1.2
done
Checking in browser/locales/en-US/chrome/browser/preferences/fonts.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/fonts.dtd,v 
<--  fonts.dtd
new revision: 1.7; previous revision: 1.6
done
Checking in browser/locales/en-US/chrome/browser/preferences/permissions.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/permissions.dtd,v
 <--  permissions.dtd
new revision: 1.4; previous revision: 1.3
done
Checking in browser/locales/en-US/chrome/browser/preferences/privacy.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/privacy.dtd,v
<--  privacy.dtd
new revision: 1.6; previous revision: 1.5
done
Checking in browser/locales/en-US/chrome/browser/preferences/sanitize.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/sanitize.dtd,v
 <--  sanitize.dtd
new revision: 1.5; previous revision: 1.4
done
Checking in browser/locales/en-US/chrome/browser/preferences/tabs.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/tabs.dtd,v 
<--  tabs.dtd
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/passwordmgr/resources/content/passwordManager.xul;
/cvsroot/mozilla/toolkit/components/passwordmgr/resources/content/passwordManager.xul,v
 <--  passwordManager.xul
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd,v
<--  passwordManager.dtd
new revision: 1.3; previous revision: 1.2
done

Comment 42

[Aaron Leventhal]

Maybe this checkin caused the XML error in the "Content" tab of Tools->Options
today?

It's complaining about 

            <button id="xformsCrossDomainButton
------------^

But the rest of the message is cut off.

Comment 43

[Marek Stępień [:marcoos, inactive]]

(In reply to comment #42)
> It's complaining about 
>             <button id="xformsCrossDomainButton
> ------------^

Yes, it's complaining about the allowedSites.label entity which has been removed
by this checkin. But this is not Firefox's problem, it's a problem that (in my
opinion) should be fixed in the XForms extension.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Attachment: Revert unnecessary renaming

Comment 45

[Mike Connor [:mconnor]]

Comment on attachment 190420 [details] [diff] [review]
Revert  unnecessary renaming

xforms is going to fix this

Comment 46

[Bryan]

Attachment: Three fixes to accesskeys that are broken due to two new checkins!

This patch fixes three broken accesskeys due to the checkins for bugs 285440
(duplicate use of _S_) and 299507 (duplicate use of _W_).

Comment 47

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 48

[:Gavin Sharp [email: [e-mail]]]

Checking in advanced.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/advanced.dtd,v
  <--  advanced.dtd
new revision: 1.8; previous revision: 1.7
done
Checking in sanitize.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/preferences/sanitize.dtd,v
  <--  sanitize.dtd
new revision: 1.7; previous revision: 1.6
done

Comment 49

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change