277463 - don't underline radio / checkboxes accesskeys if ui.key.menuAccessKey==0 (accesskeys should not be visible on Mac)

Status: VERIFIED FIXED

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

Description

[Jo Hermans]

Bug 68841 introduced underlining for access keys in XUL dialog boxes. That's
against the Apple Human Interface guidelines. We shouldn't do that on the Mac.
Should we even allow accesskeys in the GUI at all ?

Underlining inside an HTML page is bug 56701, and is different though.

Comment 1

[Stefan Borggraefe]

-> Core/Keyboard: Navigation

> Should we even allow accesskeys in the GUI at all ?

Why not? This helps using Mozilla with the keyboard which is often faster than
with the mouse.

Comment 2

[Jo Hermans]

(In reply to comment #1)
> -> Core/Keyboard: Navigation
> 
> > Should we even allow accesskeys in the GUI at all ?
> 
> Why not? This helps using Mozilla with the keyboard which is often faster than
> with the mouse.

It's against the Apple Human Interface Guidelines, which doesn't use access keys
  for checks boxes and radio boxes. They're sometimes used for accessing buttons
(Cmd-D for 'Don't Save' for example, but that's not supported by Mozilla
anyway), but they're not visible in the interface. The point is that the
interface should be as clean as possible for common users (using the mouse),
allow users with disablities (see below), but still hide shortcuts for advanced
users. I agree that these last ones aren't discoverable unless you read the Help.

Anyway, bug 187508 was recently checked in, and should allow access to the
controls with ctrl-tab, arrow keys and the spacebar. But it only works if you've
changed the System Preferences. This allows access for people that can't control
a keyboard, but uses sticky keys, a pointing stick, a joystick, voice control,
etc ...

Supporting the Apple's accessibility API is bug 157209

Comment 3

[Asaf Romano (gone)]

Note: some of the accesskeys are expected to work, but they should *never* be
visible on mac.

Comment 4

[Asaf Romano (gone)]

taking,-> 1.8b.

Comment 5

[Asaf Romano (gone)]

Attachment: patch

I'm not sure why we're hardcoding accesskeys for checkboxes/radios but here's
it.

Comment 6

[[e-mail]]

While I'm on the subject I thought I'd just comment that the checkbox/radio
access key code doesn't account for intl.menuitems.alwaysappendaccesskeys

Comment 7

[Asaf Romano (gone)]

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

please ignore the toolkit part

Comment 8

[Asaf Romano (gone)]

Attachment: toolkit patch

Comment 9

[Asaf Romano (gone)]

Attachment: patch for seamonkey (follow ui.key.menuAccessKey)

Comment 10

[[e-mail]]

Comment on attachment 170847 [details] [diff] [review]
patch for seamonkey (follow ui.key.menuAccessKey)

>+            var prefs = Components.classes["@mozilla.org/preferences-service;1"].getService
>+                                                    (Components.interfaces.nsIPrefBranch);
The correct way to wrap this is to break just after the ] and line up the
.getService with the .classes

>+            if (!/Mac/.test(navigator.platform))
>+              this.setAttribute("underlineAccesskey", true);
This needs to be a field, not an attribute (and you can init the field with
!/Mac/.test(navigator.platform) directly).

Comment 11

[sairuh (rarely reading bugmail)]

note to self: an example of underlined XUL accesskeys: Prefs > Web Features panel.

Comment 12

[Asaf Romano (gone)]

Attachment: patch for seamonkey

Comment 13

[[e-mail]]

Comment on attachment 170896 [details] [diff] [review]
patch for seamonkey

Consider an != 0 on the pref just to clarify that the field is a boolean. Also
xml doesn't need a space before /> (handily text.xml is the only binding
document with extra spaces, so at least you're consistent!)

Comment 14

[Asaf Romano (gone)]

Comment on attachment 170896 [details] [diff] [review]
patch for seamonkey

with neil's comments fixed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170896 [details] [diff] [review]
patch for seamonkey

sr=bzbarsky if you init the field with !/Mac/.etc directly like Neil asked (and
drop the code in the catch in the constructor, of course).

Comment 16

[Asaf Romano (gone)]

Attachment: address last comments (checked in)

moving reviews.

Comment 17

[Asaf Romano (gone)]

Attachment: toolkit patch

Comment 18

[Asaf Romano (gone)]

Comment on attachment 170972 [details] [diff] [review]
toolkit patch

oops, i touhjt it wasn't the right one, sorry for spam.

Comment 19

[Simon Montagu :smontagu]

xpfe patch checked in.

Comment 20

[Simon Montagu :smontagu]

toolkit patch checked in.

Comment 21

[Bill Gianopoulos [:WG9s]]

I know this comment is a bit late because this has already been checked in, but
I think a better approach would have been to have added a preference for the
underlining of the GUI access keys and default the choice according to operating
system.  That way the application still works according to the human interface
guidelines for the GUI, but the choice of how the application works is given
back to the user.

I really don't care any more about how Steve Jobs thinks a computer application
should work than I do about what Bill Gates thinks.

Also, remember, these are called Human Interface Guidelines.  They are therefore
neither standards nor requirements.  

I would rather have the coice of how applications behave on my compter by mine.

Just my $.02.

Comment 22

[Asaf Romano (gone)]

read the summary again, the comment (ui.key.menuAccessKey is a pref).

Comment 23

[Bill Gianopoulos [:WG9s]]

Oops.  Sorry for that (and this) bugspam then.  Should have known you guys would
get it right.

Comment 24

[Jo Hermans]

Verified to be fixed in the latest trunk (after the toolkit patch
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050116
Firefox/1.0+

You can get the underlines back if you set ui.key.menuAccessKey to 17 (default
is 0). No need to restart. The easiest place to check is Preferences -> Web
Features.