174889 - Active Accessibility: unable to get keyboard shortcut for Tree Item node

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[dsirnapalli]

Enter the following code in a xul file and open it in Mozilla.
<tree seltype="single" flex="1">
  <treecols>
    <treecol id="sender" label="Sender" flex="1"/>
    <treecol id="subject" label="Subject" flex="2"/>
  </treecols>
  
  <treechildren>
    <treeitem accesskey="i">
      <treerow>
        <treecell label="[e-mail]"/>
        <treecell label="Top secret plans"/>
      </treerow>  
    </treeitem>
    <treeitem>
      <treerow>
        <treecell label="[e-mail]"/>
        <treecell label="Lets do lunch"/>
      </treerow>  
    </treeitem>      
  </treechildren>
</tree>

Open Inspect tool. Tab to the tree item node. Watch the keyboard shortcut in
Inspect tool. It shows empty string. It should show "Alt+i".

Comment 1

[Kyle Yuan]

I wonder if the accesskey can actually work with XUL tree ever?

Comment 2

[dsirnapalli]

hi kyle,
i am able to get accesskey for tree node, treecols node and treecol node. anyway
when i hit the alt+accesskey on the keyboard it doesnot bring focus on to those
nodes. From the xulplanet.com site i found that there is no accesskey attribute
for tree node. but when i use our nsIAccessible api to get the keyboardshortcut
it returns the accesskey. Also i noticed this for other elements. So i dont know
exactly whether tree will support accesskey. if it doesnot support our api
should not return the accesskey even if we put accesskey attribute in the tree tag.

Comment 3

[Kyle Yuan]

taking...

Jan, we are not going to support accesskey for tree.treecol/treeitem..., right?

Comment 4

[Aaron Leventhal]

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

Comment 5

[alexander :surkov (:asurkov)]

I think we shouldn't check @accesskey attribute on element. Instead we should have a method on nsIEventStateManager to check whether there is registered accesskey on the element.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

not tested yet

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 8

[Aaron Leventhal]

Comment on attachment 273543 [details] [diff] [review]
patch2

Evan, do you have time to review this while I'm out of town?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 273543 [details] [diff] [review]
patch2

This doesn't event compile.
When changing an interface, you should update the IID.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

s/event/even/

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 273543 [details] [diff] [review]
patch2

Oops, I downloaded the first patch, not this one.

>diff -u -p -8 -r1.82 nsIEventStateManager.h
Update the IID.

>+FindTargetForAccessKey(nsHashKey *aKey, void *aData, void* aClosure)
...
>+    info->mAccessKey = aKey->HashCode();

Because this relies on the implementation of the HashCode(), it might be good
to add an assertion to RegisterAccessKey which checks that HashCode() returns the
same value which is used there to create the hashkey.

>+nsEventStateManager::GetRegisteredAccessKey(nsIContent* aContent,
...
>+  AccessKeyInfo* info = new AccessKeyInfo(aContent);

Couldn't you stack allocate AccessKeyInfo and then pass &info to Enumerate()


How often GetRegisteredAccessKey is called? If there are many accesskeys registered, it isn't
very fast method  (O(n)).

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> >+nsEventStateManager::GetRegisteredAccessKey(nsIContent* aContent,
> ...
> >+  AccessKeyInfo* info = new AccessKeyInfo(aContent);
> 
> Couldn't you stack allocate AccessKeyInfo and then pass &info to Enumerate()

Not sure I got you. How can I do it?
 
> 
> How often GetRegisteredAccessKey is called? If there are many accesskeys
> registered, it isn't
> very fast method  (O(n)).
> 

It shouldn't be too often. Though if you have an idea how to improve this I can do it.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 273770 [details] [diff] [review]
patch3


>+static PRBool PR_CALLBACK
Not PRBool but PRIntn

>+FindTargetForAccessKey(nsHashKey *aKey, void *aData, void* aClosure)
...
>+  if (aTarget == info->mTarget) {
>+    info->mAccessKey = aKey->HashCode();
>+    return PR_FALSE;
return kHashEnumerateStop;

>+  }
>+
>+  return PR_TRUE;
>+}
return kHashEnumerateNext

See http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsHashtable.h#109

With those, r=me for the content/ changes, assuming that accessibility code doesn't call
GetRegisteredAccessKey all the time.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 16

[Ginn Chen]

Comment on attachment 273777 [details] [diff] [review]
patch4

+  if (!key)
     return NS_OK;
+
+  nsAutoString accesskey(key);

I'd like to see something like
nsAutoString accesskey(PRUnichar(key));

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> (From update of attachment 273777 [details] [diff] [review])
> +  nsAutoString accesskey(key);
> 
> I'd like to see something like
> nsAutoString accesskey(PRUnichar(key));
> 

Why? Is it char_type for nsAutoString?

Comment 18

[Ginn Chen]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 273777 [details] [diff] [review] [details])
> > +  nsAutoString accesskey(key);
> > 
> > I'd like to see something like
> > nsAutoString accesskey(PRUnichar(key));
> > 
> 
> Why? Is it char_type for nsAutoString?
> 
Just help me understand that "key" is an ASCII char.
PRUnichar equals to PRInt16 or PRInt32.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)

> > Why? Is it char_type for nsAutoString?
> > 
> Just help me understand that "key" is an ASCII char.
> PRUnichar equals to PRInt16 or PRInt32.
> 

Ok, I don't mind.

Comment 20

[alexander :surkov (:asurkov)]

Johnny, any plans to review this?

Comment 21

[Aaron Leventhal]

Comment on attachment 273777 [details] [diff] [review]
patch4

Let's try Mats instead. Email him if we don't get a response :)

Comment 22

[Mats Palmgren (inactive)]

A couple nits:
In nsIEventStateManager.h/cpp: please place the * in the parameter type
consistently, follow the establish style for the file, which in this case
is with the type name.

Reading the documentation for nsIEventStateManager::GetRegisteredAccessKey()
and nsAccUtils::GetAccessKeyFor() - it's unclear to me what will be returned
in case there is no accesskey registered for the content.
Please document it explicitly that aKey=0 and NS_OK is returned in that case.

A couple of questions:
You have changed nsAccessible::GetKeyboardShortcut() so that it now
returns NS_OK when the accesskey is not found (was NS_ERROR_FAILURE).
This makes DOMI list keyboardShortcut="" for every accessible node
that does not have an accesskey registered (previously it was absent).
Is this intentional?

Regarding performance, I think we can prune the number of calls to ESM
by using (in GetKeyboardShortcut()):

if (!content->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::accesskey))
  return 0;

AFAIK, noone registers an accesskey that doesn't come from that attribute.

This bug is really about the opposite case isn't it? - we have an accesskey
attr but the content chose not to register it for some reason - please
correct me if I misunderstood something.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch5

with mats recommendations

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #22)

> This bug is really about the opposite case isn't it? - we have an accesskey
> attr but the content chose not to register it for some reason - please
> correct me if I misunderstood something.
> 

you're right.

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 278267 [details] [diff] [review]
patch5

Index: accessible/src/base/nsAccessibilityUtils.cpp
+  // whether it is presented on the given element to avoid not fast call of
+  // nsIEventStateManager::GetRegisteredAccessKey() method.

s/not fast call of/the slow/


Index: accessible/src/base/nsAccessible.cpp
+nsAccessible::GetKeyboardShortcut(nsAString& aAccessKey)
...
+  if (!key)
     return NS_OK;

Please add aAccessKey.Truncate() before the return to clear any
existing value (you don't need it when returning a failure code though).


Index: accessible/src/xul/nsXULMenuAccessible.cpp
     if (accesskey.IsEmpty())
       return NS_OK;

_retval.Truncate() before the return


Index: content/events/public/nsIEventStateManager.h
+  /**
+   * Get accesskey registered on the given element or 0 if there is no
+   * an accesskey.

s/no an accesskey/none/


Index: content/events/src/nsEventStateManager.cpp
+FindTargetForAccessKey(nsHashKey *aKey, void *aData, void* aClosure)

nsHashKey* and void*

+nsEventStateManager::GetRegisteredAccessKey(nsIContent* aContent,
+                                            PRUint32 *aKey)

PRUint32*


Index: content/events/src/nsEventStateManager.h
+  NS_IMETHOD GetRegisteredAccessKey(nsIContent* aContent, PRUint32 *aKey);
 
PRUint32*


r+sr=mats with the above addressed.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #22)

> A couple of questions:
> You have changed nsAccessible::GetKeyboardShortcut() so that it now
> returns NS_OK when the accesskey is not found (was NS_ERROR_FAILURE).
> This makes DOMI list keyboardShortcut="" for every accessible node
> that does not have an accesskey registered (previously it was absent).
> Is this intentional?

Sorry, forgot to answer. Yes I did this intentional. We should return NS_OK when there is not error actually. It makes the js life easier. It's another issue with DOMi. I think I should file bug for this for investigation.

Comment 27

[alexander :surkov (:asurkov)]

Attachment: patch6

mats comments are addressed

Comment 28

[alexander :surkov (:asurkov)]

checked in

Comment 29

[Aaron Leventhal]

I think this probably caused bug 394501.