Bug 51932 – GTK: AX: atk tests need to be updated after recent changes

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

With the change to support WK2 accessibility, the root object of the AX hierarchy may be different from what GTK expects. 

Many of the atktests are failing because 

AtkObject* object = gtk_widget_get_accessible(GTK_WIDGET(webView));

is returning something different.

These tests (or some method) needs to be updated to reflect the new hierarchy

AXScrollView -> AXWebArea -> elements

[chris fleizach]

I don't know enough about GTK to fix this on my own. I'm not sure where gtk_widget_get_accessible() gets its result from

[Martin Robinson]

I'll skip all the ATK tests for now, so that we can keep the tree green. Mario is probably the best person to look at these failures.

[chris fleizach]

(In reply to comment #2)
> I'll skip all the ATK tests for now, so that we can keep the tree green. Mario is probably the best person to look at these failures.

Thanks. 

Mario, if you need any help when looking at this, email me right away.

[Martin Robinson]

Created attachment 78165 [details]
Patch

[Mario Sanchez Prada]

(In reply to comment #3)
> (In reply to comment #2)
> > I'll skip all the ATK tests for now, so that we can keep the tree green. Mario is probably the best person to look at these failures.
> 
> Thanks. 
> 
> Mario, if you need any help when looking at this, email me right away.

I just came back from holidays and I have to say my plans for tomorrow where more about finishing some paperwork I need to get done soon but suddently all my priorities have changed :-)

I'll work on this tomorrow morning (in 9 h from now) with top priority, fortunately I think I have an idea of where the problem is and how I could try to fix it... let's see if I'm right.

Martin, Chris, don't worry, I'll keep you informed on this and won't hesitate to ask you if I need some help, you've been warned! :-)

Btw, happy new year, a bit late, but here it is anyway.

[Martin Robinson]

Comment on attachment 78165 [details]
Patch

Clearing flags on attachment: 78165

Committed r75201: <http://trac.webkit.org/changeset/75201>

[Mario Sanchez Prada]

(In reply to comment #0)
> With the change to support WK2 accessibility, the root object of the AX hierarchy may be different from what GTK expects. 
> 
> Many of the atktests are failing because 
> 
> AtkObject* object = gtk_widget_get_accessible(GTK_WIDGET(webView));
> 
> is returning something different.

Yep, the problem is that now, with the new hierarchy, WebKitGTK is returning an 'unknown' object (from the POV of ATK roles) as the wrapper for the AXScrollView AccessibleObject.

In other words, WebKitGTK is returning the parent AtkObject of the object it should be returning instead (that would have the AtkRole ATK_ROLE_DOCUMENT_FRAME, instead of ATK_ROLE_UNKNOWN), so that's why ATK tests are failing now: the hierarchy is shifted.


> These tests (or some method) needs to be updated to reflect the new hierarchy
> 
> AXScrollView -> AXWebArea -> elements

Yes, however I wonder whether the actual fix for now (at least while WK2 is not supported in the GTK port) would be, instead of changing all the tests to expect such object as the root AtkObject, just to change the implementation of gtk_widget_get_accessible() in WebKitWebView so it actually keeps returning the old, well-known, AtkObject of role ATK_ROLE_DOCUMENT_FRAME.

That way we'd be obviously ignoring the new AXScrollView object in the GTK port, but I guess it could be a good enough solution while WK2 a11y is not supported in this port... or perhaps I'm missing something crucial and I'm just saying a completely stupid thing.

So, opinions?

Btw, a patch that would fix the issue by changing what gtk_widget_get_accessible() returns (tested to make testatk and testatkroles work fine again) would be something like this:

--- a/WebKit/gtk/webkit/webkitwebview.cpp
+++ b/WebKit/gtk/webkit/webkitwebview.cpp
@@ -1452,11 +1452,24 @@ static AtkObject* webkit_web_view_get_accessible(GtkWidget* widget)
     if (!doc)
         return NULL;
 
-    AccessibilityObject* coreAccessible = doc->axObjectCache()->rootObject();
-    if (!coreAccessible || !coreAccessible->wrapper())
+    AccessibilityObject* rootAccessible = doc->axObjectCache()->rootObject();
+    if (!rootAccessible)
         return NULL;
 
-    return coreAccessible->wrapper();
+    // We need to return the root accessibility object's first child
+    // to get to the actual ATK Object associated with the web view.
+    // See https://bugs.webkit.org/show_bug.cgi?id=51932
+    AtkObject* axRoot = rootAccessible->wrapper();
+    if (!axRoot || !ATK_IS_OBJECT(axRoot))
+        return NULL;
+
+    AtkObject* axWebView = atk_object_ref_accessible_child(ATK_OBJECT(axRoot), 0);
+    if (!axWebView || !ATK_IS_OBJECT(axWebView))
+        return NULL;
+
+    // We don't want the extra reference returned by ref_accessible_child.
+    g_object_unref(axWebView);
+    return axWebView;
 }

[Mario Sanchez Prada]

Created attachment 78221 [details]
Patch proposal (change gtk_widget_get_accessible in WebKitWebView)

Attaching the patch just in case we all agree on it, in order to speed things up

[chris fleizach]

> 
> > These tests (or some method) needs to be updated to reflect the new hierarchy
> > 
> > AXScrollView -> AXWebArea -> elements
> 
> Yes, however I wonder whether the actual fix for now (at least while WK2 is not supported in the GTK port) would be, instead of changing all the tests to expect such object as the root AtkObject, just to change the implementation of gtk_widget_get_accessible() in WebKitWebView so it actually keeps returning the old, well-known, AtkObject of role ATK_ROLE_DOCUMENT_FRAME.
> 
> That way we'd be obviously ignoring the new AXScrollView object in the GTK port, but I guess it could be a good enough solution while WK2 a11y is not supported in this port... or perhaps I'm missing something crucial and I'm just saying a completely stupid thing.
> 
> So, opinions?
> 

My question is, does GTK need to support an AXScrollView in the AX hierarchy? If they don't, then your patch looks fine because it just ignores what it doesn't need to know about

if however, GTK, should have a scroll view around the web areas in accessibility, then the correct fix is to incorporate the scroll view in to the tests as the root object, or add a method to return the root web area that is used instead of the root scroll view.

[Mario Sanchez Prada]

Created attachment 78242 [details]
Patch proposal (change gtk_widget_get_accessible in WebKitWebView + fix bottom up navigation)

Attaching a new patch, this time seriously asking for revision after having checked with Joanmarie Diggs a couple of issues I was missing before (bottom up navigation through the get_parent() method), and confirming that returning the WebArea object when calling gtk_widget_get_accessible(webView) is the right thing to do (opposite to returning the scrollArea).

The patch also unskip the tests, I guess it's fine that way.

[chris fleizach]

Comment on attachment 78242 [details]
Patch proposal (change gtk_widget_get_accessible in WebKitWebView + fix bottom up navigation)

looks good

[Mario Sanchez Prada]

Sorry Chris, I didn't see your reply before attaching my last patch

(In reply to comment #9)
> [...]
> 
> My question is, does GTK need to support an AXScrollView in the AX hierarchy? 
> If they don't, then your patch looks fine because it just ignores what it 
> doesn't need to know about

After talking through IRC with Joanmarie, it seems the right thing is to keep returning the WebArea role in that case, so my patch would actually make sense. However, some additional work was still needed to fix bottom up navigation through the get_parent() method of AtkObject, since there's always a point where there's apparently no parent and we need to manually work it around by checking that (1) we're in the root a11y object of webCore and (2) getting the accessible object from GAIL associated to the GtkWidget acting as the container of the WebView.

> if however, GTK, should have a scroll view around the web areas in 
> accessibility, then the correct fix is to incorporate the scroll view in to 
> the tests as the root object, or add a method to return the root web area
> that is used instead of the root scroll view.

AFAIK after talking to Joanmarie, this is not actually needed, at least for the moment, so I guess the correct approach is the already proposed one.

Thanks a lot for the feedback, even if I read it *after* attaching the last patch!

[Mario Sanchez Prada]

Committed r75250: <http://trac.webkit.org/changeset/75250>