Closed Bug 959 (Accesskey-XUL) Opened 21 years ago Closed 18 years ago

[FEATURE] Accesskey attribute not implemented yet for XUL

Categories

(Core :: User events and focus handling, defect, P1, blocker)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: angus, Assigned: jag+mozilla)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

(Keywords: access, helpwanted, Whiteboard: done for html, needs to be done for xul)

Attachments

(6 files, 24 obsolete files)

636 bytes, text/html
Details
15.58 KB, image/jpeg
Details
568 bytes, text/html
Details
42.05 KB, patch
Details | Diff | Splinter Review
29.35 KB, patch
attinasi
: review+
hewitt
: superreview+
Details | Diff | Splinter Review
1.55 KB, application/vnd.mozilla.xul+xml
Details
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...
Status: NEW → ASSIGNED
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
Setting all current Open/Normal to M4.
per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser
QA Contact: 4015 → 4137
QA contact re-assigned to INPUT element owner.
*** Bug 3422 has been marked as a duplicate of this bug. ***
Target Milestone: M4 → M5
This is not getting done by M4.  Moving to M5.
Target Milestone: M5 → M6
OS: Windows 95 → All
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).
Moving out to M7
Target Milestone: M7 → M8
Moving to M8
Target Milestone: M8 → M9
No time for M8
Whiteboard: [MAKINGTEST] Antti.Nayha@oulu.fi
Target Milestone: M9 → M11
No dependencies yelling for this so its lower priority.  Moving to M11.
Is this important for beta?

Hyatt, will the menu key listener interfer with this?
Speaking for QA (and QA only) this is unimportant for beta. angus?
Moving multiple bugs to m12
Moving to m13 because Joki seems to be distracted.
Whiteboard: [MAKINGTEST] Antti.Nayha@oulu.fi
Moving off bugs that won't make M13
Target Milestone: M13 → M14
Summary: accesskey attribute not implemented yet → [FEATURE] accesskey attribute not implemented yet
Target Milestone: M14 → M16
Moving M16.
*** Bug 7559 has been marked as a duplicate of this bug. ***
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 ...
Hardware: PC → All
*** Bug 33318 has been marked as a duplicate of this bug. ***
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?
Nominating for nsbeta2 and adding the 'html4' keyword.
Adding dependency on this bug to bug 7954 (HTML 4.0 compliance meta bug).
Blocks: html4.01
Keywords: html4, nsbeta2
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. 
Whiteboard: [nsbeta2+][5/16]
changing QA contact
QA Contact: cpratt → sairuh
*** Bug 33318 has been marked as a duplicate of this bug. ***
Okay, this is 90% fixed.  Accesskey is in and works for everything except links.  
Working on that last bit.
Rods has generously volunteered to finish this bug, which more or less means 
adding Register and and UnRegister calls for accesskeys on links.
Assignee: joki → rods
Status: ASSIGNED → NEW
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-
Status: NEW → ASSIGNED
Whiteboard: [nsbeta2+][5/16] → [nsbeta2+][5/16] see my comment below (rods)
now regs and unregs in SetDocument - fixed
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
over to janc 9or jrgm?) to verify.
QA Contact: sairuh → janc
This must work for XUL as well.  Should I give myself a separate bug for XUL, or 
do we want to use this bug?

i am now taking this bug for xul.  it's done for html, and now it needs to be 
done for xul.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [nsbeta2+][5/16] see my comment below (rods) → [nsbeta2+]
taking bug...
Assignee: rods → hyatt
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Whiteboard: [nsbeta2+] → [nsbeta2+] done for html, needs to be done for xul
removing nsbeta2, since this bug is now about xul and should probably be 
re-evaluated
Whiteboard: [nsbeta2+] done for html, needs to be done for xul → done for html, needs to be done for xul
moving to m18.  
Target Milestone: M16 → M18
Putting on [nsbeta2-] radar.  
Summary: [FEATURE] accesskey attribute not implemented yet → [FEATURE]Accesskey attribute not implemented yet
Whiteboard: done for html, needs to be done for xul → [nsbeta-]done for html, needs to be done for xul
Whiteboard: [nsbeta-]done for html, needs to be done for xul → [nsbeta2-]done for html, needs to be done for xul
Target Milestone: M18 → M20
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?
> 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.
#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)?

#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.)
Sorry if I sound argumentative.  I agree with you guys :)
> #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.)
Isn't this long done?
*** Bug 43772 has been marked as a duplicate of this bug. ***
*** Bug 43772 has been marked as a duplicate of this bug. ***
I think we'll have to live without this in current release, -> Future
Target Milestone: M20 → Future
Blocks: uaag
Mass update:  changing qacontact to ckritzer@netscape.com
QA Contact: janc → ckritzer
Nominating for nsbeta3.
Keywords: nsbeta3
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.
Whiteboard: [nsbeta2-]done for html, needs to be done for xul → [nsbeta2-][nsbeta3-] done for html, needs to be done for xul
Keywords: access
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???
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.
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
Hey Dan. I suggest you file the above as a separate bug report, if it should be 
done. Otherwise, it will get no attention.
*** Bug 49968 has been marked as a duplicate of this bug. ***
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...
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).
Blocks: 51940
Updating QA Contact.
QA Contact: ckritzer → lorca
Updating Summary to reflect Status since it seems fairly relevant.
Summary: [FEATURE]Accesskey attribute not implemented yet → [FEATURE]Accesskey attribute not implemented yet for XUL
Blocks: robin's
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.
Yep, you're right.  Removing those.
No longer blocks: html4.01
Keywords: html4
Blocks: 18575
Nominating for the new nsbeta1 and removing old [nsbeta2-][nsbeta3-].
Keywords: nsbeta1
Whiteboard: [nsbeta2-][nsbeta3-] done for html, needs to be done for xul → done for html, needs to be done for xul
Blocks: 64457
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.
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).
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.
*** Bug 64730 has been marked as a duplicate of this bug. ***
REMOVING nsbeta2/3 and replacing with nsbeta1 KW.  Also clearing 
Status Whiteboard of nsbeta2/3 result.  Accessibility is important.
Keywords: nsbeta2, nsbeta3
Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.
QA Contact: lorca → gerardok
Blocks: 40759
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!
Keywords: helpwanted
Target Milestone: Future → ---
I agree, this is fairly important for accessibility (although not embedding, 
really).  cc'ing trudelle.
[Really CCing Trudelle]
Summary: [FEATURE]Accesskey attribute not implemented yet for XUL → [FEATURE] Accesskey attribute not implemented yet for XUL
*** Bug 68769 has been marked as a duplicate of this bug. ***
Blocks: 70216
Nominating for mozilla0.9 (this blocks a whole lot of bugs).
Keywords: mozilla0.9
->mozilla1.0, is this absolutely required for accessibility in next rev of the apps?
Target Milestone: --- → mozilla1.0
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.
Blocks: 24873
Blocks: 62703
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).
Attached image The image file
Blocks: 78153
Blocks: 78261
Blocks: 68841
*** Bug 70216 has been marked as a duplicate of this bug. ***
QA contact updated
QA Contact: gerardok → madhur
*** Bug 43756 has been marked as a duplicate of this bug. ***
No longer blocks: robin's
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.
Assignee: hyatt → aegis
Blocks: robin's
Status: ASSIGNED → NEW
Attached patch preliminary (obsolete) — Splinter Review
Oh yeah, forgot to mention... There are some changes to the find search dialog
in there. I used those for testing.
Status: NEW → ASSIGNED
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.
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?

Filed bug 88151 for modifying HTML accesskeys via the DOM.
Blocks: 89143
I don't think this bug blocks bug 41368 any longer-at least not since the
switchover from HTML to XUL.
No longer blocks: robin's
Attached patch second attempt (obsolete) — Splinter Review
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.

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.

Depends on: 90221
Attached patch third patch (obsolete) — Splinter Review
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.
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!
Blocks: 90318
Depends on: 90406
Attached patch fourth patch (obsolete) — Splinter Review
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...
Attached patch fifth patch (obsolete) — Splinter Review
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)
Attached patch sixth patch (obsolete) — Splinter Review
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?
Blocks: 91050
Keywords: fcc508
We can get this in for 0.9.3.
Target Milestone: mozilla1.0 → mozilla0.9.3
Don't do this:

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


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?
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.
Attached patch seventh patch (obsolete) — Splinter Review
pushing to 0.9.4 for aegis.  won't make the 0.9.3 train unfortunately...
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Oops, ignore the change to checkbox.xml.
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.
<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
Attached patch eighth patch (obsolete) — Splinter Review
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.
Attached patch patch numero neuf (obsolete) — Splinter Review
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.
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?
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.

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.
Attached patch patch #10! (obsolete) — Splinter Review
jag?  blake?  wanna r= me?

filed bug 94222 for the freaky radiobuttons
Depends on: 94222
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).
I don't need to see another patch, but reversing the "for" and the "accesskey"
would be cool.  sr=hyatt
Attached patch eleventh patch (obsolete) — Splinter Review
I don't have CVS access anyway, so here's the eleventh patch.
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="">. :)

Attached patch patch 12 (obsolete) — Splinter Review
patch 12 has timeless's whitespace nits.  Nothing else changed.
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).
+        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.
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).
We have no nsXULAtoms::control yet.  ;)  Should I add it?  Does it magically 
Just Work in the parser?
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?
Attached file testcase (XUL) (obsolete) —
Attached file testcase (JS) (obsolete) —
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.
Taking this.
Assignee: aegis → jaggernaut
Status: ASSIGNED → NEW
Attached patch [patch] Umpteenth patch (obsolete) — Splinter Review
This patch includes the patch in bug 94222. This won't work without that patch.

Looking for constructive comments, r= and sr=
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?
->0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Wah. Why?
Too risky for 0.9.4, no immediate need for it, can wait till 0.9.5?
Attached patch Incorporate aegis' remarks (obsolete) — Splinter Review
Removed <radio> specialcasing, fixed comment from 'for' to 'control'. Leaving 
PRUint32 "key" attributes and the redundant "release" comments for another 
patch.
Status: NEW → ASSIGNED
Jag says this is ready for r=/sr=

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

Reviewers? blake? hyatt?
upping severity to blocker since it is blocking work on several accessibility bugs
Severity: normal → blocker
picking this up for the end game
Assignee: jaggernaut → saari
Status: ASSIGNED → NEW
-> 0.9.6
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.5 → mozilla0.9.6
*** Bug 103783 has been marked as a duplicate of this bug. ***
Blocks: 104166
*** Bug 106339 has been marked as a duplicate of this bug. ***
Priority: P2 → P1
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Attached patch Patch updated to trunk (obsolete) — Splinter Review
Attachment #40242 - Attachment is obsolete: true
Attachment #41734 - Attachment is obsolete: true
Attachment #41889 - Attachment is obsolete: true
Attachment #42040 - Attachment is obsolete: true
Attachment #42087 - Attachment is obsolete: true
Attachment #42245 - Attachment is obsolete: true
Attachment #43303 - Attachment is obsolete: true
Attachment #44472 - Attachment is obsolete: true
Attachment #44632 - Attachment is obsolete: true
Attachment #45001 - Attachment is obsolete: true
Attachment #45046 - Attachment is obsolete: true
Attachment #45176 - Attachment is obsolete: true
Attachment #45230 - Attachment is obsolete: true
Attachment #46741 - Attachment is obsolete: true
Attachment #48748 - Attachment is obsolete: true
Attached patch Above -buw (obsolete) — Splinter Review
Attached file simple XUL testcase (obsolete) —
Attachment #45447 - Attachment is obsolete: true
Attachment #45449 - Attachment is obsolete: true
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.
Blocks: 111031
Attached patch Update to nsBoxFrame.cpp (obsolete) — Splinter Review
Attached patch Above, -uw (obsolete) — Splinter Review
+
+    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.
Blocks: 94802
Blocks: 113946
Blocks: 68899
Blocks: 117068
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.
No longer blocks: 117068
Attached patch Latest patch (obsolete) — Splinter Review
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
Attachment #59807 - Attachment is obsolete: true
Attachment #59808 - Attachment is obsolete: true
Attachment #60367 - Attachment is obsolete: true
Attachment #60368 - Attachment is obsolete: true
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.
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.
Back to me again :-)
Assignee: saari → jaggernaut
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.7 → mozilla0.9.9
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.


Blocks: 40761
Hyatt, it looks like this needs sr=.
Can we get this in and file a separate bug for the multi line labels?
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...
Blake, if you could post that I'd appreciate it.
Keywords: nsbeta1
I have a new version that gets rid of mOldAccessKey in the nsBoxFrame case. I'll
attach that.
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).
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.
Also, we should use HasAttr when checking for the |control| attr, and probably
|accesskey| as well.
I'll make those changes.
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.
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.
That would do the trick. I'll but hyatt later.
        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?
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.
Attachment #64816 - Attachment is obsolete: true
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.
Blocks: 121680
Attached patch -uw of aboveSplinter Review
Attachment #68863 - Attachment is obsolete: true
Comment on attachment 70111 [details] [diff] [review]
-uw of above

sr=me
Attachment #70111 - Flags: superreview+
Comment on attachment 70111 [details] [diff] [review]
-uw of above

r=attinasi for the AreaFrame changes
Attachment #70111 - Flags: review+
a=asa (on behalf of drivers) for checkin to 0.9.9
Keywords: mozilla0.9.9+
nsbeta1+ per ADT triage team
Keywords: nsbeta1nsbeta1+
Checked in.
.
Status: NEW → RESOLVED
Closed: 20 years ago18 years ago
Resolution: --- → FIXED
No longer blocks: 113946
*** Bug 113946 has been marked as a duplicate of this bug. ***
This patch doesn't deal with tabpanel accesskeys. Was that just an oversight?
Yeah, that was. Could you file a new bug on that though? It shouldn't be too
hard to add.
QA Contact: madhur → rakeshmishra
Attached file Test case
Test case for xul file
Attachment #59811 - Attachment is obsolete: true
verified on the trunk build 2002-04-30-08-trunk on Windows 2000.
Status: RESOLVED → VERIFIED
Blocks: robin's
Alias: AccesskeyXUL
Alias: AccesskeyXUL → Accesskey-XUL
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.