Make our MSAA objects also support IDispatch methods for scripters

RESOLVED FIXED in mozilla1.3alpha

Status

()

Core
Disability Access APIs
P5
normal
RESOLVED FIXED
15 years ago
7 years ago

People

(Reporter: Aaron Leventhal, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Trunk
mozilla1.3alpha
x86
Windows 2000
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

15 years ago
We have a place to support IDispatch interfaces on our Accessible objects here:

http://bugzilla.mozilla.org/show_bug.cgi?id=166993

According to Adam:

> Basically you just let the OLE system to implement the IDispatch methods 
> for you using the type library to marshal calls to your dual interface 
> methods. I have a VB sample which demonstrates the object being called 
> via IDispatch and directly on the dual interface to demonstrate it works.
(Reporter)

Updated

15 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.3alpha
(Reporter)

Comment 1

15 years ago
Oops, pasted the wrong link.

Here's the correct LXR link:

http://lxr.mozilla.org/seamonkey/source/widget/src/windows/Accessible.cpp#857

Comment 2

15 years ago
Created attachment 98121 [details]
Zipped up simple sample

Contained is a simple COM object demonstrating the principle. I'll attach the
most pertinent source file seperately next to save people the pain of unzipping
to see what's involved.

Comment 3

15 years ago
Created attachment 98123 [details]
TlbDemoObject.cpp

This is a simple COM object with a few dummy methods. It exposes a
dual-interface (i.e. derived from IDispatch) just like IAccesible. The
IDispatch implementation is the most important thing to note - Replace the impl
in mozilla/widget/src/windows/Accessible.cpp with this changing the Typelib
GUID and major/minor version numbers, ensuring that QI works for IDispatch and
IAccesible and that's about all there is to it.

The file win32 file oleacc.h defines the typelib GUID as LIBID_Accessibility,
and probably 1,0 or 1,1 will do for the major, minor versions. I have 1.1
installed on my machine, but I don't know what the baseline version is.
(Reporter)

Comment 4

15 years ago
Created attachment 98250 [details] [diff] [review]
Okay, here's a patch -- how do I test this?

Was I suppoed to use IID_IAccessible here? 
  hr = typeLib->GetTypeInfoOfGuid(IID_IAccessible, ppTInfo);

Anyway, this builds okay, but how do we test it?

Comment 5

15 years ago
Yes, IID_IAccessible.

Doesn't the Accessibility SDK have tools to test it's working? They must make
calls via IDispatch of course.

If the tools go via IAccesible, you'll have to throw something together in VB to
test it. The trickiest thing will be getting the IDispatch from the window,
which presumably means calling AccessibleObjectFromPoint or something similar.

Comment 6

15 years ago
The MSAA SDK 1.3 has a sample called VBInsp which uses the IDispatch impl. In
its current state it should make a good sanity test (hover over the Moz/NS
window and see if it can get back some info) and you could probably add some
extra tests to it too.
(Reporter)

Comment 7

15 years ago
I haven't tried VBInsp yet, but checking "Use Invoke" in Inspect.exe seems to
test the stuff.

Unfortunately it goes into an infinite recursive loop when Invoke() gets called.

Invoke() calls GetTypeInfo(), and then Invoke() again, which ends up being
Invoke() on the same object.

I better read up on MSDN. I still don't understand what these IDispatch methods
are supposed to do.

Comment 8

15 years ago
Ideally you implement IDispatch::Invoke, put any specialized code in and then
forward to ITypeInfo::Invoke which then does the actual invoke. Are you trying
to invoke "Invoke"? I would imagine that would cause problems.

Comment 9

15 years ago
Created attachment 98487 [details] [diff] [review]
Updated patch

This is a slightly updated patch which caches the type info instead of
refetching it everytime. Otherwise it's the same.

Comment 10

15 years ago
The patch works for me. The invoke method gets called from the inspector exe and
then the appropriate dual interface implementation. The patch lets the system do
the marshaling.

I don't understand why the ITypeInfo::Invoke would call the IDispatch::Invoke()
again. It doesn't for me. The actual implementation does little more than grab
the ITypeInfo object representation of the interface and call Invoke on it,
passing the accessible instance in as the object whose methods are to be called.
(Reporter)

Comment 11

15 years ago
When I build with the new patch, I get these errors:
C:\run\vstudio\VC98\atl\include\atlcom.h(2945) : error C2065: '_Module' :
undeclared identifier
C:\run\vstudio\VC98\atl\include\atlcom.h(2945) : error C2228: left of '.Lock'
must have class/struct/union type
C:\run\vstudio\VC98\atl\include\atlcom.h(2947) : error C2228: left of '.Unlock'
must have class/struct/union type
C:\run\vstudio\VC98\atl\include\atlcom.h(3318) : error C2228: left of
'.m_csTypeInfoHolder' must have class/struct/union type
C:\run\vstudio\VC98\atl\include\atlcom.h(3340) : error C2228: left of
'.m_csTypeInfoHolder' must have class/struct/union type
C:\run\vstudio\VC98\atl\include\atlcom.h(3341) : error C2228: left of
'.AddTermFunc' must have class/struct/union type
(Reporter)

Comment 12

15 years ago
I can get it to compile of I move

#define _ATLBASE_IMPL
#include <atlbase.h>
extern CComModule _Module;
#define _ATLCOM_IMPL
#include <atlcom.h>

into Accessible.h (replace the old <atlbase.h>, <atlcom.h> includes in the .h
with those lines)
(Reporter)

Comment 13

15 years ago
This is really weird. With the new patch this line in GetTypeInfo()

  EnterCriticalSection(&_Module.m_csTypeInfoHolder);

seems to call GetTypeInfo() again.
(Reporter)

Comment 14

15 years ago
Maybe some kind of exception is getting thrown on my system.
The page I'm using to test is www.google.com.

Comment 15

15 years ago
Can you try clobbering the whole widget dir and rebuilding? It could be bustage
caused by broken dependencies.
(Reporter)

Comment 16

15 years ago
I tried clobbering and rebuilding with a completely fresh tree, and it made no
difference.

And I verified that it's not coming back to me from EnterCriticalSection() by
using printfs. It doesn't reach the printf right after that. Next time my code
is entered is via Invoke() with a different dispid.
(Reporter)

Comment 17

15 years ago
Created attachment 98671 [details] [diff] [review]
Fixes EnterCriticalSection problem, by using InitializeCriticalSection()

There is still the problem of typeInfo->Invoke() not returning. It's behaving
similarly, as if it had it's own uninitialized critical section.
Attachment #98250 - Attachment is obsolete: true
Attachment #98487 - Attachment is obsolete: true
(Reporter)

Comment 18

15 years ago
  EnterCriticalSection(&_Module.m_csTypeInfoHolder);
  if (!gmTypeInfo)
  {
    CComPtr<ITypeLib> typeLib;
    hr = LoadRegTypeLib(LIBID_Accessibility, wMajor, wMinor, lcid, &typeLib);
    if (SUCCEEDED(hr)) 
      hr = typeLib->GetTypeInfoOfGuid(IID_IAccessible, &gmTypeInfo);
  }
  LeaveCriticalSection(&_Module.m_csTypeInfoHolder);


Wouldn't it be better to put the EnterCriticalSection and LeaveCriticalSection
inside the if?
(Reporter)

Comment 19

15 years ago
Do I need to initialize CComModule _Module ?
_Module->Init( _ATL_OBJMAP_ENTRY* p, HINSTANCE h );

I don't have a clue what's wrong with it.

Comment 20

15 years ago
If you're not linking against ATL, which has it's own instance, then you'll need
to declare it and initialize it. Either in DLLMain or main. If you're linking
against ATL then you should be able to just declare it extern, and ATL will do
the rest.

And you should leave the Enter/Leave calls outside of the if. The if depends on
the setting of gmTypeInfo in the call to GetTypeInfoOfGuid. So the if needs to
be within the critical section as well.

Comment 21

15 years ago
I thought that the m_csTypeInfoHolder was initialised in the CComModule
constructor, but looks like it is done in CComModule::Init. 

Unless you want to hack your code to be properly conformant with ATL I suggest
declaring a CComAutoCriticalSection static and just using that instead.

Making it use ATL properly wouldn't be a bad thing. ATL has some useful macros
for taking the drudgery out of implementing COM objects. It also has an
IDispatchImpl template which means you wouldn't need to implement it for
youself. It is possible to statically link to ATL too, though it is 95% inline
anyway.
(Reporter)

Updated

13 years ago
Keywords: access, helpwanted
(Reporter)

Updated

12 years ago
Priority: P3 → P5
(Reporter)

Comment 22

11 years ago
Apparently this is needed for JAWS scripting.
(Reporter)

Updated

11 years ago
Blocks: 342901
(Reporter)

Updated

10 years ago
Blocks: 368873
No longer blocks: 342901
(Reporter)

Updated

10 years ago
Assignee: aaronleventhal → surkov.alexander
Status: ASSIGNED → NEW
(Reporter)

Comment 23

10 years ago
I believe we'll want IDispatch support for our IA2 support as well, so it works with JAWS scripting. Check with #ia2 or IAccessible2 mailing list.
(Assignee)

Comment 24

10 years ago
Neither JAWS or Window-Eyes need IDispatch for IA2. Do we need IDispatch for IAccessible. If we need then it may be a problem to make MSAA object implements both IA and IA2 interfaces.
(Reporter)

Comment 25

10 years ago
> If we need then it may be a problem to make MSAA object implements
> both IA and IA2 interfaces.

What kind of problem?
(Assignee)

Comment 26

10 years ago
Created attachment 258137 [details] [diff] [review]
patch

I tried to call accName property, GetIDsOfNames() for it returns S_OK but Invoke() doesn't work. Debugger doesn't show me HRESULT for DispInvoke() due to unknown reasons for me.
(Assignee)

Comment 27

10 years ago
(In reply to comment #25)
> > If we need then it may be a problem to make MSAA object implements
> > both IA and IA2 interfaces.
> 
> What kind of problem?
> 

Ah, ignore this. I was wrong.
(Reporter)

Comment 28

10 years ago
Are you looking for review of last patch? (I would not be the right person -- need someone who knows COM better).
(Assignee)

Comment 29

10 years ago
(In reply to comment #28)
> Are you looking for review of last patch? (I would not be the right person --
> need someone who knows COM better).
> 

Actually my patch is much like previous one excepting I don't use ATL and use helper methods like DispInvoke(). Can we find MS COM guru to help us?
(Reporter)

Comment 30

10 years ago
Maybe bsmedberg?
(Assignee)

Comment 31

10 years ago
I tried with my patch to call get_accChildCount from JScript. Invoke() didn't fail. But nsAccessibleWrap::get_accChildCount() wasn't called. Therefore I wonder when I load ITypeInfo and when I pass nsAccessibleWrap to ITypeInfo::Invoke() then how will Invoke know what method to call? Can anybody clarify how Invoke() works?

On the other hand we can easy I guess to implement IDispatch support without typeinfo. Here we can just to implement manually GetIDsOfNames() and Invoke() like it is shown on http://lists.kde.org/?l=kde-accessibility&m=103849116623673&w=2.
(Assignee)

Comment 32

10 years ago
(In reply to comment #31)
> I tried with my patch to call get_accChildCount from JScript. Invoke() didn't
> fail. But nsAccessibleWrap::get_accChildCount() wasn't called. Therefore I
> wonder when I load ITypeInfo and when I pass nsAccessibleWrap to
> ITypeInfo::Invoke() then how will Invoke know what method to call? Can anybody
> clarify how Invoke() works?

I mean it looks something was called and invoked get_accChildCount returned untitialized value (and debugger showed that it wasn't our nsAccessibleWrap::get_accChildCount). Really I can't understand magic of loaded ITypeInfo::Invoke (ITypeInfo I get via boundle of calls LoadRegTypeLib/GetTypeInfoOfGuid).

Benjamin, can you explain this?
(Reporter)

Comment 33

10 years ago
It would still be helpful to have this for MSAA even though we're not planning to do it for IA2
(Reporter)

Updated

10 years ago
Blocks: 396347
(Reporter)

Comment 34

9 years ago
Surkov, how hard is this to fix? We're getting fairly regular criticism for not support IDispatch on our COM objects.
(Assignee)

Comment 35

9 years ago
(In reply to comment #34)
> Surkov, how hard is this to fix? We're getting fairly regular criticism for not
> support IDispatch on our COM objects.
> 

The problem is the latest patch doesn't work (I tried it with accExplorer) but it looks correctly with me. I really haven't idea what's wrong.
QA Contact: dsirnapalli → accessibility-apis

Comment 36

8 years ago
Is this bug still "helpwanted"? It seems current version of oleacc.dll have learned a few new tricks and now handle the dispinterface part of IAccessible themselves.

Anyway, I believe (I didn't actually test this theory) the first patch was already almost there and just missing one essential detail. In this line

hr = typeInfo->Invoke(this, dispIdMember, wFlags, pDispParams, pVarResult, pExcepInfo, puArgErr);

'this' is an Accessible * , but ITypeInfo::Invoke requires an IAccessible *. IOW, you were passing the wrong vtable and the symptoms of that are exactly what you've described in comments 7 and 32: seemingly random method calls all over the place. Replace |Invoke(this, | with |Invoke(static_cast<IAccessible *>(this), | and you should be golden.
(Assignee)

Comment 37

8 years ago
(In reply to comment #36)
> Is this bug still "helpwanted"?

I think so.

> It seems current version of oleacc.dll have
> learned a few new tricks and now handle the dispinterface part of IAccessible
> themselves.

Really? That's great. So then we don't need to do anything to expose IAccessible. But will it harm us if we do?

We could expose IAccessible2 interfaces as well I guess.

> 
> Anyway, I believe (I didn't actually test this theory) the first patch was
> already almost there and just missing one essential detail. In this line
> 
> hr = typeInfo->Invoke(this, dispIdMember, wFlags, pDispParams, pVarResult,
> pExcepInfo, puArgErr);
> 
> 'this' is an Accessible * , but ITypeInfo::Invoke requires an IAccessible *.
> IOW, you were passing the wrong vtable and the symptoms of that are exactly
> what you've described in comments 7 and 32: seemingly random method calls all
> over the place. Replace |Invoke(this, | with |Invoke(static_cast<IAccessible
> *>(this), | and you should be golden.

Could you look at the last patch please? Does it have the same problem in HRESULT hr = DispInvoke(this, ?

Comment 38

8 years ago
(In reply to comment #37)
> Really? That's great. So then we don't need to do anything to expose
> IAccessible. But will it harm us if we do?

It won't harm you, your code simply won't be called with these versions of oleacc. But Windows 2000 SP4 still has an older version, here Inspector shows E_NOTIMPL for tons of stuff when you try the "Use Invoke" option with Firefox. Users of that OS would still benefit if you'd implement IDispatch yourself.

> Could you look at the last patch please? Does it have the same problem in
> HRESULT hr = DispInvoke(this, ?

Yes, since nsAccessibleWrap also uses multiple inheritance. DispInvoke is really a quite useless API: DispInvoke(p, typeInfoPtr, ...) is exactly the same as typeInfoPtr->Invoke(p, ...) .
(Assignee)

Comment 39

7 years ago
Created attachment 420083 [details] [diff] [review]
patch2
Attachment #258137 - Attachment is obsolete: true
Attachment #420083 - Flags: review?(bolterbugz)
How do we test this?
(Assignee)

Comment 41

7 years ago
(In reply to comment #40)
> How do we test this?

Only if you have Windows 2000 (see comment #36, comment #38). We could ask Martin to look again at the patch if he can see any other issue.
Comment on attachment 420083 [details] [diff] [review]
patch2

r=me with comments:

(In reply to comment #41)
> We could ask
> Martin to look again at the patch if he can see any other issue.

OK so this is a tweaked version of Aaron's patch? I'm very very rusty on this part of COM... so yes let's get his review.

It needs to be tested before landing.

Updated

7 years ago
Attachment #420083 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 43

7 years ago
(In reply to comment #42)
> (From update of attachment 420083 [details] [diff] [review])
> r=me with comments:
> 
> (In reply to comment #41)
> > We could ask
> > Martin to look again at the patch if he can see any other issue.
> 
> OK so this is a tweaked version of Aaron's patch? I'm very very rusty on this
> part of COM... so yes let's get his review.

I think so. I don't remember though already. Do you mean review from Aaron? I'm not sure it's good idea to summon him for these stuffs.

> It needs to be tested before landing.

Ideally yes. If someone of us can get windows 2000 then it would be easy, otherwise I don't have any idea. Also I think it's not very big area of screen reader users who use windows 2000 and write scripts. Actually I think we can trust Martin comment so we could think the fix is right and don't spare time for testing.
Can you do a build and ask Martin or Adam to test it?
(Assignee)

Comment 45

7 years ago
(In reply to comment #44)
> Can you do a build and ask Martin or Adam to test it?

Sure, try server build is here https://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-07e98c881a2a/

Comment 46

7 years ago
(In reply to comment #45)
> Sure, try server build is here
> https://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-07e98c881a2a/

The patch looks ok to me and Inspect32 under Windows 2000 likes this build very much (same information with or without "Use Invoke" checked). Loading the type library once for every Invoke call is less than ideal performance-wise, but probably not worth fixing unless somebody complains about it.
(In reply to comment #46)
> (In reply to comment #45)
> Loading the type
> library once for every Invoke call

Does it? I thought this guarded against reload:

+  if (mTypeInfo)
+    return mTypeInfo;

Comment 48

7 years ago
> Does it? I thought this guarded against reload:
> 
> +  if (mTypeInfo)
> +    return mTypeInfo;

Yes, you're absolutely right, don't know how I missed those lines.
(Assignee)

Comment 49

7 years ago
(In reply to comment #46)
> (In reply to comment #45)
> > Sure, try server build is here
> > https://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-07e98c881a2a/
> 
> The patch looks ok to me and Inspect32 under Windows 2000 likes this build very
> much (same information with or without "Use Invoke" checked). 

Great. Thank you much!

Removing "helpwanted" keyword.
Keywords: helpwanted
(Assignee)

Comment 50

7 years ago
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/1f9ae6f1f9c5
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 51

7 years ago
Martin, btw, could we turn mTypeInfo into static variable, right (and GetTI while we are here) (just noticed this after landing)?
(Assignee)

Comment 52

7 years ago
(In reply to comment #51)
> Martin, btw, could we turn mTypeInfo into static variable, right (and GetTI
> while we are here) (just noticed this after landing)?

ah, then we should deal with critical sections when type lib is created and used?

Comment 53

7 years ago
(In reply to comment #51)
> Martin, btw, could we turn mTypeInfo into static variable, right (and GetTI
> while we are here) (just noticed this after landing)?

You probably could, yes. I know nothing about Gecko's threading architecture, it has only one thread pumping messages for all windows and thus this thread handles all WM_GETOBJECT messages and creates all accessible objects? If so, you won't need to use a critical section, AFAICS MSAA doesn't require your objects to be free threaded.

If you revisit this patch you could also remove the ResultFromScode macro, it's obsolete and doesn't do anything.
(Assignee)

Comment 54

7 years ago
cc'ing Benjamin again to help in answer on comment #53.
Yes, the main thread pumps all messages that I know of.
(Assignee)

Updated

7 years ago
Blocks: 539699
(Assignee)

Comment 56

7 years ago
Ok, thanks. I filed bug 539699 to deal with this.
You need to log in before you can comment on or make changes to this bug.