Bug 230257 – [GTK][a11y] Add implementation of component interface when building with ATSPI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Implement the component interface

[Carlos Garcia Campos]

Created attachment 440592 [details]
Patch

[Carlos Garcia Campos]

The API test failures must be unrelated, this patch only affects a11y and when building with atspi enabled, which is disabled by default.

[Andres Gonzalez]

(In reply to Carlos Garcia Campos from comment #1)
> Created attachment 440592 [details]
> Patch

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h

+    RefPtr<AXIsolatedObject> nodeForID(AXID) const;

nodeForID belongs to the tree not to the isolated object. I think it is a better design to keep it in AXIsolatedTree. Do you have a reason to move it to AXIsolatedObject?

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #3)
> (In reply to Carlos Garcia Campos from comment #1)
> > Created attachment 440592 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> 
> +    RefPtr<AXIsolatedObject> nodeForID(AXID) const;
> 
> nodeForID belongs to the tree not to the isolated object. I think it is a
> better design to keep it in AXIsolatedTree. Do you have a reason to move it
> to AXIsolatedObject?

Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR actually. Since WTR calls accessibility api from the web process main thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's always expected to be called from the secondary thread (nodeForID only works from the a11y thread). I can't use callOnAXthreadAndWait, because first thing AXIsolatedObject::accessibilityHitTest() is retrieveValueFromMainThread() but main thread is blocked. So, I split it adding AccessibilityObjectAtspi::objectAtPoint() that is always called by the main thread and does the coordinate conversion + accessibilityHitTest(). When called from the main thread (WTR only) it's called directly and the wrapper object returned. When called from the AX thread I need the nodeForID() call to get the wrapper from the isolated object. 

I agree nodeForID doesn't belong to AXIsolatedObject, so the other solution would be to just make AXIsolatedObject::tree() public.

[Andres Gonzalez]

(In reply to Carlos Garcia Campos from comment #4)
> (In reply to Andres Gonzalez from comment #3)
> > (In reply to Carlos Garcia Campos from comment #1)
> > > Created attachment 440592 [details]
> > > Patch
> > 
> > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > 
> > +    RefPtr<AXIsolatedObject> nodeForID(AXID) const;
> > 
> > nodeForID belongs to the tree not to the isolated object. I think it is a
> > better design to keep it in AXIsolatedTree. Do you have a reason to move it
> > to AXIsolatedObject?
> 
> Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR
> actually. Since WTR calls accessibility api from the web process main
> thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's
> always expected to be called from the secondary thread (nodeForID only works
> from the a11y thread). I can't use callOnAXthreadAndWait, because first
> thing AXIsolatedObject::accessibilityHitTest() is
> retrieveValueFromMainThread() but main thread is blocked. So, I split it
> adding AccessibilityObjectAtspi::objectAtPoint() that is always called by
> the main thread and does the coordinate conversion + accessibilityHitTest().
> When called from the main thread (WTR only) it's called directly and the
> wrapper object returned. When called from the AX thread I need the
> nodeForID() call to get the wrapper from the isolated object. 

Can't you get the wrapper from the isolated object? AXCoreObject::wrapper() should work in all objects, isolated or live.

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #5)
> (In reply to Carlos Garcia Campos from comment #4)
> > (In reply to Andres Gonzalez from comment #3)
> > > (In reply to Carlos Garcia Campos from comment #1)
> > > > Created attachment 440592 [details]
> > > > Patch
> > > 
> > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > 
> > > +    RefPtr<AXIsolatedObject> nodeForID(AXID) const;
> > > 
> > > nodeForID belongs to the tree not to the isolated object. I think it is a
> > > better design to keep it in AXIsolatedTree. Do you have a reason to move it
> > > to AXIsolatedObject?
> > 
> > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR
> > actually. Since WTR calls accessibility api from the web process main
> > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's
> > always expected to be called from the secondary thread (nodeForID only works
> > from the a11y thread). I can't use callOnAXthreadAndWait, because first
> > thing AXIsolatedObject::accessibilityHitTest() is
> > retrieveValueFromMainThread() but main thread is blocked. So, I split it
> > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by
> > the main thread and does the coordinate conversion + accessibilityHitTest().
> > When called from the main thread (WTR only) it's called directly and the
> > wrapper object returned. When called from the AX thread I need the
> > nodeForID() call to get the wrapper from the isolated object. 
> 
> Can't you get the wrapper from the isolated object? AXCoreObject::wrapper()
> should work in all objects, isolated or live.

hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's safe to send the id from main to AX thread, but not the wrapper?

[Andres Gonzalez]

(In reply to Carlos Garcia Campos from comment #6)
> (In reply to Andres Gonzalez from comment #5)
> > (In reply to Carlos Garcia Campos from comment #4)
> > > (In reply to Andres Gonzalez from comment #3)
> > > > (In reply to Carlos Garcia Campos from comment #1)
> > > > > Created attachment 440592 [details]
> > > > > Patch
> > > > 
> > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > > 
> > > > +    RefPtr<AXIsolatedObject> nodeForID(AXID) const;
> > > > 
> > > > nodeForID belongs to the tree not to the isolated object. I think it is a
> > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it
> > > > to AXIsolatedObject?
> > > 
> > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR
> > > actually. Since WTR calls accessibility api from the web process main
> > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's
> > > always expected to be called from the secondary thread (nodeForID only works
> > > from the a11y thread). I can't use callOnAXthreadAndWait, because first
> > > thing AXIsolatedObject::accessibilityHitTest() is
> > > retrieveValueFromMainThread() but main thread is blocked. So, I split it
> > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by
> > > the main thread and does the coordinate conversion + accessibilityHitTest().
> > > When called from the main thread (WTR only) it's called directly and the
> > > wrapper object returned. When called from the AX thread I need the
> > > nodeForID() call to get the wrapper from the isolated object. 
> > 
> > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper()
> > should work in all objects, isolated or live.
> 
> hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed
> that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's
> safe to send the id from main to AX thread, but not the wrapper?

Let's go back for a moment to the origin of the problem. As you said, you couldn't use AccessibilityController::executeOnAXThreadAndWait for HitTest because HitTest  will dispatch back to the main thread that it would be waiting, resulting in deadlock. The way we worked around this was by:

AccessibilityController::executeOnAXThreadAndWait calls
AXThread::dispatch that in turn calls
axThread.wakeUpRunLoop

On Mac wakeUpRunLoop does

void AXThread::wakeUpRunLoop()
{
    CFRunLoopSourceSignal(m_threadRunLoopSource.get());
    CFRunLoopWakeUp(m_threadRunLoop.get());
}

But it is a no-op in GTK. Could we try something equivalent in GTK?

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #7)
> (In reply to Carlos Garcia Campos from comment #6)
> > (In reply to Andres Gonzalez from comment #5)
> > > (In reply to Carlos Garcia Campos from comment #4)
> > > > (In reply to Andres Gonzalez from comment #3)
> > > > > (In reply to Carlos Garcia Campos from comment #1)
> > > > > > Created attachment 440592 [details]
> > > > > > Patch
> > > > > 
> > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > > > 
> > > > > +    RefPtr<AXIsolatedObject> nodeForID(AXID) const;
> > > > > 
> > > > > nodeForID belongs to the tree not to the isolated object. I think it is a
> > > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it
> > > > > to AXIsolatedObject?
> > > > 
> > > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR
> > > > actually. Since WTR calls accessibility api from the web process main
> > > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's
> > > > always expected to be called from the secondary thread (nodeForID only works
> > > > from the a11y thread). I can't use callOnAXthreadAndWait, because first
> > > > thing AXIsolatedObject::accessibilityHitTest() is
> > > > retrieveValueFromMainThread() but main thread is blocked. So, I split it
> > > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by
> > > > the main thread and does the coordinate conversion + accessibilityHitTest().
> > > > When called from the main thread (WTR only) it's called directly and the
> > > > wrapper object returned. When called from the AX thread I need the
> > > > nodeForID() call to get the wrapper from the isolated object. 
> > > 
> > > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper()
> > > should work in all objects, isolated or live.
> > 
> > hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed
> > that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's
> > safe to send the id from main to AX thread, but not the wrapper?
> 
> Let's go back for a moment to the origin of the problem. As you said, you
> couldn't use AccessibilityController::executeOnAXThreadAndWait for HitTest
> because HitTest  will dispatch back to the main thread that it would be
> waiting, resulting in deadlock. The way we worked around this was by:
> 
> AccessibilityController::executeOnAXThreadAndWait calls
> AXThread::dispatch that in turn calls
> axThread.wakeUpRunLoop
> 
> On Mac wakeUpRunLoop does
> 
> void AXThread::wakeUpRunLoop()
> {
>     CFRunLoopSourceSignal(m_threadRunLoopSource.get());
>     CFRunLoopWakeUp(m_threadRunLoop.get());
> }
> 
> But it is a no-op in GTK. Could we try something equivalent in GTK?

The issue is not unique to HitTest, but you would have the same problem for any call into the API that needs to dispatch to the main thread. So we need to find a general solution.

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #7)
> (In reply to Carlos Garcia Campos from comment #6)
> > (In reply to Andres Gonzalez from comment #5)
> > > (In reply to Carlos Garcia Campos from comment #4)
> > > > (In reply to Andres Gonzalez from comment #3)
> > > > > (In reply to Carlos Garcia Campos from comment #1)
> > > > > > Created attachment 440592 [details]
> > > > > > Patch
> > > > > 
> > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > > > 
> > > > > +    RefPtr<AXIsolatedObject> nodeForID(AXID) const;
> > > > > 
> > > > > nodeForID belongs to the tree not to the isolated object. I think it is a
> > > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it
> > > > > to AXIsolatedObject?
> > > > 
> > > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR
> > > > actually. Since WTR calls accessibility api from the web process main
> > > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's
> > > > always expected to be called from the secondary thread (nodeForID only works
> > > > from the a11y thread). I can't use callOnAXthreadAndWait, because first
> > > > thing AXIsolatedObject::accessibilityHitTest() is
> > > > retrieveValueFromMainThread() but main thread is blocked. So, I split it
> > > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by
> > > > the main thread and does the coordinate conversion + accessibilityHitTest().
> > > > When called from the main thread (WTR only) it's called directly and the
> > > > wrapper object returned. When called from the AX thread I need the
> > > > nodeForID() call to get the wrapper from the isolated object. 
> > > 
> > > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper()
> > > should work in all objects, isolated or live.
> > 
> > hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed
> > that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's
> > safe to send the id from main to AX thread, but not the wrapper?
> 
> Let's go back for a moment to the origin of the problem. As you said, you
> couldn't use AccessibilityController::executeOnAXThreadAndWait for HitTest
> because HitTest  will dispatch back to the main thread that it would be
> waiting, resulting in deadlock. The way we worked around this was by:
> 
> AccessibilityController::executeOnAXThreadAndWait calls
> AXThread::dispatch that in turn calls
> axThread.wakeUpRunLoop
> 
> On Mac wakeUpRunLoop does
> 
> void AXThread::wakeUpRunLoop()
> {
>     CFRunLoopSourceSignal(m_threadRunLoopSource.get());
>     CFRunLoopWakeUp(m_threadRunLoop.get());
> }
> 
> But it is a no-op in GTK. Could we try something equivalent in GTK?

hmm, I don't understand how waking up the AX thread helps.

[Carlos Garcia Campos]

The AX thread run loop I meant.

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #8)
> (In reply to Andres Gonzalez from comment #7)
> > (In reply to Carlos Garcia Campos from comment #6)
> > > (In reply to Andres Gonzalez from comment #5)
> > > > (In reply to Carlos Garcia Campos from comment #4)
> > > > > (In reply to Andres Gonzalez from comment #3)
> > > > > > (In reply to Carlos Garcia Campos from comment #1)
> > > > > > > Created attachment 440592 [details]
> > > > > > > Patch
> > > > > > 
> > > > > > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > > > > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
> > > > > > 
> > > > > > +    RefPtr<AXIsolatedObject> nodeForID(AXID) const;
> > > > > > 
> > > > > > nodeForID belongs to the tree not to the isolated object. I think it is a
> > > > > > better design to keep it in AXIsolatedTree. Do you have a reason to move it
> > > > > > to AXIsolatedObject?
> > > > > 
> > > > > Yes, I need it in AccessibilityObjectAtspi::hitTest() only because of WTR
> > > > > actually. Since WTR calls accessibility api from the web process main
> > > > > thread, I can't use AXIsolatedObject::accessibilityHitTest() because it's
> > > > > always expected to be called from the secondary thread (nodeForID only works
> > > > > from the a11y thread). I can't use callOnAXthreadAndWait, because first
> > > > > thing AXIsolatedObject::accessibilityHitTest() is
> > > > > retrieveValueFromMainThread() but main thread is blocked. So, I split it
> > > > > adding AccessibilityObjectAtspi::objectAtPoint() that is always called by
> > > > > the main thread and does the coordinate conversion + accessibilityHitTest().
> > > > > When called from the main thread (WTR only) it's called directly and the
> > > > > wrapper object returned. When called from the AX thread I need the
> > > > > nodeForID() call to get the wrapper from the isolated object. 
> > > > 
> > > > Can't you get the wrapper from the isolated object? AXCoreObject::wrapper()
> > > > should work in all objects, isolated or live.
> > > 
> > > hmm, I'm not sure it's ok to get the wrapper from the main thread, I assumed
> > > that AXIsolatedObject::accessibilityHitTest() returned the AXID because it's
> > > safe to send the id from main to AX thread, but not the wrapper?
> > 
> > Let's go back for a moment to the origin of the problem. As you said, you
> > couldn't use AccessibilityController::executeOnAXThreadAndWait for HitTest
> > because HitTest  will dispatch back to the main thread that it would be
> > waiting, resulting in deadlock. The way we worked around this was by:
> > 
> > AccessibilityController::executeOnAXThreadAndWait calls
> > AXThread::dispatch that in turn calls
> > axThread.wakeUpRunLoop
> > 
> > On Mac wakeUpRunLoop does
> > 
> > void AXThread::wakeUpRunLoop()
> > {
> >     CFRunLoopSourceSignal(m_threadRunLoopSource.get());
> >     CFRunLoopWakeUp(m_threadRunLoop.get());
> > }
> > 
> > But it is a no-op in GTK. Could we try something equivalent in GTK?
> 
> The issue is not unique to HitTest, but you would have the same problem for
> any call into the API that needs to dispatch to the main thread. So we need
> to find a general solution.

I managed to handle all other cases by just splitting the methods, a common one that always runs in the main thread (or simply calls retrieveValueFromMainThread) and the one that is always expected to be called from the AX thread. For requests coming from the ATSPI service I use the method that is expected to be called from the AX thread and calls the common one. From WTR I call the common one directly from the main thread, since retrieveValueFromMainThread just calls the lambda if already in the main thread. That's the advantage of using the WebCore wrappers directly from WTR, without any a11y toolkit in the middle. There are a few cases like children() that I always run from the AX thread in WTR because those don't schedule a main thread task.

I have the WTR changes in a local commit that I plan to land after landing the text interface support, that's why you still see stubs for everything when building with atspi.

[Carlos Garcia Campos]

Created attachment 441055 [details]
Patch

Removed the cross-platform changes in the end.

[Andres Gonzalez]

(In reply to Carlos Garcia Campos from comment #12)
> Created attachment 441055 [details]
> Patch
> 
> Removed the cross-platform changes in the end.

Sorry that is taking me long to get back. So you are not doing layout tests for HitTest? 

I have to look at the question concerning the runloop wakeup. The intention at least was to wakeup the main thread to avoid the deadlock, but there may be a bug.

Concerning LayoutTests calling different functions than the actual AT client, we would like to avoid that as a way to go in the backdoor. I think it would be good that the LayoutTests JS uses the same entry points into accessibility as a real client would.

[Carlos Garcia Campos]

(In reply to Andres Gonzalez from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > Created attachment 441055 [details]
> > Patch
> > 
> > Removed the cross-platform changes in the end.
> 
> Sorry that is taking me long to get back.

No problem!

> So you are not doing layout tests
> for HitTest? 

Yes, but the WTR support is not upstream yet, and ATSPI implementation is disabled by default because it's still work in progress, so no bots are running tests with ATSPI.

> I have to look at the question concerning the runloop wakeup. The intention
> at least was to wakeup the main thread to avoid the deadlock, but there may
> be a bug.

But the main loop is blocked in a semaphore, or weaking up the main thread loop in the AX thread makes the code scheduled for the main thread to run in the AX thread?

> Concerning LayoutTests calling different functions than the actual AT
> client, we would like to avoid that as a way to go in the backdoor. I think
> it would be good that the LayoutTests JS uses the same entry points into
> accessibility as a real client would.

They will end up running the same code. In any case, I think it will be easier to discuss the WTR implementation once I submit the patch for it. I plan to upstream the text implementation after the component one, and then add the initial WTR support.

[Carlos Garcia Campos]

Committed r284367 (243152@main): <https://commits.webkit.org/243152@main>