Last Comment Bug 672092 - Access keys not exposed as part of AtkAction
: Access keys not exposed as part of AtkAction
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: alexander :surkov
:
Mentors:
: 440568 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-16 20:06 PDT by Joanmarie Diggs
Modified: 2013-01-02 19:24 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (376 bytes, text/html)
2011-07-16 20:06 PDT, Joanmarie Diggs
no flags Details
test script (809 bytes, text/x-python)
2011-07-16 20:07 PDT, Joanmarie Diggs
no flags Details
patch (42.08 KB, patch)
2011-07-17 09:12 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (42.08 KB, patch)
2011-07-17 10:32 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (42.07 KB, patch)
2011-07-17 10:33 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (42.10 KB, patch)
2011-07-17 23:03 PDT, alexander :surkov
neil: superreview+
mzehe: feedback+
Details | Diff | Splinter Review
patch3 (42.14 KB, patch)
2011-07-18 04:01 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch4 (41.71 KB, patch)
2011-07-18 22:57 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
crash AccessKey() on application accessible (1.63 KB, patch)
2011-07-19 06:05 PDT, alexander :surkov
fherrera: review+
Details | Diff | Splinter Review
menu KeyboardShortcut() fix (912 bytes, patch)
2011-07-21 09:50 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Joanmarie Diggs 2011-07-16 20:06:01 PDT
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).
Comment 1 Joanmarie Diggs 2011-07-16 20:07:03 PDT
Created attachment 546373 [details]
test script
Comment 2 alexander :surkov 2011-07-17 09:12:55 PDT
Created attachment 546426 [details] [diff] [review]
patch
Comment 3 alexander :surkov 2011-07-17 10:32:25 PDT
Created attachment 546429 [details] [diff] [review]
patch2

Trevor, could you check if linux part work as expected please?
Comment 4 alexander :surkov 2011-07-17 10:33:54 PDT
Created attachment 546430 [details] [diff] [review]
patch
Comment 5 neil@parkwaycc.co.uk 2011-07-17 13:24:19 PDT
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 2011-07-17 22:58:31 PDT
(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 2011-07-17 23:03:33 PDT
Created attachment 546466 [details] [diff] [review]
patch2
Comment 8 Marco Zehe (:MarcoZ) on PTO until August 15 2011-07-18 01:02:03 PDT
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 2011-07-18 01:33:10 PDT
(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 neil@parkwaycc.co.uk 2011-07-18 02:39:00 PDT
(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 2011-07-18 02:44:45 PDT
(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 neil@parkwaycc.co.uk 2011-07-18 02:45:24 PDT
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 2011-07-18 04:01:07 PDT
Created attachment 546489 [details] [diff] [review]
patch3

addressed Neil, Marco and Trevor (per irc) comments
Comment 14 Marco Zehe (:MarcoZ) on PTO until August 15 2011-07-18 04:40:12 PDT
My f+ still stands after testing the patch locally.
Comment 15 Trevor Saunders (:tbsaunde) 2011-07-18 21:22:27 PDT
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 2011-07-18 22:49:10 PDT
(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 2011-07-18 22:57:03 PDT
Created attachment 546712 [details] [diff] [review]
patch4
Comment 18 alexander :surkov 2011-07-18 22:59:11 PDT
(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) 2011-07-19 00:11:57 PDT
> > >+      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) 2011-07-19 00:49:03 PDT
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 2011-07-19 00:53:55 PDT
(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) 2011-07-19 01:09:22 PDT
> 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) 2011-07-19 01:23:37 PDT
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 25 alexander :surkov 2011-07-19 06:05:05 PDT
Created attachment 546765 [details] [diff] [review]
crash AccessKey() on application accessible
Comment 27 alexander :surkov 2011-07-21 09:50:37 PDT
Created attachment 547421 [details] [diff] [review]
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) 2011-07-21 10:06:55 PDT
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 2011-07-21 17:55:09 PDT
nsXULMenuAccessible::KeyboardShortcut fix is landed http://hg.mozilla.org/integration/mozilla-inbound/rev/b2c429bb9f3f
Comment 31 alexander :surkov 2013-01-02 19:24:21 PST
*** Bug 440568 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.