Created attachment 546372[details]
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).

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

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

(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

> > >+ 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 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 :)

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