143065 - Scope of accesskey should be limited to a tab panel/-moz-deck

Status: RESOLVED FIXED

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

Description

[Charles Manske]

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.

Comment 1

[Jesse Ruderman]

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

Comment 2

[Gilbert Fang]

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). 

Comment 3

[Jessie Li]

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?

Comment 4

[:deanis74]

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?

Comment 5

[Aaron Leventhal]

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

Comment 6

[Aaron Leventhal]

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?

Comment 7

[Aaron Leventhal]

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

Comment 8

[[e-mail]]

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...

Comment 9

[Aaron Leventhal]

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.

Comment 10

[[e-mail]]

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

Comment 11

[:deanis74]

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.

Comment 12

[Aaron Leventhal]

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

Comment 13

[[e-mail]]

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?

Comment 14

[Nian Liu(n/a in a long time)]

anyone want to write a test case?

Comment 15

[Nian Liu(n/a in a long time)]

Attachment: test case

accesskey only works for tab2, not for tab1

Comment 16

[:deanis74]

Attachment: testcase with description

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.

Comment 17

[[e-mail]]

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.

Comment 18

[Nian Liu(n/a in a long time)]

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^_^

Comment 19

[[e-mail]]

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.

Comment 20

[Aaron Leventhal]

(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.

Comment 21

[Nian Liu(n/a in a long 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

Comment 22

[[e-mail]]

(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.

Comment 23

[Nian Liu(n/a in a long time)]

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. 

Comment 24

[Aaron Leventhal]

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 25

[Aaron Leventhal]

Attachment: Posting feature-complete patch for general feeback. Not refined for review yet.

Comment 26

[[e-mail]]

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...

Comment 27

[Aaron Leventhal]

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 28

[Vidar Haarr (not reading bugmail)]

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'.

Comment 29

[Mike Connor [:mconnor]]

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

Comment 30

[[e-mail]]

>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 31

[[e-mail]]

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 ;-)

Comment 32

[[e-mail]]

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.

Comment 33

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

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 34

[[e-mail]]

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.

Comment 35

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

(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).

Comment 36

[Mats Palmgren (inactive)]

(filed bug 344453 on the mutation notification problem)

Comment 37

[[e-mail]]

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?

Comment 38

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

(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 39

[[e-mail]]

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 40

[Mats Palmgren (inactive)]

Comment on attachment 229311 [details] [diff] [review]
Patch rev. 4

Ok, I'll change the comment as you suggested before checkin.

Comment 41

[Brian Ryner (not reading)]

Comment on attachment 229311 [details] [diff] [review]
Patch rev. 4

I'm not going to have time for this one, sorry.

Comment 42

[Boris Zbarsky [:bzbarsky]]

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?

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+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.

Comment 44

[[e-mail]]

(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.

Comment 45

[Majken Connor [:Kensie]]

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

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 229311 [details] [diff] [review]
Patch rev. 4

clearing request until comments are addressed

Comment 48

[Bryan]

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

Comment 49

[Aaron Leventhal]

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

Comment 50

[Robert Kaiser]

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?

Comment 51

[Karsten Düsterloh]

(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.

Comment 52

[alexander :surkov (:asurkov)]

(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?

Comment 53

[[e-mail]]

Attachment: Updated patch

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.

Comment 54

[alexander :surkov (:asurkov)]

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?

Comment 55

[[e-mail]]

(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.

Comment 56

[alexander :surkov (:asurkov)]

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?

Comment 57

[[e-mail]]

(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.)

Comment 58

[[e-mail]]

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.

Comment 59

[alexander :surkov (:asurkov)]

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?

Comment 60

[[e-mail]]

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

Comment 61

[alexander :surkov (:asurkov)]

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).

Comment 62

[alexander :surkov (:asurkov)]

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

Comment 63

[[e-mail]]

(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.

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 65

[[e-mail]]

(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.

Comment 66

[Aaron Leventhal]

>>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.

Comment 67

[[e-mail]]

(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?

Comment 68

[[e-mail]]

Attachment: Prototype patch

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

Comment 69

[[e-mail]]

Attachment: Alternate patch

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.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This looks great.

Comment 71

[Mats Palmgren (inactive)]

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

Comment 72

[[e-mail]]

Attachment: Correct patch

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

Comment 73

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 290374 [details] [diff] [review]
Correct patch

a=beltzner for drivers

Comment 74

[[e-mail]]

Fix checked in.

Comment 75

[Mats Palmgren (inactive)]

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).)