672092 - Access keys not exposed as part of AtkAction

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: test case

Steps to reproduce:
1. View the test case in Firefox
2. In a terminal window, launch the test script
3. Return focus to Firefox

Expected results: For the two links (which have access keys), the key would be shown in the terminal window.

Actual results: For the two links, the key is not shown. Only the semicolon (that separates the keybindings associated with an AtkAction) is shown.

I can reproduce this issue both with Firefox 5 which ships with Fedora 15 and the nightly (8.0a1 from 2011-07-16).

Comment 1

[Joanmarie Diggs]

Attachment: test script

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch2

Trevor, could you check if linux part work as expected please?

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 5

[[e-mail]]

Comment on attachment 546430 [details] [diff] [review]
patch

>+    keyBinding.ToString(str, KeyBinding::eAtkFormat);
>+    keyBindingsStr.Append(str);
ToString truncates its string. If it didn't do that, or if there was a version that didn't do that, then you could call keyBinding.AppendToString(keyBindingsStr);

>+  KeyBinding keyBinding = AccessKey();
>+  keyBinding.ToString(accesskey);
ToString is const, so you should be able to write this as AccessKey().ToString(accesskey);

>+void
>+KeyBinding::ToAtkFormat(nsString& aValue) const
>+{
>+  aValue.Assign(mKey);
>+
>+  nsAutoString modifierName;
>+  if (mModifierMask & kControl)
>+    aValue.Insert(NS_LITERAL_STRING("<Control>"), 0);
>+
>+  if (mModifierMask & kAlt)
>+    aValue.Insert(NS_LITERAL_STRING("<Alt>"), 0);
>+
>+  if (mModifierMask & kShift)
>+    aValue.Insert(NS_LITERAL_STRING("<Shift>"), 0);
>+
>+  if (mModifierMask & kMeta)
>+    aValue.Insert(NS_LITERAL_STRING("<Meta>"), 0);
Append is cheaper than Insert, and this would be an easy rewrite.

>+  inline void ToString(nsString& aValue, Format aFormat = ePlatformFormat) const
...
>+  void ToPlatformFormat(nsString& aValue) const;
>+  void ToAtkFormat(nsString& aValue) const;
Nothing here seems to require an nsString&; if you switched to nsAString& then you could avoid some temporary strings.

>+      aFormat == ePlatformFormat ?
>+        ToPlatformFormat(aValue) : ToAtkFormat(aValue);
I'm actually surprised this compiles, but I don't like the style anyway.

>+  *(aKeyBinding[0]) = ::SysAllocStringLen(keyStr.get(), keyStr.Length());
>+  if (!*(aKeyBinding[0])) {
>+    nsMemory::Free(*aKeyBinding);
Shouldn't this say ::SysFreeString like the previous code did? (I have to admit I only looked closely at the code relating to the new KeyBinding class and only happened to notice this by chance.)

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> >+  *(aKeyBinding[0]) = ::SysAllocStringLen(keyStr.get(), keyStr.Length());
> >+  if (!*(aKeyBinding[0])) {
> >+    nsMemory::Free(*aKeyBinding);
> Shouldn't this say ::SysFreeString like the previous code did? (I have to
> admit I only looked closely at the code relating to the new KeyBinding class
> and only happened to notice this by chance.)

::SysFreeString was used to free memory allocated for all strings except failed one, so we failed to allocate memory for this string and thus no need to free it.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 8

[Marco Zehe (:MarcoZ)]

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

>+        is(menu.keyboardShortcut, "Alt+u",
>+           "Wrong accesskey on " + prettyName(this.menuitemNode));

Will this not fail on Mac OS since the accellerator key is different there?

>+        is(menuitem.defaultKeyBinding, "Ctrl+l",
>+           "Wrong keyboard shortcut on " + prettyName(this.menuitemNode));

The same?

f=me with the above answered/adjusted.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Comment on attachment 546466 [details] [diff] [review] [review]
> patch2
> 
> >+        is(menu.keyboardShortcut, "Alt+u",
> >+           "Wrong accesskey on " + prettyName(this.menuitemNode));
> 
> Will this not fail on Mac OS since the accellerator key is different there?

because mac tests aren't running on try :) I'll adjust it.

> >+        is(menuitem.defaultKeyBinding, "Ctrl+l",
> >+           "Wrong keyboard shortcut on " + prettyName(this.menuitemNode));
> 
> The same?

no, because control is explicitly specified in modifiers.

> f=me with the above answered/adjusted.

Marco, please check MSAA part. The logic has been changed.

Comment 10

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #5)
> > >+  *(aKeyBinding[0]) = ::SysAllocStringLen(keyStr.get(), keyStr.Length());
> > >+  if (!*(aKeyBinding[0])) {
> > >+    nsMemory::Free(*aKeyBinding);
> > Shouldn't this say ::SysFreeString like the previous code did? (I have to
> > admit I only looked closely at the code relating to the new KeyBinding class
> > and only happened to notice this by chance.)
> 
> ::SysFreeString was used to free memory allocated for all strings except
> failed one, so we failed to allocate memory for this string and thus no need
> to free it.

Ah, I failed to notice that you had removed the deallocator because you had removed the loop.

I only noticed it because it looks wrong to be using nsMemory with COM. As far as I could tell, it only works because both use malloc anyway.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)

> I only noticed it because it looks wrong to be using nsMemory with COM. As
> far as I could tell, it only works because both use malloc anyway.

right, I will fix it.

Comment 12

[[e-mail]]

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

> NS_IMETHODIMP
> nsAccessible::GetKeyboardShortcut(nsAString& aAccessKey)
> {
>   aAccessKey.Truncate();
> 
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;
> 
>+  AccessKey().ToString(aAccessKey);
>+  return NS_OK;
>+}
Ah yes, that looks much better :-)

>+  inline void ToString(nsAString& aValue,
>+                       Format aFormat = ePlatformFormat) const
>+  {
>+    aValue.Truncate();
>+    AppendToString(aValue);
>+  }
You forgot to forward aFormat. sr=me with that fixed.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch3

addressed Neil, Marco and Trevor (per irc) comments

Comment 14

[Marco Zehe (:MarcoZ)]

My f+ still stands after testing the patch locally.

Comment 15

[Trevor Saunders (:tbsaunde)]

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


>+    nsAccessible* parent = acc->GetParent();
>+    PRUint32 atkRole = atkRoleMap[parent ? parent->NativeRole() : 0];

any reason not to use internal roles, so you don't have to worry about the role map impl, and don't have to fetch it into cache from memory?

>+    if ((atkRole == ATK_ROLE_MENU) || (atkRole == ATK_ROLE_MENU_ITEM)) {
>+      // It is submenu, expose keyboard shortcuts from menu hierarchy like
>+      // "s;<Alt>f:s"
>+      nsAutoString keysInHierarchyStr = keyBindingsStr;
>+      do {
>+        KeyBinding parentKeyBinding = parent->AccessKey();
>+        if (!parentKeyBinding.IsEmpty()) {
>+          keysInHierarchyStr.Insert(':', 0);

couldn't you make this an append to str after Keybinding::ToString() if append is faster than Insert?

> 
>+          nsAutoString str;
>+          parentKeyBinding.ToString(str, KeyBinding::eAtkFormat);
>+          keysInHierarchyStr.Insert(str, 0);
>+        }
>+      } while ((parent = parent->GetParent()) &&
>+               atkRoleMap[parent->NativeRole()] != ATK_ROLE_MENU_BAR);
> 
>+      keyBindingsStr.Append(';');
>+      keyBindingsStr.Append(keysInHierarchyStr);

in the case that the first two parts are empty, but the shortcut has a value we'll return ;<shortcut> not ;;<shortcut> since this ';' is only appended if we have part 1.

>+    }
>+  }
>+  keyBindingsStr.Append(';');
> 
>-                } while ((grandParent = grandParent->GetParent()) &&
>-                         atkRoleMap[grandParent->NativeRole()] != ATK_ROLE_MENU_BAR);
>+  // Get keyboard shortcut.
>+  keyBinding = acc->KeyboardShortcut();
>+  if (!keyBinding.IsEmpty()) {
>+    keyBinding.AppendToString(keyBindingsStr, KeyBinding::eAtkFormat);
>+    keyBindingsStr.Append(';');

I don't believe this trailing ';' is desired.

> NS_IMETHODIMP
> nsAccessible::GetKeyboardShortcut(nsAString& aAccessKey)
> {
>   aAccessKey.Truncate();
> 
>   if (IsDefunct())
>     return NS_ERROR_FAILURE;
> 
>+  AccessKey().ToString(aAccessKey);

It would be great if we could expose both formats to js for testing.

>+  // Get modifier mask. Use ui.key.generalAccessKey (unless it is -1).

I assume you can only actually have one modifier, not say control and alt?

>+  switch (itemType) {
>+  case nsIDocShellTreeItem::typeChrome:
>+    rv = Preferences::GetInt("ui.key.chromeAccess", &modifierMask);
>+    break;
>+

personally, I'd probably treat break like return and not add this blank line.

btw any reason you chose  to use the form of  Preferences::GetType()  that returns a nsresult and uses an out param here?  I'd be tempted to just return Keybinding(key, Preferences::GetInt("pref"); in each case.

>+}

>+  keyStringBundle->GetStringFromName(NS_LITERAL_STRING("MODIFIER_SEPARATOR").get(),
>+                                     getter_Copies(separator));
>+
>+  nsAutoString modifierName;
>+  if (mModifierMask & kControl) {
>+    keyStringBundle->GetStringFromName(NS_LITERAL_STRING("VK_CONTROL").get(),
>+                                       getter_Copies(modifierName));

any reason not to reverse the order you get these strings in so you could only have 1 local string instead of2?  you could also append it after all the ifs right?

>+KeyBinding
>+nsApplicationAccessible::AccessKey() const
>+{
>+  return KeyBinding();
>+}
>+
>+KeyBinding
>+nsApplicationAccessible::KeyboardShortcut() const
>+{
>+  return KeyBinding();
>+}

any particular reason to move these in the file?

> CAccessibleAction::get_keyBinding(long aActionIndex, long aNumMaxBinding,
>                                   BSTR **aKeyBinding,
>                                   long *aNumBinding)
> {
> __try {
>   *aKeyBinding = NULL;
>   *aNumBinding = 0;

null check these out params?

>+  nsRefPtr<nsAccessible> acc(do_QueryObject(this));
>   if (!acc)
>     return E_FAIL;

we should check if we're defunct too right?

>+  // Expose keyboard shortcut if it's not exposed via MSAA keyboard shortcut.
>+  KeyBinding keyBinding = acc->AccessKey();
>+  if (keyBinding.IsEmpty())
>     return S_FALSE;
> 
>+  keyBinding = acc->KeyboardShortcut();

so we only care that AccessKey() returns something not what it returns? since you just threw the value away even if KeyboarShortcut() returns no shortcut?  That does really seem to make sense with the comment.

>+  *aKeyBinding = static_cast<BSTR*>(::CoTaskMemAlloc(sizeof(BSTR*)));
>   if (!*aKeyBinding)
>     return E_OUTOFMEMORY;
> 
>+  *(aKeyBinding[0]) = ::SysAllocStringLen(keyStr.get(), keyStr.Length());

The docs for this method in the ia2 idl in our tree contradict themselves about who should allocate this string, but seems like we  should infact be doing it, so probably just ask Peet to fix the docs.



>       switch (gMenuAccesskeyModifier) {

I think I'd prefer we had a default case that asserted it shouldn't be reached.

>+    accelKey = Preferences::GetInt("ui.key.accelKey", accelKey);

do you need  the local var for something?

>+    switch (accelKey)
>+    {

brace shouldn't be on its own line right?

>+      case nsIDOMKeyEvent::DOM_VK_CONTROL:
>+      default:
>+        modifierMask |= KeyBinding::kControl;

control is the default set somewhere?

>diff --git a/accessible/tests/mochitest/test_keys.html b/accessible/tests/mochitest/actions/test_keys.html
>rename from accessible/tests/mochitest/test_keys.html
>rename to accessible/tests/mochitest/actions/test_keys.html
>--- a/accessible/tests/mochitest/test_keys.html
>+++ b/accessible/tests/mochitest/actions/test_keys.html
>@@ -7,17 +7,17 @@
>         href="chrome://mochikit/content/tests/SimpleTest/test.css" />


we should probably test links as well as inputs in  html.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> Comment on attachment 546489 [details] [diff] [review] [review]
> patch3
> 
> 
> >+    nsAccessible* parent = acc->GetParent();
> >+    PRUint32 atkRole = atkRoleMap[parent ? parent->NativeRole() : 0];
> 
> any reason not to use internal roles, so you don't have to worry about the
> role map impl, and don't have to fetch it into cache from memory?

no reason I think, just preserved existing code. I've changed to Role() making ARIA menus working the same way, and added checkbox and radio menuitems. Thanks for the catch, two more bugs are fixed.

> >+          keysInHierarchyStr.Insert(':', 0);
> 
> couldn't you make this an append to str after Keybinding::ToString() if
> append is faster than Insert?

done

> >+      } while ((parent = parent->GetParent()) &&
> >+               atkRoleMap[parent->NativeRole()] != ATK_ROLE_MENU_BAR);
> > 
> >+      keyBindingsStr.Append(';');
> >+      keyBindingsStr.Append(keysInHierarchyStr);
> 
> in the case that the first two parts are empty, but the shortcut has a value
> we'll return ;<shortcut> not ;;<shortcut> since this ';' is only appended if
> we have part 1.

does it really make sense to return ;;<shortcut>? Is this a way to distinguish accesskey from shortcuts?

> >+  keyBinding = acc->KeyboardShortcut();
> >+  if (!keyBinding.IsEmpty()) {
> >+    keyBinding.AppendToString(keyBindingsStr, KeyBinding::eAtkFormat);
> >+    keyBindingsStr.Append(';');
> 
> I don't believe this trailing ';' is desired.

Ok, maybe it makes sense to not change existing template.

> > NS_IMETHODIMP
> > nsAccessible::GetKeyboardShortcut(nsAString& aAccessKey)
> > {
> >   aAccessKey.Truncate();
> > 
> >   if (IsDefunct())
> >     return NS_ERROR_FAILURE;
> > 
> >+  AccessKey().ToString(aAccessKey);
> 
> It would be great if we could expose both formats to js for testing.
> 
> >+  // Get modifier mask. Use ui.key.generalAccessKey (unless it is -1).
> 
> I assume you can only actually have one modifier, not say control and alt?

correct, it takes key code (see nsIDOMKeyEvent)

> >+  switch (itemType) {
> >+  case nsIDocShellTreeItem::typeChrome:
> >+    rv = Preferences::GetInt("ui.key.chromeAccess", &modifierMask);
> >+    break;

> btw any reason you chose  to use the form of  Preferences::GetType()  that
> returns a nsresult and uses an out param here?  I'd be tempted to just
> return Keybinding(key, Preferences::GetInt("pref"); in each case.

I don't want to return valid KeyBinding in case of failure.

> >+  keyStringBundle->GetStringFromName(NS_LITERAL_STRING("MODIFIER_SEPARATOR").get(),
> >+                                     getter_Copies(separator));
> >+
> >+  nsAutoString modifierName;
> >+  if (mModifierMask & kControl) {
> >+    keyStringBundle->GetStringFromName(NS_LITERAL_STRING("VK_CONTROL").get(),
> >+                                       getter_Copies(modifierName));
> 
> any reason not to reverse the order you get these strings in so you could
> only have 1 local string instead of2?  you could also append it after all
> the ifs right?

Not sure I follow your idea, can you give details pelase?

> >+KeyBinding
> >+nsApplicationAccessible::AccessKey() const
> >+{
> >+  return KeyBinding();
> >+}
> >+
> >+KeyBinding
> >+nsApplicationAccessible::KeyboardShortcut() const
> >+{
> >+  return KeyBinding();
> >+}
> 
> any particular reason to move these in the file?

it can share same implementation

> >+  nsRefPtr<nsAccessible> acc(do_QueryObject(this));
> >   if (!acc)
> >     return E_FAIL;
> 
> we should check if we're defunct too right?
> 
> >+  // Expose keyboard shortcut if it's not exposed via MSAA keyboard shortcut.
> >+  KeyBinding keyBinding = acc->AccessKey();
> >+  if (keyBinding.IsEmpty())
> >     return S_FALSE;
> > 
> >+  keyBinding = acc->KeyboardShortcut();
> 
> so we only care that AccessKey() returns something not what it returns?
> since you just threw the value away even if KeyboarShortcut() returns no
> shortcut?  That does really seem to make sense with the comment.

right

> >+  *aKeyBinding = static_cast<BSTR*>(::CoTaskMemAlloc(sizeof(BSTR*)));
> >   if (!*aKeyBinding)
> >     return E_OUTOFMEMORY;
> > 
> >+  *(aKeyBinding[0]) = ::SysAllocStringLen(keyStr.get(), keyStr.Length());
> 
> The docs for this method in the ia2 idl in our tree contradict themselves
> about who should allocate this string, but seems like we  should infact be
> doing it, so probably just ask Peet to fix the docs.

ok

> 
> >       switch (gMenuAccesskeyModifier) {
> 
> I think I'd prefer we had a default case that asserted it shouldn't be
> reached.

we shouldn't assert since it's user preference, maybe warning into console make sense.

> >+    accelKey = Preferences::GetInt("ui.key.accelKey", accelKey);
> 
> do you need  the local var for something?

> 
> >+    switch (accelKey)
> >+    {
> 
> >+      case nsIDOMKeyEvent::DOM_VK_CONTROL:
> >+      default:
> >+        modifierMask |= KeyBinding::kControl;
> 
> control is the default set somewhere?

hope made this better

> we should probably test links as well as inputs in  html.

sure, if you like

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> Created attachment 546712 [details] [diff] [review] [review]
> patch4

> switch (Preferences::GetInt("ui.key.accelKey")) {

fixed to

switch (Preferences::GetInt("ui.key.accelKey", 0)) {

Comment 19

[Trevor Saunders (:tbsaunde)]

> > >+      keyBindingsStr.Append(';');
> > >+      keyBindingsStr.Append(keysInHierarchyStr);
> > 
> > in the case that the first two parts are empty, but the shortcut has a value
> > we'll return ;<shortcut> not ;;<shortcut> since this ';' is only appended if
> > we have part 1.
> 
> does it really make sense to return ;;<shortcut>? Is this a way to
> distinguish accesskey from shortcuts?

I think so see below, mostly to make parsing easier.

> > >+  keyBinding = acc->KeyboardShortcut();
> > >+  if (!keyBinding.IsEmpty()) {
> > >+    keyBinding.AppendToString(keyBindingsStr, KeyBinding::eAtkFormat);
> > >+    keyBindingsStr.Append(';');
> > 
> > I don't believe this trailing ';' is desired.
> 
> Ok, maybe it makes sense to not change existing template.

the docs say they want a;b;c I think we always want to have the 2 ';'s even if some set of a b and c are '' because if I were writing a parser for that format I would split on ';' so I think it makes the at's job the easiest to always have the parts even if there empty.

> Not sure I follow your idea, can you give details pelase?

it was a bad idea / wouldn't actually work  forget about it.

> > 
> > >       switch (gMenuAccesskeyModifier) {
> > 
> > I think I'd prefer we had a default case that asserted it shouldn't be
> > reached.
> 
> we shouldn't assert since it's user preference, maybe warning into console
> make sense.

sure

> > we should probably test links as well as inputs in  html.
> 
> sure, if you like

please

Comment 20

[Trevor Saunders (:tbsaunde)]

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


>+  if (!keyBinding.IsEmpty()) {
>+    keyBindingsStr.Append(';');

I think we always want the second ';' not just if there is a keyboard shortcut, but I don't think this is as big a deal.  Joanie opinions?

>+    keyBinding.AppendToString(keyBindingsStr, KeyBinding::eAtkFormat);
>+  }

>+  PRUint32 mKey;
>+  PRUint32 mModifierMask;
>+};
>+
>+

did you mean for there to be two blank lines here?

> #endif

>   *aNumBinding = 0;
>+  if (!aNumBinding)
>+    return E_INVALIDARG;

you want to check then assign right? checking after is pretty silly :)

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> Comment on attachment 546712 [details] [diff] [review] [review]
> patch4
> 
> 
> >+  if (!keyBinding.IsEmpty()) {
> >+    keyBindingsStr.Append(';');
> 
> I think we always want the second ';' not just if there is a keyboard
> shortcut, but I don't think this is as big a deal.  Joanie opinions?

ok, moved Append(;) from if.

> >   *aNumBinding = 0;
> >+  if (!aNumBinding)
> >+    return E_INVALIDARG;
> 
> you want to check then assign right? checking after is pretty silly :)

tired, fixed, thank you

do you need new patch for this?

Comment 22

[Trevor Saunders (:tbsaunde)]

> tired, fixed, thank you

heh, no worries :-)

> do you need new patch for this?

NO, i THINK i'M FINE WITH WHAT i HAVE THANKS

Comment 23

[Trevor Saunders (:tbsaunde)]

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

ok, I see <alt><shift>a and <alt><shift>b with the test case / script.   this looks like the right value, so r=me

Comment 24

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/37617ddae628

Comment 25

[alexander :surkov (:asurkov)]

Attachment: crash AccessKey() on application accessible

Comment 26

[alexander :surkov (:asurkov)]

patch: http://hg.mozilla.org/mozilla-central/rev/37617ddae628
follow up crash: http://hg.mozilla.org/mozilla-central/rev/aa2de73abc19

Comment 27

[alexander :surkov (:asurkov)]

Attachment: menu KeyboardShortcut() fix

Thanks to Ms2ger for pointing this out.

Trevor, they look mostly the same for me in the editor. But how did you miss it? :)

Comment 28

[Trevor Saunders (:tbsaunde)]

Comment on attachment 547421 [details] [diff] [review]
menu KeyboardShortcut() fix

uh, not really sure how I missed that. any way  obviously correct.

Comment 29

[alexander :surkov (:asurkov)]

nsXULMenuAccessible::KeyboardShortcut fix is landed http://hg.mozilla.org/integration/mozilla-inbound/rev/b2c429bb9f3f

Comment 30

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b2c429bb9f3f