[PATCH v3] oleacc: Add support for retrieving an HWND from accNavigate to WindowFromAccessibleObject.
Signed-off-by: Connor McAdams cmcadams@codeweavers.com --- dlls/oleacc/main.c | 51 ++++++++++++++--- dlls/oleacc/tests/main.c | 121 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 162 insertions(+), 10 deletions(-)
diff --git a/dlls/oleacc/main.c b/dlls/oleacc/main.c index dced84be7b5..1128b6c9b44 100644 --- a/dlls/oleacc/main.c +++ b/dlls/oleacc/main.c @@ -398,6 +398,22 @@ HRESULT WINAPI AccessibleObjectFromWindow( HWND hwnd, DWORD dwObjectID, return CreateStdAccessibleObject(hwnd, dwObjectID, riid, ppvObject); }
+#define NAVDIR_INTERNAL_HWND 10 +static HWND get_hwnd_from_acc_nav(IAccessible *acc) +{ + HWND hwnd = NULL; + VARIANT v, cid; + HRESULT hr; + + VariantInit(&v); + variant_init_i4(&cid, CHILDID_SELF); + hr = IAccessible_accNavigate(acc, NAVDIR_INTERNAL_HWND, cid, &v); + if(SUCCEEDED(hr) && V_VT(&v) == VT_I4) + hwnd = IntToPtr(V_I4(&v)); + + return hwnd; +} + HRESULT WINAPI WindowFromAccessibleObject(IAccessible *acc, HWND *phwnd) { IDispatch *parent; @@ -406,29 +422,46 @@ HRESULT WINAPI WindowFromAccessibleObject(IAccessible *acc, HWND *phwnd)
TRACE("%p %p\n", acc, phwnd);
- IAccessible_AddRef(acc); - while(1) { - hres = IAccessible_QueryInterface(acc, &IID_IOleWindow, (void**)&ow); + *phwnd = NULL; + hres = IAccessible_QueryInterface(acc, &IID_IOleWindow, (void**)&ow); + if(SUCCEEDED(hres)) { + hres = IOleWindow_GetWindow(ow, phwnd); + IOleWindow_Release(ow); if(SUCCEEDED(hres)) { - hres = IOleWindow_GetWindow(ow, phwnd); - IOleWindow_Release(ow); - IAccessible_Release(acc); + if(!*phwnd) + *phwnd = get_hwnd_from_acc_nav(acc); + return hres; } + } + + *phwnd = get_hwnd_from_acc_nav(acc); + if(*phwnd) + return S_OK;
+ IAccessible_AddRef(acc); + while(1) { hres = IAccessible_get_accParent(acc, &parent); IAccessible_Release(acc); if(FAILED(hres)) return hres; - if(hres!=S_OK || !parent) { - *phwnd = NULL; + if(hres!=S_OK || !parent) return hres; - }
hres = IDispatch_QueryInterface(parent, &IID_IAccessible, (void**)&acc); IDispatch_Release(parent); if(FAILED(hres)) return hres; + + hres = IAccessible_QueryInterface(acc, &IID_IOleWindow, (void**)&ow); + if(SUCCEEDED(hres)) { + hres = IOleWindow_GetWindow(ow, phwnd); + IOleWindow_Release(ow); + if(SUCCEEDED(hres)) { + IAccessible_Release(acc); + return hres; + } + } } }
diff --git a/dlls/oleacc/tests/main.c b/dlls/oleacc/tests/main.c index 7854764ec08..70b66d82ca1 100644 --- a/dlls/oleacc/tests/main.c +++ b/dlls/oleacc/tests/main.c @@ -59,8 +59,10 @@ DEFINE_EXPECT(Accessible_get_accChildCount); DEFINE_EXPECT(Accessible_get_accChild); DEFINE_EXPECT(Accessible_get_accName); DEFINE_EXPECT(Accessible_get_accParent); +DEFINE_EXPECT(Accessible_accNavigate); DEFINE_EXPECT(Accessible_child_get_accName); DEFINE_EXPECT(Accessible_child_get_accParent); +DEFINE_EXPECT(Accessible_child_accNavigate);
static HANDLE (WINAPI *pGetProcessHandleFromHwnd)(HWND);
@@ -91,7 +93,11 @@ static BOOL iface_cmp(IUnknown *iface1, IUnknown *iface2) return unk1 == unk2; }
+static IAccessible Accessible; static IAccessible Accessible_child; +static IOleWindow OleWindow; +static HWND Accessible_accnav_hwnd; +static HWND OleWindow_hwnd;
static HRESULT WINAPI Accessible_QueryInterface( IAccessible *iface, REFIID riid, void **ppvObject) @@ -104,6 +110,12 @@ static HRESULT WINAPI Accessible_QueryInterface( return S_OK; }
+ if(IsEqualIID(riid, &IID_IOleWindow) && (iface == &Accessible)) { + *ppvObject = &OleWindow; + IAccessible_AddRef(iface); + return S_OK; + } + if(IsEqualIID(riid, &IID_IEnumVARIANT)) { CHECK_EXPECT(Accessible_QI_IEnumVARIANT); return E_NOINTERFACE; @@ -155,7 +167,12 @@ static HRESULT WINAPI Accessible_get_accParent( IAccessible *iface, IDispatch **ppdispParent) { if(iface == &Accessible_child) + { CHECK_EXPECT(Accessible_child_get_accParent); + if (OleWindow_hwnd) + return IAccessible_QueryInterface(&Accessible, &IID_IDispatch, + (void **)ppdispParent); + } else CHECK_EXPECT(Accessible_get_accParent);
@@ -292,10 +309,25 @@ static HRESULT WINAPI Accessible_accLocation(IAccessible *iface, LONG *pxLeft, return E_NOTIMPL; }
+#define NAVDIR_INTERNAL_HWND 10 static HRESULT WINAPI Accessible_accNavigate(IAccessible *iface, LONG navDir, VARIANT varStart, VARIANT *pvarEnd) { - ok(0, "unexpected call\n"); + if(iface == &Accessible_child) + CHECK_EXPECT(Accessible_child_accNavigate); + else + CHECK_EXPECT(Accessible_accNavigate); + + /* + * Magic number value for retrieving an HWND. Used by DynamicAnnotation + * IAccessible wrapper. + */ + if(navDir == NAVDIR_INTERNAL_HWND) { + V_VT(pvarEnd) = VT_I4; + V_I4(pvarEnd) = HandleToULong(Accessible_accnav_hwnd); + return S_OK; + } + return E_NOTIMPL; }
@@ -358,8 +390,44 @@ static IAccessibleVtbl AccessibleVtbl = { Accessible_put_accValue };
+static HRESULT WINAPI OleWindow_QueryInterface(IOleWindow *iface, REFIID riid, void **obj) +{ + return IAccessible_QueryInterface(&Accessible, riid, obj); +} + +static ULONG WINAPI OleWindow_AddRef(IOleWindow *iface) +{ + return IAccessible_AddRef(&Accessible); +} + +static ULONG WINAPI OleWindow_Release(IOleWindow *iface) +{ + return IAccessible_Release(&Accessible); +} + +static HRESULT WINAPI OleWindow_GetWindow(IOleWindow *iface, HWND *hwnd) +{ + *hwnd = OleWindow_hwnd; + return S_OK; +} + +static HRESULT WINAPI OleWindow_ContextSensitiveHelp(IOleWindow *iface, BOOL f_enter_mode) +{ + ok(0, "unexpected call\n"); + return E_NOTIMPL; +} + +static const IOleWindowVtbl OleWindowVtbl = { + OleWindow_QueryInterface, + OleWindow_AddRef, + OleWindow_Release, + OleWindow_GetWindow, + OleWindow_ContextSensitiveHelp +}; + static IAccessible Accessible = {&AccessibleVtbl}; static IAccessible Accessible_child = {&AccessibleVtbl}; +static IOleWindow OleWindow = {&OleWindowVtbl};
static void test_getroletext(void) { @@ -1826,6 +1894,56 @@ static void test_default_edit_accessible_object(void) DestroyWindow(hwnd); }
+static void test_WindowFromAccessibleObject(void) +{ + HRESULT hr; + HWND hwnd; + + /* Successfully retrieve an HWND from the IOleWindow interface. */ + Accessible_accnav_hwnd = NULL; + OleWindow_hwnd = (HWND)0xdeadf00d; + hwnd = (HWND)0xdeadbeef; + hr = WindowFromAccessibleObject(&Accessible, &hwnd); + ok(hr == S_OK, "got %lx\n", hr); + ok(hwnd == (HWND)0xdeadf00d, "hwnd != 0xdeadf00d!\n"); + + /* Successfully retrieve an HWND from IAccessible::accNavigate. */ + Accessible_accnav_hwnd = (HWND)0xdeadf00d; + OleWindow_hwnd = NULL; + hwnd = (HWND)0xdeadbeef; + SET_EXPECT(Accessible_accNavigate); + hr = WindowFromAccessibleObject(&Accessible, &hwnd); + ok(hr == S_OK, "got %lx\n", hr); + /* This value gets sign-extended on 64-bit. */ + ok(hwnd == IntToPtr(0xdeadf00d), "hwnd != 0xdeadf00d!\n"); + CHECK_CALLED(Accessible_accNavigate); + + /* Return a NULL HWND from both methods, no accParent call. */ + Accessible_accnav_hwnd = NULL; + OleWindow_hwnd = NULL; + hwnd = (HWND)0xdeadbeef; + SET_EXPECT(Accessible_accNavigate); + hr = WindowFromAccessibleObject(&Accessible, &hwnd); + ok(hr == S_OK, "got %lx\n", hr); + ok(!hwnd, "hwnd %p\n", hwnd); + CHECK_CALLED(Accessible_accNavigate); + + /* Successfully retrieve an HWND from a parent IAccessible's IOleWindow interface. */ + Accessible_accnav_hwnd = NULL; + OleWindow_hwnd = (HWND)0xdeadf00d; + hwnd = (HWND)0xdeadbeef; + SET_EXPECT(Accessible_child_accNavigate); + SET_EXPECT(Accessible_child_get_accParent); + hr = WindowFromAccessibleObject(&Accessible_child, &hwnd); + ok(hr == S_OK, "got %lx\n", hr); + ok(hwnd == (HWND)0xdeadf00d, "hwnd != 0xdeadf00d!\n"); + CHECK_CALLED(Accessible_child_accNavigate); + CHECK_CALLED(Accessible_child_get_accParent); + + Accessible_accnav_hwnd = NULL; + OleWindow_hwnd = NULL; +} + START_TEST(main) { int argc; @@ -1867,6 +1985,7 @@ START_TEST(main) test_AccessibleObjectFromPoint(); test_CreateStdAccessibleObject_classes(); test_default_edit_accessible_object(); + test_WindowFromAccessibleObject();
unregister_window_class(); CoUninitialize();
Hi Connor,
I'm afraid it's not moving in right direction. It's partially caused by returning success and NULL window in some of the tests. While it may be interesting to test we end testing corner behavior only. Some paths in native implementation accepts NULL HWND while other not - it may lead to wrong conclusions.
What do you think about something like in attached diff (generated on top of your patch)? Please note that I didn't test it well. It would be good to understand what SID_AccessibilityUnk is for.
Thanks, Piotr
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=114709
Your paranoid android.
=== debian11 (build log) ===
error: patch failed: dlls/oleacc/main.c:398 Task: Patch failed to apply
=== debian11 (build log) ===
error: patch failed: dlls/oleacc/main.c:398 Task: Patch failed to apply
On Sat, May 14, 2022 at 06:54:27PM +0200, Piotr Caban wrote:
Hi Connor,
I'm afraid it's not moving in right direction. It's partially caused by returning success and NULL window in some of the tests. While it may be interesting to test we end testing corner behavior only. Some paths in native implementation accepts NULL HWND while other not - it may lead to wrong conclusions.
What do you think about something like in attached diff (generated on top of your patch)? Please note that I didn't test it well. It would be good to understand what SID_AccessibilityUnk is for.
Thanks, Piotr
Hi Piotr,
SID_AccessibilityUnk is used to get the wrapped IAccessible out of the Dynamic Annotation wrapper. This ends up being useful due to the fact that the IAccessible wrapper doesn't pass QI calls to the IAccessible it wraps, which means things like IOleWindow don't get passed through.
The diff you've attached looks good. Is it good for you? Would you like more tests around this?
Thanks, Connor.
On Mon, May 16, 2022 at 09:40:33AM -0400, Connor McAdams wrote:
On Sat, May 14, 2022 at 06:54:27PM +0200, Piotr Caban wrote:
Hi Connor,
I'm afraid it's not moving in right direction. It's partially caused by returning success and NULL window in some of the tests. While it may be interesting to test we end testing corner behavior only. Some paths in native implementation accepts NULL HWND while other not - it may lead to wrong conclusions.
What do you think about something like in attached diff (generated on top of your patch)? Please note that I didn't test it well. It would be good to understand what SID_AccessibilityUnk is for.
Thanks, Piotr
Hi Piotr,
SID_AccessibilityUnk is used to get the wrapped IAccessible out of the Dynamic Annotation wrapper. This ends up being useful due to the fact that the IAccessible wrapper doesn't pass QI calls to the IAccessible it wraps, which means things like IOleWindow don't get passed through.
The diff you've attached looks good. Is it good for you? Would you like more tests around this?
Thanks, Connor.
Oh, just as an example of this: You can test this out in test_AccessibleObjectFromEvent, where we have:
todo_wine ok(!iface_cmp((IUnknown*)acc, (IUnknown*)&Accessible), "acc == &Accessible\n");
If you query for SID_AccessibilityUnk, you'll get back the &Accessible pointer, since AccessibleObjectFromEvent is returning a Dynamic Annotation wrapper IAccessible.
On 5/16/22 15:53, Connor McAdams wrote:
On Mon, May 16, 2022 at 09:40:33AM -0400, Connor McAdams wrote:
On Sat, May 14, 2022 at 06:54:27PM +0200, Piotr Caban wrote:
Hi Connor,
I'm afraid it's not moving in right direction. It's partially caused by returning success and NULL window in some of the tests. While it may be interesting to test we end testing corner behavior only. Some paths in native implementation accepts NULL HWND while other not - it may lead to wrong conclusions.
What do you think about something like in attached diff (generated on top of your patch)? Please note that I didn't test it well. It would be good to understand what SID_AccessibilityUnk is for.
Thanks, Piotr
Hi Piotr,
SID_AccessibilityUnk is used to get the wrapped IAccessible out of the Dynamic Annotation wrapper. This ends up being useful due to the fact that the IAccessible wrapper doesn't pass QI calls to the IAccessible it wraps, which means things like IOleWindow don't get passed through.
The diff you've attached looks good. Is it good for you? Would you like more tests around this?
Thanks, Connor.
Oh, just as an example of this: You can test this out in test_AccessibleObjectFromEvent, where we have:
todo_wine ok(!iface_cmp((IUnknown*)acc, (IUnknown*)&Accessible), "acc == &Accessible\n");
If you query for SID_AccessibilityUnk, you'll get back the &Accessible pointer, since AccessibleObjectFromEvent is returning a Dynamic Annotation wrapper IAccessible.
Since you have figured it out and it's easy to test I think it will make sense to add the test to wine. It doesn't have to be part of the WindowFromAccessibleObject patch.
Thanks, Piotr
On 5/16/22 15:40, Connor McAdams wrote:
SID_AccessibilityUnk is used to get the wrapped IAccessible out of the Dynamic Annotation wrapper. This ends up being useful due to the fact that the IAccessible wrapper doesn't pass QI calls to the IAccessible it wraps, which means things like IOleWindow don't get passed through.
Since you already know what it is for it might be good to rename SID_AccessibilityUnk to something more descriptive (this name was introduced by me to show what native is doing).
The diff you've attached looks good. Is it good for you? Would you like more tests around this?
I was planning to have another look on the tests. I didn't look on it closely yet.
Thanks, Piotr
Hi Connor,
On 5/13/22 18:43, Connor McAdams wrote:
@@ -155,7 +167,12 @@ static HRESULT WINAPI Accessible_get_accParent( IAccessible *iface, IDispatch **ppdispParent) { if(iface == &Accessible_child)
- { CHECK_EXPECT(Accessible_child_get_accParent);
if (OleWindow_hwnd) > + return IAccessible_QueryInterface(&Accessible,
&IID_IDispatch,
(void **)ppdispParent);
I don't understand this part. Why do you need to check OleWindow_hwnd? It would probably make sense to return parent unconditionally.
+#define NAVDIR_INTERNAL_HWND 10 static HRESULT WINAPI Accessible_accNavigate(IAccessible *iface, LONG navDir, VARIANT varStart, VARIANT *pvarEnd) {
- ok(0, "unexpected call\n");
- if(iface == &Accessible_child)
CHECK_EXPECT(Accessible_child_accNavigate);- else
CHECK_EXPECT(Accessible_accNavigate);- /*
* Magic number value for retrieving an HWND. Used by DynamicAnnotation* IAccessible wrapper.*/- if(navDir == NAVDIR_INTERNAL_HWND) {
V_VT(pvarEnd) = VT_I4;V_I4(pvarEnd) = HandleToULong(Accessible_accnav_hwnd);return S_OK;
S_FALSE or E_INVALIDARG looks like a better return value when Accessible_accnav_hwnd is NULL (unless there's value in testing S_OK). How about changing Accessible_accnav_hwnd name to Accessible_hwnd?
+static HRESULT WINAPI OleWindow_GetWindow(IOleWindow *iface, HWND *hwnd) +{
- *hwnd = OleWindow_hwnd;
- return S_OK;
Again, I don't know if there's value in testing NULL HWND and S_OK return. If not, documentation suggests returning E_FAIL.
Thanks, Piotr
On Mon, May 16, 2022 at 04:17:10PM +0200, Piotr Caban wrote:
Hi Connor,
On 5/13/22 18:43, Connor McAdams wrote:
@@ -155,7 +167,12 @@ static HRESULT WINAPI Accessible_get_accParent( IAccessible *iface, IDispatch **ppdispParent) { if(iface == &Accessible_child)
- { CHECK_EXPECT(Accessible_child_get_accParent);
if (OleWindow_hwnd) > + returnIAccessible_QueryInterface(&Accessible,
&IID_IDispatch,
(void **)ppdispParent);I don't understand this part. Why do you need to check OleWindow_hwnd? It would probably make sense to return parent unconditionally.
Largely to gate returning a parent interface in other tests that call get_accParent on Accessible_child. The idea was to only return a parent interface for the WindowFromAccessibleObject tests where we're trying to retrieve an HWND from the parent's IOleWindow interface.
+#define NAVDIR_INTERNAL_HWND 10 static HRESULT WINAPI Accessible_accNavigate(IAccessible *iface, LONG navDir, VARIANT varStart, VARIANT *pvarEnd) {
- ok(0, "unexpected call\n");
- if(iface == &Accessible_child)
CHECK_EXPECT(Accessible_child_accNavigate);- else
CHECK_EXPECT(Accessible_accNavigate);- /*
* Magic number value for retrieving an HWND. Used by DynamicAnnotation* IAccessible wrapper.*/- if(navDir == NAVDIR_INTERNAL_HWND) {
V_VT(pvarEnd) = VT_I4;V_I4(pvarEnd) = HandleToULong(Accessible_accnav_hwnd);return S_OK;S_FALSE or E_INVALIDARG looks like a better return value when Accessible_accnav_hwnd is NULL (unless there's value in testing S_OK). How about changing Accessible_accnav_hwnd name to Accessible_hwnd?
I was mainly testing whether or not it'd accept returning a NULL hwnd, or continue trying to use other methods to ascertain the HWND if NULL is returned. I can try other return codes. As it is, it seems that if the IAccessible doesn't have an IOleWindow interface, and the accNav method returns a NULL hwnd, it goes up the parent chain. If the accNav method returns a non-NULL hwnd, it returns immediately.
+static HRESULT WINAPI OleWindow_GetWindow(IOleWindow *iface, HWND *hwnd) +{
- *hwnd = OleWindow_hwnd;
- return S_OK;
Again, I don't know if there's value in testing NULL HWND and S_OK return. If not, documentation suggests returning E_FAIL.
Thanks, Piotr
Sure. I can do E_FAIL here instead if the hwnd is NULL. I hadn't looked at the IOleWindow documentation, guess I should have. :)
Thanks for the review.
-
Connor McAdams -
Marvin -
Piotr Caban