Currently, an accesskey must be unique for an entire dialog, even though a user only sees widgets within one tabpanel at a time. It is difficult enough to determine unique accesskeys within one visible dialog context (especially in complicated dialogs such as Composer's). Not being able to reuse a letter in different panels makes it *very* difficult to find unique accesskeys.

Note that accesskeys should not be limited to panes (bug 64606).

yes, it is really a defect. one example that upsets me is when in composer, the "table properties" window's "Advanced *E*dit" accesskey works strangely. There are two tabs--"table" and "cells" . Each one has a "Advanced *E*dit", but table's accesskey does not work while cells's accesskey works. I think one way to resolve this bug is to register the accesskey with its tabpanel's corresponding nsIFrame( maybe not right, because i am not familiar with the layout).

I think this is an important issue because there are a lot of dialogs having tabpanel and there are still buttons needing accesskeys. If this bug can't be fixed, the work around is to select different keys but it is difficult for users. So any ideas about this bug?

It looks to me like the code is doing the right think. It walks up the parent tree and if any parent view is hidden, the accesskey isn't processed. http://lxr.mozilla.org/mozilla/source/content/events/src/nsEventStateManager.cpp#869 This is the same thing that nsEventStateManager::GetNextTabbableContent does, and that works fine. Then there's the additional step of making sure the element itself is visible. http://lxr.mozilla.org/mozilla/source/content/events/src/nsEventStateManager.cpp#905 Shouldn't one of these checks boot us out?

Gilbert, you did some low-level work with accesskeys didn't you? Would you consider taking this bug?

Note specific to tabpanels. Looks more like a problem with -moz-deck tabpanels { -moz-binding: url("chrome://global/content/bindings/tabbox.xml#tabpanels"); display: -moz-deck; } Anyone know how we can tell which element is being displayed for -moz-deck?

It looks like deck views are shown/hidden by this code in nsDeckFrame: http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsDeckFrame.cpp#45

I don't think it's a problem with the view being hidden, my bet is that Mozilla only allows one accesskey to be registered per window or something...

Neil, I believe it's per event state manager, but that's probably it. Not 1 accesskey per esm, but each esm can have any given accesskey once.

So how do menupopups handle it when several menuitems have duplicate keys?

If there are duplicate access keys in a single popup, I believe the first menu item is executed. If there are no specific access keys defined, we fall into menu-specific code that uses the first letter of the menu item as the access key.

Dean's right. The code for handling accesskeys is entirely separate for menus.

I assume it would be too inefficient to unregister and reregister access keys every time we flip a panel? So, instead, we need lists of access keys for each ESM?

anyone want to write a test case?

accesskey only works for tab2, not for tab1

I don't know where I was going in comment 4. The visibility checking part works. The accesskey only functions when the tab is active. Neil's on the right track.

Annoyingly, the <xul:button>-specific code to allow keypresses to turn into access keys doesn't follow the rules, so if you press B when either button has focus then that button is clicked but if you press Alt+B then that only works on the second button. Maybe we should move the access key code into the XUL button frame C++ code so that it can call on the ESM to handle the access key.

key reason here is that for each esm, member variable mAccessKeys can only register one accesskey for one content. so if multiple content share one accesskey, only the last one can be triggered with accesskey. several possible solutions are: a)make another data structure as hast set, make it possible to share one hash key among multiple content. then use that structure to replace hashtable here. this may bring too many changes on the trunk, very dangerous. however, make the systme more flexible. b)once a tabpanel shows/hides, reregister accesskey for visible content only. as neil's comment#13, perfomance issue may exist. c)use content address*1000+accessky as hash key to register accesskey. the hash key is unique, so there is no conflict among contents using the same accesskey. problem is that once fetch the content, we lost the advantage of hashtable. we must enumerate each key to get the content. d)use some other sturctures to register the situation that multiple contents share one accesskey. and use mAccessKeys only register one-one map relation. when HandleAccessKey, first search if multiple contents share one accesskey, then search mAccessKeys i can patch it with method c and d. what's your options? your choice, my patch^_^

Well, I suppose that if you already have an access key registered (I see some DEBUG_jag code that tests for that) you could switch from an nsIContent to an nsISupportsArray (putting both the original nsIContent and the new one in the array of course). Alternatively you could decide that the overhead of keeping registration lists in an nsClassHashtable of nsCOMArrays would be negligible.

(In reply to comment #17) > Annoyingly, the <xul:button>-specific code to allow keypresses to turn into > access keys doesn't follow the rules ... Another flaw with that code is that it should also work for radio buttons and checkboxes. The rule should be: if you're currently focused a widget that doesn't need unmodified character keystrokes to mean anything, then you can fire the accesskey by just pressing the character without the modifier. I realize that should be filed as a separate bug, but if the code is being moved into C++ anyway, perhaps that can be fixed at the same time.

neil, esm use nsSupportsHashTable which use nsHashTable to register accesskey. in my understanding, what you suggested "switch from an nsIContent to an nsISupportsArray" means the change of that structrues and related method implementation. is it acceptable? if i understand correctly, your suggest to use nsClassHashtable to register extra nsIContent using same accesskey. i don't think it's reliable since there may be more than two nsIContent sharing same accesskey. corret me if i'm wrong. neil and aaron, which solution of my four do you think is better acceptable? others solution is welcomed

(In reply to comment #21) >neil, esm use nsSupportsHashTable which use nsHashTable to register accesskey. >in my understanding, what you suggested "switch from an nsIContent to an >nsISupportsArray" means the change of that structrues and related method >implementation. is it acceptable? You can store an nsISupportsArray in an nsSupportsHashTable, if that's what you mean. You can even store both and use QueryInterface to find out which one you've got. >if i understand correctly, your suggest to use nsClassHashtable to register >extra nsIContent using same accesskey. i don't think it's reliable since there >may be more than two nsIContent sharing same accesskey. corret me if i'm wrong. With the nsClassHashtable you would store an nsCOMArray (instead of the nsISupportsArray in the option above) for each access key, and list all the nsIContent sharing the same accesskey in that array.

neil, for both option 1&2, are there any examples that i can refer to? i'd like to have a chance to be familar with both of them.

I have a 3/4 baked patch for this. It works but hasn't been cleaned up yet. It also fixes accesskeys so that if there are >=2 items with the same accesskey that are both visible, we will cycle through them without activating. It's a fairly complex page that is probably too much to take it at this part of the cycle, and I think it should wait until the branch for Mozilla 1.9 opens.

Comment on attachment 191964 [details] [diff] [review] Posting feature-complete patch for general feeback. Not refined for review yet. >+ contentArray->RemoveElementAt(index); >+ PRUint32 length; >+ contentArray->Count(&length); >+ if (length == 0) { >+ mAccessKeys->Remove(&key); >+ } Alternatively if length == 1 you could get the single element and set it as the content. >- if (document.commandDispatcher.focusedElement != this.inputField) >+ if (document.commandDispatcher.focusedElement != this.inputField) { > this.inputField.focus(); >+ if (!this.hasAttribute('multiline')) { >+ this.select(); // Allow accesskey to select for single line textboxes >+ } >+ } I really hope you're not seriously suggesting this change...

tField.focus(); > >+ if (!this.hasAttribute('multiline')) { > >+ this.select(); // Allow accesskey to select for single line textboxes > I really hope you're not seriously suggesting this change... I suppose your objecting to the code itself not the behavior? Using an accesskey should select the contents of a textfield (similar to Alt+D for the address bar). I'm not sure what's sooo evil about those lines that they don't deserve an explanation of what's wrong with them. I must be a terrible coder for not grasping that instantly? Perhaps you'd like to suggest the right way and help out?

Comment on attachment 191964 [details] [diff] [review] Posting feature-complete patch for general feeback. Not refined for review yet. >Index: toolkit/content/widgets/textbox.xml >+ if (!this.hasAttribute('multiline')) { >+ this.select(); // Allow accesskey to select for single line textboxes >+ } >+ } Please note bug 172203. The pref probably counts for textfields focused with an accesskey as well as tab (although I'm not sure), seeing as the signal is called 'gtk-entry-select-on-focus', and not 'gtk-entry-select-when-tabbed-to'.

Not really a blocker, depends on focus changes that would need a lot of trunk baking to be considered later.

>I suppose your objecting to the code itself not the behavior? No, I'm objecting to the behaviour. That code block runs for other sorts of focus as well as access keys.

Comment on attachment 191964 [details] [diff] [review] Posting feature-complete patch for general feeback. Not refined for review yet. >+ nsCOMPtr<nsIDOMXULElement> xulElt(do_QueryInterface(content)); >+ if (xulElt) { >+ if (shouldActivate) { >+ xulElt->Click(); > } >+ else { >+ xulElt->Focus(); >+ } >+ return; >+ } Hmm... how does this handle a toolbarbutton with a duplicate access key ;-)

I guess you also want to select editable menulists as well as single line textboxes, so maybe you need something like this (written from memory, not tested or even compiled): nsCOMPtr<nsIDOMXULElement> xulElt(do_QueryInterface(content)); if (xulElt) { nsCOMPtr<nsIDOMNode> inputField; nsCOMPtr<nsIDOMXULTextBoxElement> textBox(do_QueryInterface(xulElt); if (textBox) textbox->GetInputField(inputField); else { nsCOMPtr<nsIDOMXULMenuListElement) menuList(do_QueryInterface(xulElt); if (menuList) menuList->GetInputField(inputField); else shouldActivate = PR_TRUE; } nsCOMPtr<nsIDOMHTMLInputElement> input(do_QueryInterface(inputField)); if (input) { input->Focus(); input->Select(); } else if (content->Tag() != nsXULAtoms::toolbarbutton) { xulElt->Focus(); } return; } The other alternative is that I have misunderstood the rest of your C++, and in fact what's happening is that although toolbarbuttons, editable menulists and textboxes register an access key it is ignored because the control itself does not take focus. In the latter cases the inner HTML form control takes focus, which means that ChangeFocusWith already handles calling Select for you anyway.

Aarons patch updated to trunk + 1. let the "if (xulElt) {" block fall through to the "*aStatus = nsEventStatus_eConsumeNoDefault;" we currently have, otherwise we would activate both a button and a menu on platforms where ui.key.generalAccessKey == ui.key.menuAccessKey (eg Linux) 2. added a "IsFocusable()" as a condition for doing "ChangeFocusWith()" in the HTML case. 3. changed the "xulElt->Focus()" to "ChangeFocusWith()" so we get the selection for free where needed. 4. a few other minor improvements...

Comment on attachment 228838 [details] [diff] [review] Patch rev. 2 >+ if (!content) { ...much snipped... >+ else { >+ if (!IsAccessKeyTarget(content)) { >+ return; >+ } >+ shouldActivate = ShouldActivateFromAccessKey(content); >+ } I found this really confusing to read; I suggest switching the clauses. >- if (atom == nsXULAtoms::textbox || atom == nsXULAtoms::menulist) { >- // if it's a text box or menulist, give it focus >- element->Focus(); >- } else if (atom == nsXULAtoms::toolbarbutton) { >- // if it's a toolbar button, just click >- element->Click(); >- } else { >- // otherwise, focus and click in it >- element->Focus(); >- element->Click(); >+ nsCOMPtr<nsIDOMXULElement> xulElt(do_QueryInterface(content)); >+ if (xulElt) { >+ if (shouldActivate) { >+ xulElt->Click(); > } >+ else { >+ ChangeFocusWith(content, eEventFocusedByKey); > } > } You changed the meaning of the code here, and the new code no longer focuses (when focusable) activated elements e.g. radiobuttons. r=me with this fixed.

(In reply to comment #34) > I found this really confusing to read; I suggest switching the clauses. Fixed by swapping the clauses. > You changed the meaning of the code here, and the new code no longer > focuses (when focusable) activated elements e.g. radiobuttons. Good catch. This patch should correspond to the old code except for the case where we have multiple targets and we're looking at a toolbarbutton, then we need to focus it for the cycling to work. I also found an additional problem. In some cases radio buttons would not activate at all - for example "Preferences->Navigator->Display On: Home Page". The reason was that it had been registered twice (so it looked like we had multiple controls with the same key and we will not activate in that case). This bug is not in ESM but rather in the notification of mutation events - I'll file a separate bug on it. For now, I added a check in nsEventStateManager::RegisterAccessKey that it isn't already registered (with a NS_WARNING).

(filed bug 344453 on the mutation notification problem)

Comment on attachment 229019 [details] [diff] [review] Patch rev. 3 >+nsIContent* >+nsEventStateManager::GetTargetForAccessKey(nsIContent* aAccessKeyContent) >+{ >+ nsCOMPtr<nsIContent> content; >+ if (aAccessKeyContent->IsNodeOfType(nsINode::eXUL) && >+ aAccessKeyContent->Tag() == nsXULAtoms::label) { >+ // If anything fails, this will be null ... >+ nsCOMPtr<nsIDOMElement> element; >+ nsAutoString control; >+ aAccessKeyContent->GetAttr(kNameSpaceID_None, nsXULAtoms::control, control); >+ if (!control.IsEmpty()) { >+ nsCOMPtr<nsIDOMDocument> domDocument = >+ do_QueryInterface(aAccessKeyContent->GetDocument()); >+ if (domDocument) { >+ domDocument->GetElementById(control, getter_AddRefs(element)); >+ } >+ } >+ // ... that here we'll either change |content| to the element >+ // referenced by |element|, or clear it. >+ content = do_QueryInterface(element); >+ } >+ else { >+ // Not a XUL label >+ // For HTML labels we can use the <label> content for focus/activation >+ // and it will transfer to the appropriate control. >+ // XXX Need patch for bug 142898 to expose GetForContent >+ content = aAccessKeyContent; >+ } >+ return content; >+} I thought this was just cut-n-paste but now you've changed it it's open season ;-) What do you think of this version: return nsCOMPtr<nsIContent>(do_QueryInterface(element)); } // Not a XUL label ... return aAccessKeyContent; >+ // Don't focus a toolbar button with a unique accesskey, just click. >+ if (!(shouldActivate && content->Tag() == nsXULAtoms::toolbarbutton)) { I found that non-obvious, but usefully shorter than this equivalent: if (!shouldActivate) { ChangeFocusWith(...); } else { if (!toolbarbutton) ChangeFocusWith(...); xulElt->Click(); } Or we could of course ignore toolbarbutton focus: if (!toolbarbutton) ChangeFocusWith(...); if (shouldActivate) xulElt->Click(); Again, what do you think?

(In reply to comment #37) > What do you think of this version: Even better, thanks. (I removed the comments also which I didn't think added any value) > >+ // Don't focus a toolbar button with a unique accesskey, just click. > >+ if (!(shouldActivate && content->Tag() == nsXULAtoms::toolbarbutton)) { > > I found that non-obvious, but usefully shorter than this equivalent: I prefer my version, but perhaps we can make the comment clearer? How about: // Always focus the element except for a toolbar button with a unique // accesskey (which we will activate). > Or we could of course ignore toolbarbutton focus That would break cycling through toolbar buttons with the same accesskey.

Comment on attachment 229311 [details] [diff] [review] Patch rev. 4 Possible suggestion: // As a special case we don't focus a toolbarbutton if its accesskey is unique, instead we just click it.

Comment on attachment 229311 [details] [diff] [review] Patch rev. 4 Ok, I'll change the comment as you suggested before checkin.

Comment on attachment 229311 [details] [diff] [review] Patch rev. 4 I'm not going to have time for this one, sorry.

I'm not going to be able to get to this in a reasonable timeframe. Please have someone more familiar with the ESM (roc and bryner come to mind?) look at this?

+nsIContent* +nsEventStateManager::GetTargetForAccessKey(nsIContent* aAccessKeyContent) Can't this just be a static function? Or possibly in nsContentUtils? +PRBool +nsEventStateManager::IsAccessKeyTarget(nsIContent* aContent) +{ + nsIFrame* frame = mPresContext->PresShell()->GetPrimaryFrameFor(aContent); + + if (frame) { Instead, do an early exit "if (!frame)". + nsIAtom* tag = aContent->Tag(); + return (tag != nsXULAtoms::textbox && tag != nsXULAtoms::menulist); Instead of calling Tag() in various places, should we perhaps be using the XBL service's ResolveTag --- is this doing the right thing if someone tries to write an XBL binding that resolves to, say, a XUL textbox or menulist? + for (count = start + 1; count < length + start + 1; ++count) { I think it would work out more simply if you let count run from 1 to length and add 'start' when you compute the array index. + if (shouldActivate && length > 0 && count != (PRUint32)start) { Can count ever be equal to start here? I think not. I doubt that it's worth storing mAccessKeys. Access key lookup is not frequent, we can afford to traverse the document's content (and frames, although I feel that access keys should be associated with content alone) to find element(s) with a given access key. Also, the scheme used here seems fragile; for example the order of elements in a list for an access key would vary depending on how elements are dynamically added to a document, and hence so would the access key behaviour.

(In reply to comment #43) >+ nsIAtom* tag = aContent->Tag(); >+ return (tag != nsXULAtoms::textbox && tag != nsXULAtoms::menulist); >Instead of calling Tag() in various places, should we perhaps be using the XBL >service's ResolveTag --- is this doing the right thing if someone tries to >write an XBL binding that resolves to, say, a XUL textbox or menulist? Actually the menulist binding resolves to a menu (because it wants a menu frame) but what you could do is to QI to the appropriate XUL element interfaces.

ccing mconnor since we had fun with the preferences accesskeys for 2

Comment on attachment 229311 [details] [diff] [review] Patch rev. 4 clearing request until comments are addressed

Any chance someone, MConnor? can address ROC's comments? It would be great to finally resolve this long standing issue. Currently accesskeys are hit or miss when navigating through Tools->Options as there are accesskeys that use the same letter across panels... ~B

The patch only needs someone to merge it to trunk and deal with Roc's comments.

Apparently, this bites us in toolkit-style pref windows if different pref panels have the same accesskeys defined and a user opens both of them sequentially. This means it probably bits lots of builds out there, localized or not, and multiple applications, including Firefox, Thunderbird and SeaMonkey. I wonder why a P1 bug that probably bites a wide audience gets marked blocking-, but anyways, can we get some movement on this? It's still marked wanted-1.9, right?

(In reply to comment #49) > The patch only needs someone to merge it to trunk Only that "only" is only nicely said: there's a difference of 67(!) revisions between Mats' last patch and reality! Especially the changes to nsEventStateManager::HandleAccessKey and nsEventStateManager::(Un)RegisterAccessKey are almost completely stale and need to be rewritten by someone who knows his whereabouts there... IOW: I tried to update it to trunk (let alone fix roc's comments!) and failed.

(In reply to comment #43) > I doubt that it's worth storing mAccessKeys. Access key lookup is not frequent, > we can afford to traverse the document's content (and frames, although I feel > that access keys should be associated with content alone) to find element(s) > with a given access key. Also, the scheme used here seems fragile; for example > the order of elements in a list for an access key would vary depending on how > elements are dynamically added to a document, and hence so would the access key > behaviour. > Mats approach doesn't require a lot of changes and it should be safe enough with respect to regressions. Roc, are you ok with the current approach if I would update the patch to trunk?

Merged to tip, and converted from nsISupportsArray to nsIArray. Also: >>+nsIContent* >>+nsEventStateManager::GetTargetForAccessKey(nsIContent* aAccessKeyContent) >Can't this just be a static function? Or possibly in nsContentUtils? Fixed. >>+ if (frame) { >Instead, do an early exit "if (!frame)". Fixed. >>+ for (count = start + 1; count < length + start + 1; ++count) { >I think it would work out more simply if you let count run from 1 to length >and add 'start' when you compute the array index. Fixed. >>+ if (shouldActivate && length > 0 && count != (PRUint32)start) { >Can count ever be equal to start here? I think not. Fixed. Not fixed: use of tags to identify toolbarbuttons, textboxes and menulists. ResolveTag is no use here, because toolbarbutton resolves to button and menulist resolves to menu. Not fixed: Complete rewrite of algorithm to walk content and/or frames.

Neil, I'm not sure the approach is 100% good now as it was. Logic to find a content and invoke an accesskey has been moved out nsEventStateManager. Therfore GetTargetForAccessKey and IsAccessKeyTarget aren't looked actual any more. Also, I don't get nsXULElement::PerformAccesskey changes. Why are they?

(In reply to comment #54) >Neil, I'm not sure the approach is 100% good now as it was. Logic to find a >content and invoke an accesskey has been moved out nsEventStateManager. >Therefore GetTargetForAccessKey and IsAccessKeyTarget aren't looked actual any >more. Also, I don't get nsXULElement::PerformAccesskey changes. Why are they? Both for the same reason: multiple possible elements. Firstly you need to see how many of them are "live". If only one is live, then processing continues as if there was only one access key. However if more than one access key is live then it is essential that none of the elements is activated, only focused.

In any way GetTargetForAccessKey() sprays the logic through the code when it tries to find content for an accesskey. Why shouldn't element be activeted but focused? Why we are not happy when we get first live content? Just it sounds for me correct to find first visible content and then activate it. Why not?

(In reply to comment #56) >Why shouldn't element be activeted but focused? Why we are not happy when we >get first live content? Just it sounds for me correct to find first visible >content and then activate it. Why not? The idea is that if you have multiple elements with the same access key then you will cycle focus through them without actually activating any of them. (This is how things work on Windows if you define duplicate access keys.)

We also do this with menus - if you have duplicate access keys in a menupopup, pressing the key does not activate any item but merely cycles through them.

Ah, ok I see. But if I have two elements with the same accesskey and the first element is in inactive tab (invisible) then the second element will be activated, right?

That's right, we'll activate if exactly one content passes the checks.

I can see one problem with the patch. nsIXTFElement::performAccesskey (http://lxr.mozilla.org/mozilla/source/content/xtf/public/nsIXTFElement.idl#159) doesn't have 'shoudActivate' argument, therefore for example, xforms trigger (button analogue) will be clicked in any way (http://lxr.mozilla.org/mozilla/source/extensions/xforms/resources/content/xforms.xml#476).

Another thing I don't like is GetTargetForAccessKey() and IsAccessKeyTarget() dublicate code from nsXULElement::PerformAccesskey.

(In reply to comment #61) >I can see one problem with the patch. nsIXTFElement::performAccesskey >(http://lxr.mozilla.org/mozilla/source/content/xtf/public/nsIXTFElement.idl#159) >doesn't have 'shouldActivate' argument, therefore for example, xforms trigger >(button analogue) will be clicked in any way I guess nsXTFElementWrapper needs to be fixed to honour its parameters.

This XPCOM array stuff is ugly. Would it really be harder to just traverse the document?

(In reply to comment #64) >Would it really be harder to just traverse the document? I'd need to completely rewrite all the logic for which elements register access keys. Currently we register access keys as follows: HTML: <a> <area> <label> <legend> plus all form controls XUL: <button> <checkbox> <radio> <tab> <textbox> <toolbarbutton> XTF: <label> <select> and possibly others, I'm not quite sure (In reply to comment #54) >Also, I don't get nsXULElement::PerformAccesskey changes. Why are they? Thinking about it, they're wrong, I should just call ChangeFocus instead.

>>Also, I don't get nsXULElement::PerformAccesskey changes. Why are they? >Thinking about it, they're wrong, I should just call ChangeFocus instead. Usually using an accesskey mnemonic just acts the same as clicking does, unless there are multiple elements with the same mnemonic.

(In reply to comment #66) >>>Also, I don't get nsXULElement::PerformAccesskey changes. Why are they? >>Thinking about it, they're wrong, I should just call ChangeFocus instead. >Usually using an accesskey mnemonic just acts the same as clicking does, unless >there are multiple elements with the same mnemonic. Yes, but my original approach of calling PerformAccesskey (sic) doesn't deal with toolbarbuttons correctly, and neither does calling ChangeFocus. (In reply to comment #64) >Would it really be harder to just traverse the document? How about I dump the hashtable and just use an nsCOMArray of registered content?

This doesn't handle multiple elements with duplicate access keys correctly, if any of them are textboxes or radiobuttons.

This just puts all the elements that register access keys into a single array and then searches the array for elements with the given key when it is pressed.

This looks great.

Comment on attachment 290233 [details] [diff] [review] Alternate patch FYI, this patch does not compile. The change to nsEventStateManager.h is missing. With that fixed there is also a compile error: nsXTFElementWrapper.cpp:545: error: ‘presContext’ was not declared in this scope

Sorry for uploading the wrong patch last time... this one compiles too :-)

Comment on attachment 290374 [details] [diff] [review] Correct patch a=beltzner for drivers

Fix checked in.

Neil, was the change to nsEventStateManager::MoveCaretToFocus() intentional? It wasn't present in the patch that was reviewed and I see no motivation why it's needed or how it relates to access keys. (It seems to have caused bug 414018 (crash).)