959 - (Accesskey-XUL) [FEATURE] Accesskey attribute not implemented yet for XUL

Status: VERIFIED FIXED

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

Description

[Angus Davis]

Several tricks here to implement...

<input type="button" value="Test" accesskey="T">

- when the user presses alt-(accesskey), the appropriate element should get
focus. If an accesskey is specified, it overrides any accesskey in the viewer
application. For example, if the accesskey on a form element were
"F," pressing "alt-F" would set focus on the element, not bring up the "File"
menu.

I assume joki owns this piece of the bug...

- we should underline the first instance of the accesskey character. In the
example above, the first "T" in the word "Test" would be underlined.

I assume chris or kevin or kipp or someone owns this piece of the bug...

Comment 1

[bkdelong]

Elements that utilize attribute ACCESSKEY and suggested actions:
A: Should select or activate link
AREA: Should select or activate hotspot on imagemap
BUTTON: Should scroll to or activate button
INPUT: Depends on type- (See below list)
       TEXT and PASSWORD:  Should scroll automatically to INPUT box
       CHECKBOX and RADIO: Should select ofractivate/check
       SUBMIT, IMAGE and RESET: Should scroll to or select button

LABEL: Should scroll to/select label

LEGEND: Should scroll to/select legend

TEXTAREA: Should scroll to/select TEXTAREA

Comment 2

[leger]

Setting all current Open/Normal to M4.

Comment 3

[Paul MacQuiddy]

per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser

Comment 4

[gerardok]

QA contact re-assigned to INPUT element owner.

Comment 5

[joki (gone)]

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

Comment 6

[joki (gone)]

This is not getting done by M4.  Moving to M5.

Comment 7

[jpbrody]

In the case of <a accesskey="a" href="http://www.mozilla.org">mozilla</a>
Please implement this by ACTIVATING the link (loading the page) when a user
presses a key, rather than giving the link focus (as IE5 does).

Comment 8

[joki (gone)]

Moving out to M7

Comment 9

[joki (gone)]

Moving to M8

Comment 10

[joki (gone)]

No time for M8

Comment 11

[joki (gone)]

No dependencies yelling for this so its lower priority.  Moving to M11.

Comment 12

[saari (gone)]

Is this important for beta?

Hyatt, will the menu key listener interfer with this?

Comment 13

[cpratt]

Speaking for QA (and QA only) this is unimportant for beta. angus?

Comment 14

[joki (gone)]

Moving multiple bugs to m12

Comment 15

[rickg]

Moving to m13 because Joki seems to be distracted.

Comment 16

[joki (gone)]

Moving off bugs that won't make M13

Comment 17

[ekrock's old account (dead)]

Moving M16.

Comment 18

[Antti Näyhä]

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

Comment 19

[Matthew T (active 1999-2002)]

A couple of suggestions:

* IE's behavior for accesskeys -- selecting a link or button, rather than
  activating it -- is very sensible. It prevents a malicious Web author from, for
  example, doing this

  <a href="thesamepage.html" accesskey="f"><font color="white">.</font></a>
  <a href="thesamepage.html" accesskey="e"><font color="white">.</font></a>
  <a href="thesamepage.html" accesskey="v"><font color="white">.</font></a>
  <a href="thesamepage.html" accesskey="g"><font color="white">.</font></a>
  <a href="thesamepage.html" accesskey="b"><font color="white">.</font></a>
  <a href="thesamepage.html" accesskey="t"><font color="white">.</font></a>
  <a href="thesamepage.html" accesskey="h"><font color="white">.</font></a>

  which would have the effect of completely, and (from the user's point of view)
  inexplicably, disabling keyboard access to the Navigator menus. (Using Alt+
  arrow keys would still be possible, but the user probably wouldn't realize
  that.)

  So, hitting an access key should select a link or button, not activate it. And
  pressing Shift+Alt+accesskey should override any accesskeys in the content and
  go to the menus, so that there is always a way to access a menu immediately
  with the keyboard even where content uses the same access key as the menu.

* When an accesskey is used, the content should always scroll so that the widget
  being manipulated is visible, before actually changing the widget state. If the
  widget wasn't visible, maybe even a short delay (~0.5 s) between the scrolling
  and the change in state would be nice, so that the user gets a chance to see
  what's happening to the widget (preventing the `aargh! what did I just do?'
  problem).

* On Mac, the modifier key for an accesskey should be Ctrl, not Option(/Alt).
  Unlike Windows and Unix, MacOS uses the Option(/Alt) key for entering non-ASCII
  characters, so unlike on Windows and Unix, Option shouldn't really be used for
  anything else.

  Some Mac apps, such as Photoshop and BBEdit, use the equivalent of accesskeys
  in their dialogs. They use the Command key to get to them (e.g. Command+D =
  `Don't Save'). So using Ctrl would be inconsistent with that. But Mozilla
  couldn't really be consistent and use Command, because in the world outside of
  dialogs, Mozilla (like other apps) uses Command for a whole lot of keyboard
  shortcuts; and the usability nightmare from accesskeys conflicting with these
  would be worse than the slight usability problem from being inconsistent with
  other MacOS apps.

  That's why I recommend Ctrl, which is little-used for anything else
  on MacOS at the moment. It would even be vaguely in line with the Mac HIGs,
  which say Ctrl should be used for `shortcut key sequences that the user defines
  using a macro-key facility'. Replace `user' with `page author', and this is
  pretty much what Mozilla would be doing.

We now return you to our regularly scheduled `Moving out to M[n+1]' comments ...

Comment 20

[Christopher Hoess (gone)]

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

Comment 21

[Jesse Ruderman]

Is it a security problem for keyboard-only users that pages can play with the 
alt keys, or will they be able to lift alt before pressing the letter to access 
the menu, on all platforms?

Comment 22

[Antti Näyhä]

Nominating for nsbeta2 and adding the 'html4' keyword.
Adding dependency on this bug to bug 7954 (HTML 4.0 compliance meta bug).

Comment 23

[leger]

Per ekrock mail:
"Sigh. This falls into the category of "important features we really better get 
in by 5/16.  Since this enables accessibility by the disabled, we could increase 
our exposure to a lawsuit if we don't implement this. So it's probably 
nsbeta2+[5/16-]."

Putting on [nsbeta2+][5/16] radar. 

Comment 24

[cpratt]

changing QA contact

Comment 25

[joki (gone)]

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

Comment 26

[joki (gone)]

Okay, this is 90% fixed.  Accesskey is in and works for everything except links.  
Working on that last bit.

Comment 27

[joki (gone)]

Rods has generously volunteered to finish this bug, which more or less means 
adding Register and and UnRegister calls for accesskeys on links.

Comment 28

[joki (gone)]

Attachment: Brief acccesskey test case

Comment 29

[rods (gone)]

Accesskeys now work for all form controls which is the most important part of 
this bug. The only part that does not work is acess keys on links, which isn't 
really very important. I probably won't get this fixed today. I suggest this now 
be downgraded to nsbeta2-

Comment 30

[rods (gone)]

now regs and unregs in SetDocument - fixed

Comment 31

[sairuh (rarely reading bugmail)]

over to janc 9or jrgm?) to verify.

Comment 32

[David Hyatt]

This must work for XUL as well.  Should I give myself a separate bug for XUL, or 
do we want to use this bug?

Comment 33

[David Hyatt]

i am now taking this bug for xul.  it's done for html, and now it needs to be 
done for xul.

Comment 34

[David Hyatt]

taking bug...

Comment 35

[David Hyatt]

removing nsbeta2, since this bug is now about xul and should probably be 
re-evaluated

Comment 36

[Peter Trudelle]

moving to m18.  

Comment 37

[leger]

Putting on [nsbeta2-] radar.  

Comment 38

[Antti Näyhä]

There's still a few problems with the current implementation:

1. The access keys specified by the HTML document don't disable those of the
application.  For instance, while viewing the attached test case on Linux,
activating the first radio button by pressing Alt+a also activates browser's
Select All function.  The same goes for Alt+l, which activates Open Web
Location.

2. Access keys on links should just give the link focus, instead of following
the link immediately.  See Matthew's earlier comment.  To clarify my point,
let's take another example:

A long page has a navigation link at the bottom:
  <a href="first.html" accesskey="f"><strong>F</strong>irst page</a>

This link is not visible without scrolling, so the user has no way of knowing
that 'f' access key is used by the page.  Now, immediately after arriving on the
page, the user presses Alt+f to bring down the File menu.  Instead (s)he's taken
to the first page of the site, which makes him/her switch back to IE in
frustration.  :-)

3. The first occurrence of the access key in the link text, button text or label
should be highlighted.  Maybe a separate bug should be filed for this issue?

Comment 39

[Matthew T (active 1999-2002)]

> 3. The first occurrence of the access key in the link text, button text or
> label should be highlighted.  Maybe a separate bug should be filed for this
> issue?

Yes. There needs to be a nice algorithm which takes a text string and an 
accelerator character, and returns the most appropriate character to underline to 
indicate that accelerator (with a value of 0 if the accelerator is not in the 
string at all). The most appropriate character isn't necessarily the first 
occurrence of that character. As much as you could determine it with an 
algorithm, it is, in order:
* the first occurrence of that character at the start of a word;
* the first occurrence of that character at the end of a word;
* the first occurrence of that character at all.

Comment 40

[Jesse Ruderman]

#1 - bug 40071

#3 - How do you decide when to turn that algorithm off (for example, if I put 
bold/underline on occurence of the accesskey letter that makes the most sense 
to me)?

Comment 41

[Jesse Ruderman]

#2 - isn't it generally worse to accidentally submit a form than to 
accidentally follow a link?

(should accesskey be disabled in e-mail? comment in bug 28327.)

Comment 42

[Jesse Ruderman]

Sorry if I sound argumentative.  I agree with you guys :)

Comment 43

[Antti Näyhä]

> #2 - isn't it generally worse to accidentally submit a form than to 
> accidentally follow a link?

Definitely.  I think access keys shouldn't directly activate any elements - only 
give them focus.  Most importantly, they should never
 - follow links (<a>, <area>)
 - "push" buttons (<button>, <input type="submit|reset|image|button">)

It's true that directly activating radio buttons, checkboxes and file dialogs 
(<input type="file">) isn't _quite_ as disastrous from a usability viewpoint as 
directly activating links and buttons.  I still think it may seriously confuse 
users in some situations.  Also, for the sake of consistency, identical 
behaviour between all accesskey-enabled elements might be the best way to go.

(I guess I only mentioned links in my last comment because the link was the most 
obviously problematic element included in the test case.  Silly me.)

Comment 44

[Blake Ross]

Isn't this long done?

Comment 45

[cpr]

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

Comment 46

[Asa Dotzler [:asa]]

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

Comment 47

[Peter Trudelle]

I think we'll have to live without this in current release, -> Future

Comment 48

[ckritzer (gone)]

Mass update:  changing qacontact to [e-mail]

Comment 49

[Chris Petersen]

Nominating for nsbeta3.

Comment 50

[Peter Trudelle]

nsbeta3-.  This bug report has morphed too much, and had so many lists of
suggestions and problems added to it that I don't know what it is about any
more.  Please file separate bugs for any issues that you care about, and only
nominate major or critical severity parts for nsbeta3, since we already have too
many nasty bugs to fix in the time remaining.

Comment 51

[Loco]

Anyone notice that if ACCESSKEY is set to "C" on a Mac, the command can be 
confusing to the user?  I.E. if Command + C is pressed, will it Copy what's 
highlighted or follow the ACCESSKEY link???

Comment 52

[Peter Trudelle]

Don't know what the std says, but in practice we're planning to mimic IE4
behavior on this, the accesskey will act on the event, the menu will ignore it
see bug 40071.

Comment 53

[Loco]

Upon further investigation, it seems that I was right.  Command+"A" (Select All 
on Macs) highlights the button, but also selects all.  Same goes for Command + 
"C" doing a copy and also selecting the "C" button.  My recommendation would be 
to change this ACCESSKEY to default to the "Option" key instead of the "Command" 
key-or Control.  How about a toggle switch in Preferences?  Just tossing out 
ideas.

Working on build 2000-09-04-08, MacOS 9.0.4, on file:
http://slip/projects/marvin/html/button_accesskey.html

Comment 54

[John Morrison]

Hey Dan. I suggest you file the above as a separate bug report, if it should be 
done. Otherwise, it will get no attention.

Comment 55

[Loco]

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

Comment 56

[Loco]

Sorry about that.  After being slapped upside the head, reopening bug and closing 
959.  The difference is that 959 was a "Not Implemented Yet" bug, whereas this is 
a "Implemented But Not Really Working" bug.  Any other bugs stemming from that 
should be opened separately-it was getting out of control.

Moving Mac ACCESSSKEY issue to http://bugzilla.mozilla.org/show_bug.cgi?id=51940.

I also recommend we start tearing this bug into smaller more manageable 
pieces...no?  There is waaaay too much going on here, I think but I'd like to 
hear some input about this idea...

Comment 57

[John Morrison]

This bug was FIXED on 5/16, but hyatt resurrected it to deal with the XUL case.
However, subsequent to that, a number of HTML issues were noted. Yes, those
should be filed as separate bugs. I will file a bug on hyatt specifically for
any XUL issues. Then this bug can be closed out (returned to FIXED for those 
issues noted up to 5/16).

Comment 58

[Loco]

Updating QA Contact.

Comment 59

[Loco]

Updating Summary to reflect Status since it seems fairly relevant.

Comment 60

[Christopher Hoess (gone)]

Can someone with the permissions remove the 7954 block and html4 keyword?  The
attached HTML testcase works fine-I think all HTML accesskey issues have moved
to other bugs.

Comment 61

[Antti Näyhä]

Yep, you're right.  Removing those.

Comment 62

[Jesse Ruderman]

Nominating for the new nsbeta1 and removing old [nsbeta2-][nsbeta3-].

Comment 63

[Matthew T (active 1999-2002)]

While we're using Control to activate access keys in HTML (and we don't really 
have any other choice), Mac OS convention is to use the Command key to activate 
access keys in chrome -- e.g. many users are used to pressing Command+D for 
`Don't Save' in `Save before closing?' alerts. If we follow this convention, then 
XP we will need to avoid using the accesskeys A, C, Q, V, W, X, Y, and Z in 
chrome where possible, since Command+ those keys does editing/closing operations. 
Or we could just use Control for chrome like we do for HTML, and not have to 
worry about this.

Comment 64

[Loco]

I agree with mpt's comments above-in that we should be careful to not to use the 
Command + [key] combinations common in editing.  Despite that, my vote is for 
using Command for the chrome commands though-I would never expect to use Control 
in a Mac app basically-especially being that Command is the norm for UI controls.  
Oh, and underlining the accesskey in chrome would certainly go a faaaar way for 
me as well (in a good way).

Comment 65

[timeless]

I'm not sure this is the right bug, but i'm tired...

Instead of worrying about both html and chrome bindings, let's worry about one 
set at a time.

Basically, either you're using the browser, or you're using the content.  When 
users run mozilla, it defaults to binding chrome keybindings.  There is a 
single key trigger (pick a key, I like sysrq or any fkey or ` or whatever aaron 
and mpt can agree on) as well as a menu trigger.  Most likely, there is a 
notification/toggle in the bar that has security and online status.  A tooltip 
for this explains what the current state is and has a help link/button.

When the user wants to switch to html bindings, the user presses the key, or 
clicks the toggle or selects the menu item.

The indicator switches to indicate that commands will go to the document [I 
think a special border or something should be given to the active region].

While html keys are bound, mozilla can based on a user stylesheet more actively 
hint at their existence [possibly inlining hints].

Users can switch back to chrome using the same toggles.

I am under the impression that only letters and possibly a few other keys can 
be bound by html.  So I would suggest that back and forward (accel+left, 
accel+right), form submit (accel+enter), and possibly help (f1?) retain their 
bindings.

Activation of this feature should display a dialog explaining what is 
happening, with a check box to allow the user to not be warned again.

Comment 66

[Jesse Ruderman]

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

Comment 67

[Loco]

REMOVING nsbeta2/3 and replacing with nsbeta1 KW.  Also clearing 
Status Whiteboard of nsbeta2/3 result.  Accessibility is important.

Comment 68

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 69

[sairuh (rarely reading bugmail)]

should the milestone still be future here? clearing it for re-evaluation
--afaik, this ought to be fixed for basic accessibility in 6.5... thx!

Comment 70

[Blake Ross]

I agree, this is fairly important for accessibility (although not embedding, 
really).  cc'ing trudelle.

Comment 71

[Matthew T (active 1999-2002)]

[Really CCing Trudelle]

Comment 72

[Matthew T (active 1999-2002)]

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

Comment 73

[Jesse Ruderman]

Nominating for mozilla0.9 (this blocks a whole lot of bugs).

Comment 74

[Peter Trudelle]

->mozilla1.0, is this absolutely required for accessibility in next rev of the apps?

Comment 75

[Matthew T (active 1999-2002)]

As long as every GUI element is available by tabbing, I wouldn't regard lack of 
accesskeys as an accessibility blocker -- just annoying, especially if you are 
keyboard-bound and {the element which has focus} and {the element you want to get 
to} are far away from each other.

Comment 76

[bsharma]

The test case is attached in the url field. The accesskey works with mouse but 
are not accessible through the keyboard. Pressing the key opens the menu keys 
(e.g. Pressign Alt-B, opens Bookmark).

Comment 77

[bsharma]

Attachment: The image file

Comment 78

[Blake Ross]

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

Comment 79

[Madhur Bhatia]

QA contact updated

Comment 80

[Blake Ross]

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

Comment 81

[Chad Austin]

I have a preliminary patch!  It does checkboxes, focuses text fields, and almost
does radio buttons.  ;)  Push buttons don't work at all at the moment...  I'll
investigate more.

Assigning to me.  I'll assign it to someone else if I can't complete it.

Comment 82

[Chad Austin]

Attachment: preliminary

Comment 83

[Chad Austin]

Oh yeah, forgot to mention... There are some changes to the find search dialog
in there. I used those for testing.

Comment 84

[David Hyatt]

Ok, here are some comments.  First of all, thanks for tackling this.  It's a 
much needed XUL1.0 feature.

My main concern is over the way the accesskeys are hooked up.  Namely every 
element ever inserted into a document (i.e., all of them) has its accesskey 
attribute checked.  Two problems occur to me with the approach of doing this 
from SetDocument.  The first I just mentioned, namely that you waste time doing 
an extra GetAttribute string fetch for all XUL elements.  The second is a menu 
issue... menus shouldn't even be searched, since their accesskeys are already 
handled down a completely different code path.  I wouldn't even crawl into 
menus or menubars or menuitems.

The hookup should be kept in the front end.  I would do the hookup in the 
nsTextBox code.  Right now your code will hook up access keys on invisible 
elements (elts with display: none).  In order to work, an accesskey has to be 
visible (and therefore has to have a frame), so you can simply do the hookup in 
the frame in the same code that already checks accesskeys.  That means the 
content model is not the place to do this hookup, since you don't know if you 
even have a front end frame object built for that content.

Similarly, in the Destroy method of nsTextBox, you can do the unhooking if it 
has an accesskey.

You'll want to avoid doing all of this hooking/unhooking for text frames that 
are inside menu items and menus though, since they have their own code for 
accesskey.  

It's also easy to handle the dynamic case by using the frame code.  Your patch 
does not handle someone dynamically adding an accesskey to an element (or 
changing/removing the accesskey attribute).  The frame code already has to 
worry about that case, and if you patch the method that builds up the access 
string, you'll handle this case without writing extra code.

Comment 85

[Chad Austin]

Hyatt:  Thanks for the comments.  However, dynamic access keys don't work in
HTML at the moment either (attaching a testcase).  Would you prefer that be a
new bug or should it go into this (already cluttered) one?

Comment 86

[Chad Austin]

Attachment: dynamic HTML accesskey testcase

Comment 87

[Chad Austin]

Filed bug 88151 for modifying HTML accesskeys via the DOM.

Comment 88

[Christopher Hoess (gone)]

I don't think this bug blocks bug 41368 any longer-at least not since the
switchover from HTML to XUL.

Comment 89

[Chad Austin]

Attachment: second attempt

Comment 90

[Chad Austin]

Okay, this second patch goes along with hyatt's guidelines.  Things that don't
work:  for some reason, when an accesskey is pressed, buttons simply get focus
and don't fire.  If I set the case sensitive checkbox in the 'find in this page'
dialog's accesskey to be 'C', it doesn't work.

Comment 91

[David Hyatt]

Move the nsBoxFrame code into nsButtonBoxFrame.cpp.  This will cover radio
buttons, checkboxes, and buttons, and it will ensure that random normal boxes
don't waste time checking for an accesskey attribute.  It will also prevent a
slew of extra hookups (e.g., menus and menuitems) from happening when they don't
need to.

Comment 92

[Chad Austin]

Attachment: third patch

Comment 93

[Chad Austin]

That last patch works with checkboxes and pushbuttons.  Radio buttons are kinda 
weird at the moment.  Dynamically changing the accesskey via the DOM works too.

Comment 94

[David Hyatt]

Two comments.

(1) Don't call the nsBoxFrame::AttributeChanged method from nsButtonBox in the
case where the accesskey attribute changes.  nsBoxFrame won't do anything with
that attr, so avoid the extra call.

(2) Don't QI to nsIDOMXULElement to test the "type" of an element.  jst fairly
recently added a method for querying the type, and you can ask if an element is
XUL much more efficiently by using that method.

One more iteration should do it.  Great job!

Comment 95

[Chad Austin]

Attachment: fourth patch

Comment 96

[Chad Austin]

There is some funkiness going on with radiobuttons still...  I did some tracing 
and the XBL behaves properly in setting |checked| on the radio button...  After 
the setter for selectedItem is called, something unsets |checked| (in some 
cases).  Hopefully some event/XBL guru can tell me what's happening.  More 
investigation tomorrow...

hyatt:  Thanks for the feedback.  It's been really helpful in learning the 
codebase.

This bug is hopefully coming to close...

Comment 97

[Chad Austin]

Attachment: fifth patch

Comment 98

[Chad Austin]

Okay, the radio button problem was in the event state manager.  I accidentally 
had a little bit of the HTML accesskey code path executing on the XUL side.  
Fifth and hopefully final patch is here, can we get r= and sr=?  (bug 90221 and 
bug 90406 need to be checked in first)

Comment 99

[Chad Austin]

Attachment: sixth patch

Comment 100

[Chad Austin]

Okay, I like this patch the best.  It oncurs a little overhead (both processing
and memory) for each XUL element, but it's much more flexible and
straightforward.  in nsXULElement, you can specify which elements support
accesskeys.  In the event state manager, you can define the behavior for each
type of element.  This patch works (perfectly?) with textboxes, checkboxes,
radio buttons, and push buttons.
I've included the fixes for bug 90221 and bug 90406.

reviewers?

Comment 101

[Chad Austin]

We can get this in for 0.9.3.

Comment 102

[Chris Waterson]

Don't do this:

+              nsXULElement* element = NS_STATIC_CAST(nsXULElement*, raw_content);

Comment 103

[David Hyatt]

I do not like having the accesskey code in nsBoxFrame.  I would like that moved
to nsButtonBoxFrame.  Is there a reason why the code can't be moved that I am
missing?

You used the IsContentOfType.  That's good, but as waterson said, I wasn't
advocating eliminating the QueryInterface all together.  I just meant that your
initial determination of type should use IsContentOfType.  Once you know you're
a XUL elt, it's ok to QueryInterface to a XUL element (since you know it will
succeed).  We're only looking to avoid the cost of missed QIs here, which is why
I requested the use of IsContentOfType to ensure you didn't have HTML elts
failing QIs to a XUL elt.

What is the code in XBL doing?  It looks like you've added a redundant
implementation into <radio>.  <radio> should be handling selection already.  Why
was this code added?

Comment 104

[Chad Austin]

There are other non-button elements that need to have accesskey support.  
Textboxes right now, and possibly more later.  One way to do it would be 
putting the same code in every frame class that needed accesskey support, but 
this would be a pain to maintain.

I'll fix the QI stuff.

The odd radio button behavior is addressed by bug 90406.  Basically, calling 
nsXULElement::Click() on a <radio> element had very bizarre behavior.  I made a 
testcase with two radio buttons and two pushbuttons.  The pushbuttons would 
call click() on their respective radio buttons.  The radio group kept getting 
into situations where either one button would be selected or both would be.

In today's commercial build, calling click() on a radio element doesn't do 
anything at all.  Either way, that patch to XBL fixes the problem.  There are 
probably a hundred other ways to fix it, and if anyone knows a better way, feel 
free to comment.

Comment 105

[Chad Austin]

Attachment: seventh patch

Comment 106

[Jason Kersey]

pushing to 0.9.4 for aegis.  won't make the 0.9.3 train unfortunately...

Comment 107

[Chad Austin]

Oops, ignore the change to checkbox.xml.

Comment 108

[David Hyatt]

Could you explain why <textbox> needs to support accesskeys?  As far as I can 
tell, it doesn't need accesskey support.  It would be a label associated with 
the textbox that would require accesskey support, and that would need to listen 
for the accesskey and activate the elt specified by its "for" attribute when 
the accesskey got pressed.

As far as I can tell (just ticking through the widgets that use access keys) 
only button box frames and text frames with "for" attributes need to support 
accesskeys.

Comment 109

[Aaron Leventhal]

<hyatt> heck, a separate bug could cover the "for" case.
<hyatt> the "for" case hasn't been addressed by any patches.
<hyatt> i want the code in boxframe moved to buttonboxframe
<hyatt> right now it's in boxframe but there's a guard like
<hyatt> if (i'm a checkbox, radio, textbox, or button) { do accesskey
 stuff }
<hyatt> but textbox doesn't support accesskeys (since you have to
 use a label with a for attr for that case)
<hyatt> and the other three are all buttonboxframes.
<hyatt> so the code doesn't need to be in boxframe
<hyatt> if it were moved to buttonboxframe you could get rid of the if
 statement completely
<hyatt> and just always check for accesskeys

Comment 110

[Chad Austin]

Attachment: eighth patch

Comment 111

[Matthew T (active 1999-2002)]

Wow, cool. Finally all those underlined letters in Mozilla for Windows and X
won't be just for giggles.

On Mac OS, however, looks like this won't work. I don't see any `#ifdef PC' or
whatever in these patches, so I'm assuming you're not aware of how chrome
accesskeys (as opposed to HTML accesskeys) are dealt with on Mac. Two
preliminary facts:
(a) The only widgets on Mac OS which take keyboard focus are text fields and
    listboxes (trees and outliners).
(b) the modifier key used to activate accesskeys on Mac OS is Command, which is
    the same key as is used for keyboard shortcuts e.g. (Copy = Command+C).

So, here's a rough algorithm which is probably thoroughly inaccurate from a code
point of view:
1.  When Command+{something} is typed, let it be handled by other event handlers
    first. If it's a modal dialog, it should be taken if {something} is one of
    {A, C, V, X, Z, Shift+Z}. If it's a non-modal window, then just about
    everything will be taken probably (e.g. Command+N will open a new browser
    window, Command+W will close the focused window, Command+O will open a
    file ...).
2.  If nothing else takes it, and there's an element with that accesskey, then:
    -   if it's a label for a text field or listbox, focus it;
    -   if it's a checkbox, toggle it, but do not change the focus from what it
        was;
    -   if it's a button or radio button, activate it, but do not change the
        focus from what it was;
    -   if it's a label for a radio group, activate the next radio button in
        the group after the one which is currently activated, but do not change
        the focus from what it was;
    -   if it's anything else, ignore it.

A test is to start a Composer document, enter some text, and close the window.
Typing Command+D in the resulting alert should trigger the `Don't Save' button.

Comment 112

[Chad Austin]

Attachment: patch numero neuf

Comment 113

[Chad Austin]

mpt:  Sorry, I don't have a Mac.  saari?hyatt:  The latest patch has code in
nsTextBoxFrame also because text box frames don't inherit off of nsBoxFrame.

Comment 114

[Chad Austin]

I started working on a tenth patch to address the fact that nsBoxFrame and
nsTextBoxFrame could share some registration code, but it turned out that I
could only factor out maybe ten lines of code, and it's not really worth it.

Reviewers?

Comment 115

[David Hyatt]

Ok, I'm cool with everything in this patch except for radio.xml.  That patch is
incorrect for a few reasons.  First of all, <radiogroup> can contain nested
boxes, etc., so you can't assume <radio>'s parentNode is a <radiogroup>.  That's
true most of the time, but not necessarily all of the time.

Second, I don't understand why it's needed.  The "command" DOM event should fire
on the <radio> when you invoke Click(), and that should bubble up to the
<radiogroup>, which should have a handler in place.  Perhaps the target of the
DOM event is not being set correctly when Command() is invoked programmatically?
 Basically I'm thinking there's a bug somewhere but this isn't the right fix.

If you want to check in without radio.xml, sr=hyatt, but file a separate bug on
the radio issue.  If you want to hold up, then we can investigate and do a new
patch here.  Either is cool with me.

Comment 116

[Chad Austin]

I just did some investigation, and if you don't change radio.xml, pressing a
radiobutton's accesskey (calling nsXULElement::Click() on it) treats it just
like a checkbox (you can have multiple items selected at a time, or none at all...).

Something is quite screwy with the event code here.

Comment 117

[Chad Austin]

Attachment: patch #10!

Comment 118

[Chad Austin]

jag?  blake?  wanna r= me?

filed bug 94222 for the freaky radiobuttons

Comment 119

[David Hyatt]

I noticed one thing you could do better.  In nsTextboxFrame, it's better to
check for "for" before "accesskey".  The former will be much less common than
the latter, which is used in all of the menuitems (i.e., you'll slow down menus
a little with the current patch, since you'll *find* an accesskey attr but won't
find a "for" attr).

Comment 120

[David Hyatt]

I don't need to see another patch, but reversing the "for" and the "accesskey"
would be cool.  sr=hyatt

Comment 121

[Chad Austin]

Attachment: eleventh patch

Comment 122

[Chad Austin]

I don't have CVS access anyway, so here's the eleventh patch.

Comment 123

[David Hyatt]

One note.  Support for <label control=""> is in.  Not holding up the patch, but 
I don't want to see a bunch of people running around after this lands adding 
<text for=""> when that has been deprecated in favor of <label control="">. :)

Comment 124

[Chad Austin]

Attachment: patch 12

Comment 125

[Chad Austin]

patch 12 has timeless's whitespace nits.  Nothing else changed.

Comment 126

[jag (Peter Annema)]

Attachment: [patch] Use nsCOMPtr, get rid of redundant code, fix a few gotchas

Comment 127

[jag (Peter Annema)]

I went over the code with aegis and found a few things that could be done 
better. After some discussion with aegis we fixed/changed a few other things, 
which resulted in the latest patch (#13!). There are still a few open issues, 
I'll get back on those after I've talked to hyatt. For now I think this patch is 
fine. r=jag (and I'm sure r=aegis for the things I changed).

Comment 128

[David Hyatt]

+        nsresult rv = mContent->GetAttribute(kNameSpaceID_None,
+                                             nsHTMLAtoms::_for,
+                                             forAttribute);


Change the above to nsXULAtoms::control.
The visibility collapsed or hidden case is not being covered.  The user-focus
property is also not being checked.  We need to (if the visibility is
collapse/hidden or if user-focus is not set to normal) not do the Click().

The event state manager does this on frames, so you can check the code over
there to see this.

Comment 129

[David Hyatt]

Basically tab panels will allow you to "click" objects in panels that aren't
showing if we don't patch this to deal with visibility.

Yet another issue (that can be resolved in a separate bug) is the problem of
crossing iframes (as is the case in the prefs panel).  You should be looking
through *all* iframes in the chrome tree, so if you fail to find a registered
match in one event state manager, you need to crawl (possibly both up and down)
to the other event state managers (but avoid crawling into sandboxed HTML content).

Comment 130

[Chad Austin]

We have no nsXULAtoms::control yet.  ;)  Should I add it?  Does it magically 
Just Work in the parser?

Comment 131

[Chad Austin]

I also don't know how to check visibility on a content node...  I don't think 
I'm going to be able to get to a new patch today (my last day).  Anyone else 
want to finish up?

Comment 132

[Chad Austin]

Attachment: testcase (XUL)

Comment 133

[Chad Austin]

Attachment: testcase (JS)

Comment 134

[Aaron Leventhal]

Now that aegis is gone, who wants to take over this code?

Jag? Aegis mentioned that you had improvements you wanted to make to this patch
before it was checked in.

Comment 135

[jag (Peter Annema)]

Taking this.

Comment 136

[jag (Peter Annema)]

Attachment: [patch] Umpteenth patch

Comment 137

[jag (Peter Annema)]

This patch includes the patch in bug 94222. This won't work without that patch.

Looking for constructive comments, r= and sr=

Comment 138

[Chad Austin]

I'm assuming you know what you're doing with the autostretch restructure.

You probably should update the comment about the |for| attribute, as it now 
should be the |control| attribute.

Did you ever find out why accesskeys are registered as |PRUint32|'s?

nsEventStateManager has damn long methods.  :)  Code Complete would not be 
pleased.

Try removing the <radio> special case in nsEventStateManager.cpp.  The 
NS_XUL_CLICK fix should make that special case unnecessary.  Speaking of which, 
the NS_XUL_CLICK code should be given r= by hyatt (or you, jag), as I'm the one 
who wrote that code.

And as long as you're changing whitespace, do we really need the // releases. 
Duh. comment?

Comment 139

[jag (Peter Annema)]

->0.9.5

Comment 140

[Blake Ross]

Wah. Why?

Comment 141

[jag (Peter Annema)]

Too risky for 0.9.4, no immediate need for it, can wait till 0.9.5?

Comment 142

[jag (Peter Annema)]

Attachment: Incorporate aegis' remarks

Comment 143

[jag (Peter Annema)]

Removed <radio> specialcasing, fixed comment from 'for' to 'control'. Leaving 
PRUint32 "key" attributes and the redundant "release" comments for another 
patch.

Comment 144

[Aaron Leventhal]

Jag says this is ready for r=/sr=

All the important special cases are in there, says he.

Reviewers? blake? hyatt?

Comment 145

[saari (gone)]

upping severity to blocker since it is blocking work on several accessibility bugs

Comment 146

[saari (gone)]

picking this up for the end game

Comment 147

[saari (gone)]

-> 0.9.6

Comment 148

[Blake Ross]

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

Comment 149

[sairuh (rarely reading bugmail)]

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

Comment 150

[jag (Peter Annema)]

Attachment: Patch updated to trunk

Comment 151

[jag (Peter Annema)]

Attachment: Above -buw

Comment 152

[jag (Peter Annema)]

Attachment: simple XUL testcase

Comment 153

[jag (Peter Annema)]

For this testcase you'll need to make sure the content area is focused (click in 
the textbox for example). access+e should focus the "hello" checkbox (we don't 
support access key underlining in marked up text (e.g. xul:html) yet, the rest 
of the access keys should visible.

Comment 154

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

Comment on attachment 59808 [details] [diff] [review]
Above -buw

r=[e-mail]

Comment 155

[jag (Peter Annema)]

Attachment: Update to nsBoxFrame.cpp

Comment 156

[jag (Peter Annema)]

Attachment: Above, -uw

Comment 157

[David Hyatt]

+
+    nsString    mOldAccessKey;
+

This is unnecessary.  UnregisterAccessKey should be patched to not require that
a key be specified, since only one key can be registered for an element anyway.
 We don't want to bloat all box frames by 24 bytes in order to cache the old string.

Comment 158

[Matthew T (active 1999-2002)]

Jag, it looks like you're still missing #ifdef PC (or #ifndef MAC) in those
patches. The Control key is implemented on the Mac as the least nasty choice for
accesskeys in HTML; but in XUL, either Command+accesskey should activate the
controls, or nothing should. And if it's Command+accesskey, then you need what I
described in comment 111.

Meanwhile, I filed bug 117068 to remove the accesskey underlining if this bug
doesn't get fixed before 1.0.

Comment 159

[jag (Peter Annema)]

Attachment: Latest patch

This patch still needs some work:
 - make click() generate mousedown, mouseup events in addition to click
 - add method to XBL to find out whether a property is inherited:
     this so we know when we should and shouldn't register accesskeys

Comment 160

[David Hyatt]

The XBL problem should be fixed as follows:
(1) In the XUL text frame, call GetBindingParent on your content node.  
(2) If you have a binding parent, then get the binding manager from the content
node's document.
(3) Add a new API to nsIBindingManager called IsAttributeInherited that takes
the binding parent, the content node for the text frame, and an atom attribute
(accesskey).
(4) In the IMPL in binding manager, get the binding for the binding parent, and
call IsAttributeInherited on the nsIXBLBinding. 
(5) In the impl of nsXBLBinding, you will have to crawl up until you find the
binding with the anonymous content.  When you hit that binding, call into the
prototype binding.
(6) In the prototype binding, you'll have to go to the attribute table and get
the entry of all prototype elements that inherit the attribute.  These are
elements in the prototype content template (i.e., the original XML document). 
You'll need to use LocateInstance to get the corresponding real content node. 
If the real content node that you get for one of the prototype elements matches
the content node of the text frame, then return true.  Otherwise return false.

Comment 161

[David Hyatt]

Never mind.  I'm an idiot.  The registration was only being done for text frames
with a control attribute.  That works and avoids the need for the XBL extensions.

Comment 162

[jag (Peter Annema)]

Back to me again :-)

Comment 163

[David Hyatt]

So the one remaining issue is to handle multi-line labels.  You need to make a
new kind of frame (nsLabelFrame) instead of just creating an nsBlockFrame in
nsCSSFrameConstructor.  You could just use this frame if the control attribute
is present on the label.  

You then need to make this label frame get and cache the accesskey (responding
to updates of the accesskey attribute in its attributechanged method as well).  

This label frame should have a customized paint method that first paints the
block and then crawls its children looking for the first instance of the
accesskey character.  It should find the coordinates of the character, compute
the coordinates of an underline, and then translate those coordinates back into
the parent frame's space (by offsetting as you crawl back up your parent chain).

The parent frame should then paint the underline.  If you want to optimize this
code, you could cache the coordinates of the underline after reflows to avoid
crawling into your children on every paint, but that could come later.

This should be fixed before 959 lands.

Comment 164

[Aaron Leventhal]

Hyatt, it looks like this needs sr=.
Can we get this in and file a separate bug for the multi line labels?

Comment 165

[Blake Ross]

No, there's also the issue of getting rid of mOldAccessKey.  I did that a few
weeks ago and may still have the patch if you want it.  But it's not that
difficult...

Comment 166

[Aaron Leventhal]

Blake, if you could post that I'd appreciate it.

Comment 167

[jag (Peter Annema)]

I have a new version that gets rid of mOldAccessKey in the nsBoxFrame case. I'll
attach that.

Comment 168

[jag (Peter Annema)]

Nevermind, it's already attached above. I could apply the same logic in the 
nsTextBoxFrame case, if deemed necessary, but unregistering these elements is 
faster when you know the accesskey (though I could store the key as a PRUnichar 
instead of as a nsString).

Comment 169

[Blake Ross]

My patch didn't store it at all, anywhere.  It just modified
nsESM::UnregisterAccessKey to retrieve the accesskey itself from the content if
aKey was null.

Comment 170

[Blake Ross]

Also, we should use HasAttr when checking for the |control| attr, and probably
|accesskey| as well.

Comment 171

[jag (Peter Annema)]

I'll make those changes.

Comment 172

[jag (Peter Annema)]

Doh, I thought about that (and forgot I thought about it), but the problem there
is that when you're changing the accesskey you don't have the old accesskey to
unregister on, only the new one.

Comment 173

[Blake Ross]

I ran into that problem too, and fixed it by putting this in
nsDocument::AttributeWillChange:

  if (aAttribute == nsXULAtoms::accesskey) {
    nsIPresShell* shell = (nsIPresShell*) mPresShells.SafeElementAt(0);
    if (shell) {
      nsCOMPtr<nsIPresContext> context;
      shell->GetPresContext(getter_AddRefs(context) );
      if (context) {
        nsCOMPtr<nsIEventStateManager> esm;
        context->GetEventStateManager(getter_AddRefs(esm));
        esm->UnregisterAccessKey(nsnull, aChild, nsnull);
      }
    }
  }

perhaps hyatt can comment on whether that's lame or not.

Comment 174

[jag (Peter Annema)]

That would do the trick. I'll but hyatt later.

Comment 175

[:deanis74]

        context->GetEventStateManager(getter_AddRefs(esm));
        esm->UnregisterAccessKey(nsnull, aChild, nsnull);

Is it worthwhile to do a null check on esm, or will it always be returned?

Comment 176

[jag (Peter Annema)]

Attachment: diff -uw of patch with blake's suggestions incorporated

I thought this also worked for:

<label accesskey="f" control="bar">foopy</label>
<button id="bar" value="Bar!"/>

but apparently I still need to add some Reg/Unreg code to whatever frame deals
with the above case.

Comment 177

[jag (Peter Annema)]

Looks like I should be able to add code similar to what's in nsBoxFrame.* to
nsAreaFrame.*, but I'm hoping there's a cleaner way.

Comment 178

[jag (Peter Annema)]

Attachment: Don't hook up element referred by control, look it up at keypress

Comment 179

[jag (Peter Annema)]

Attachment: -uw of above

Comment 180

[Joe Hewitt (gone)]

Comment on attachment 70111 [details] [diff] [review]
-uw of above

sr=me

Comment 181

[Marc Attinasi]

Comment on attachment 70111 [details] [diff] [review]
-uw of above

r=attinasi for the AreaFrame changes

Comment 182

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 183

[Peter Trudelle]

nsbeta1+ per ADT triage team

Comment 184

[jag (Peter Annema)]

Checked in.

Comment 185

[jag (Peter Annema)]

.

Comment 186

[Jesse Ruderman]

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

Comment 187

[Blake Ross]

This patch doesn't deal with tabpanel accesskeys. Was that just an oversight?

Comment 188

[jag (Peter Annema)]

Yeah, that was. Could you file a new bug on that though? It shouldn't be too
hard to add.

Comment 189

[jag (Peter Annema)]

Attachment: Test case

Test case for xul file

Comment 190

[Rakesh Mishra]

verified on the trunk build 2002-04-30-08-trunk on Windows 2000.

Comment 191

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381