370790 - implement IAccessibleAction

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Add support of IA2 interfaces for MSAA wrap objects, just to append their properties and methods and don't implement them.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

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

ginn, please review especially atk part

benjamin, please look at windows part

Comment 3

[Aaron Leventhal]

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

r+ on nsAccessibleWrap part only, except for this problem:
+nsAccessibleWrap::nActions(long *aNumActions)
Always returns E_FAIL, but should succeed sometimes.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (From update of attachment 258201 [details] [diff] [review])
> +nsAccessibleWrap::nActions(long *aNumActions)
> Always returns E_FAIL, but should succeed sometimes.
> 

Ah, right. Thank you for the catch. I'd like to fix it after other reviews if reviews are fine with this.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

Fixed Aaron's comment, updated to trunk

Comment 6

[alexander :surkov (:asurkov)]

checked in on trunk to continue the work, but I still like to get benjamin's notice that all is fine if it is fine for sure :). So, leaving bug open.

Comment 7

[Benjamin Smedberg]

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

I know almost nothing about this code, so I haven't reviewed for correctness. It does look right from an XPCOM point of view.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (From update of attachment 258935 [details] [diff] [review])
> I know almost nothing about this code, so I haven't reviewed for correctness.
> It does look right from an XPCOM point of view.
> 

It's fine. Actually I looked for correctness checking. Thank you.

Comment 9

[Aaron Leventhal]

Interestingly, this piece of code caused to super annoying startup crash bug 376239. I have no idea why this snippet makes it crash. Maybe Window-Eyes and JAWS are using one of these interfaces, and their definition doesn't match ours. I don't know yet.

It took me and Wayne a long time to track this down. I had to back out one patch at a time for the last 2 weeks. 

@@ -105,20 +109,22 @@ STDMETHODIMP nsAccessibleWrap::QueryInte
 
   if (IID_IUnknown == iid || IID_IDispatch == iid || IID_IAccessible == iid)
     *ppv = NS_STATIC_CAST(IAccessible*, this);
   else if (IID_IEnumVARIANT == iid && !gIsEnumVariantSupportDisabled) {
     long numChildren;
     get_accChildCount(&numChildren);
     if (numChildren > 0)  // Don't support this interface for leaf elements
       *ppv = NS_STATIC_CAST(IEnumVARIANT*, this);
-  }
-  else if (IID_IServiceProvider == iid) {
+  } else if (IID_IServiceProvider == iid)
     *ppv = NS_STATIC_CAST(IServiceProvider*, this);
-  }
+  else if (IID_IAccessible2 == iid)
+    *ppv = NS_STATIC_CAST(IAccessible2*, this);
+  else if (IID_IAccessibleAction == iid)
+    *ppv = NS_STATIC_CAST(IAccessibleAction*, this);
 
   if (NULL == *ppv)
     return nsAccessNodeWrap::QueryInterface(iid, ppv);
 
   (NS_REINTERPRET_CAST(IUnknown*, *ppv))->AddRef();
   return S_OK;
 }
 

Comment 10

[Aaron Leventhal]

In fact it doesn't crash if we don't allow the QI to IAccessible2*.
Commenting these lines fixes the crash:
  //else if (IID_IAccessible2 == iid)
    //*ppv = NS_STATIC_CAST(IAccessible2*, this);

Comment 11

[Aaron Leventhal]

The version of Window-Eyes 6.1 Beta 1A I have is in fact trying to use IAccessible2. I suspect an API mismatch.
If the version of Window-Eyes is too old the crash probably doesn't happen.

Also, if we upgrade to the very latest Window-Eyes and IAccessible2 IDL I bet the crash goes away. 

Comment 12

[Aaron Leventhal]

I decided to close this again.
I backed out the offending IAccessible2 QI and and opened a new bug to fix just that. It's bug 376753.