Bug 25989 – [Gtk] Enable accessibility in Gtk DRT

• Component: Accessibility
• Status: RESOLVED FIXED

[Jan Alonzo]

We need to enable accessibility support in DRT Gtk so we can run the AX tests and hopefully be able to contribute more to these tests, esp. now that we're getting serious with implementing the necessary functionality to have the port accessibility-enabled.

[Jan Alonzo]

Created attachment 30765 [details]
initial impl

Initial support to enable these tests. Need to implement getting the focused element to make this useful as most of the tests rely on getting the "focused UI element" to determine whether the test passed or fail.

[Jan Alonzo]

Created attachment 31041 [details]
Add Accessibility support to GTK DRT

This patch adds basic accessibility support to gtk DRT so we can get things going. Only four tests are passing at the moment and I've enabled them in this patch.

Just an aside: A lot of the tests depend on an accessible element's attributes and I don't think we have that yet. Once we get that support, we should be able to enable more tests.

[Xan Lopez]

Comment on attachment 31041 [details]
Add Accessibility support to GTK DRT

> +AtkObject* webkit_accessible_get_focused_element(WebKitAccessible* accessible)
> +{
> +    if (!accessible->m_object)
> +        return 0;
> +
> +    // FIXME: update the backing store once we have one.
> +
> +    RefPtr<AccessibilityObject> focusedObj = accessible->m_object->focusedUIElement();
> +    if (!focusedObj)
> +        return 0;
> +
> +    return focusedObj->wrapper();
> +}
> +

I can never decide if we should add new API in one patch and its usage in another, or everything in one patch.

> diff --git a/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp b/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
> index c78323e..48494c3 100644
> --- a/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
> +++ b/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
> @@ -28,6 +28,8 @@
>  
>  #include <JavaScriptCore/JSRetainPtr.h>
>  
> +#include <limits.h>
> +
>  // Static Functions
>  

I would put this in a different patch.

> +AccessibilityUIElement AccessibilityUIElement::getChildAtIndex(unsigned index)
> +{
> +    Vector<AccessibilityUIElement> children;
> +    getChildrenWithRange(children, index, 1);
> +
> +    if (children.size() == 1)
> +        return children.at(0);
> +
> +    return 0;
> +}

Mmm, why can't you just use atk_object_ref_accessible_child with index here?

> +AccessibilityUIElement AccessibilityUIElement::parentElement()
> +{
> +    g_assert(m_element);

I guess we should use either ASSERT or g_assert, and stick to that.

> +
> +double AccessibilityUIElement::intValue()
> +{
> +    GValue value = { 0, { { 0 } } };

I suppose { 0, } like in C does not work in C++.

> +
> +    if (!ATK_IS_VALUE(m_element))
> +        return 0.0f;
> +
> +    atk_value_get_current_value(ATK_VALUE(m_element), &value);
> +    return g_value_get_double(&value);

How do we know the GValue has a double value? Shouldn't we do a G_VALUE_HOLDS_DOUBLE before?

> @@ -432,7 +434,10 @@ static void webViewWindowObjectCleared(WebKitWebView* view, WebKitWebFrame* fram
>      assert(gLayoutTestController);
>  
>      gLayoutTestController->makeWindowObject(context, windowObject, &exception);
> -    assert(!exception);
> +    ASSERT(!exception);
> +
> +    accessibilityController->makeWindowObject(context, windowObject, &exception);
> +    ASSERT(!exception);

Stupid question, where is this created? :D

The patch looks pretty good to me, but I'll mark it as r- while we work out the last details.

[Jan Alonzo]

(In reply to comment #3)
> > +++ b/WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp
> > @@ -28,6 +28,8 @@
> >  
> >  #include <JavaScriptCore/JSRetainPtr.h>
> >  
> > +#include <limits.h>
> > +
> >  // Static Functions
> >  
> 
> I would put this in a different patch.

Ok.

> 
> > +AccessibilityUIElement AccessibilityUIElement::getChildAtIndex(unsigned index)
> > +{
> > +    Vector<AccessibilityUIElement> children;
> > +    getChildrenWithRange(children, index, 1);
> > +
> > +    if (children.size() == 1)
> > +        return children.at(0);
> > +
> > +    return 0;
> > +}
> 
> Mmm, why can't you just use atk_object_ref_accessible_child with index here?

I could but I think I wanted to exercise the function and so we don't have repeated atk_object_ref_accessible_child calls in the getChild... functions. I can change it if that makes more sense.

> 
> > +AccessibilityUIElement AccessibilityUIElement::parentElement()
> > +{
> > +    g_assert(m_element);
> 
> I guess we should use either ASSERT or g_assert, and stick to that.

ok.

> 
> > +
> > +double AccessibilityUIElement::intValue()
> > +{
> > +    GValue value = { 0, { { 0 } } };
> 
> I suppose { 0, } like in C does not work in C++.

ok.

> 
> > +
> > +    if (!ATK_IS_VALUE(m_element))
> > +        return 0.0f;
> > +
> > +    atk_value_get_current_value(ATK_VALUE(m_element), &value);
> > +    return g_value_get_double(&value);
> 
> How do we know the GValue has a double value? Shouldn't we do a
> G_VALUE_HOLDS_DOUBLE before?

Ok. Should I check for G_VALUE_HOLDS_INT too and return 0 if both fail?

> 
> > @@ -432,7 +434,10 @@ static void webViewWindowObjectCleared(WebKitWebView* view, WebKitWebFrame* fram
> >      assert(gLayoutTestController);
> >  
> >      gLayoutTestController->makeWindowObject(context, windowObject, &exception);
> > -    assert(!exception);
> > +    ASSERT(!exception);
> > +
> > +    accessibilityController->makeWindowObject(context, windowObject, &exception);
> > +    ASSERT(!exception);
> 
> Stupid question, where is this created? :D

Oops. Weird, it worked even without creating it! :-\ I'll fix it and make sure the results are still the same.

> 
> The patch looks pretty good to me, but I'll mark it as r- while we work out the
> last details.

Thanks a lot for the review. Sorry took a while to respond. 

> 

[Jan Alonzo]

Created attachment 31098 [details]
Updated patch based on xan's feedback

[Xan Lopez]

Comment on attachment 31098 [details]
Updated patch based on xan's feedback

OK.

[Jan Alonzo]

(In reply to comment #6)
> (From update of attachment 31098 [details] [review])
> OK.
> 

I forgot to add the Copyright Apple line to the new DRT files. ARe you OK with me adding that prior to committing?

Thanks.

[Xan Lopez]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 31098 [details] [review] [review])
> > OK.
> > 
> 
> I forgot to add the Copyright Apple line to the new DRT files. ARe you OK with
> me adding that prior to committing?
> 
> Thanks.
> 

Sure.

[Jan Alonzo]

Our non-accessibility tests seems to be crashing with this patch. I'm holding off committing this one first until I figure out what's triggering the crashes.

[Jan Alonzo]

Comment on attachment 31098 [details]
Updated patch based on xan's feedback

Clearing review for now until I can run the full testsuite with this patch. Thanks and apologies..

[Jan Alonzo]

Created attachment 34252 [details]
updated 31/07/2009

[Jan Alonzo]

Created attachment 34255 [details]
patch updated to ToT (r46888)

[Jan Alonzo]

Created attachment 34361 [details]
patch with ChangeLog

[Xan Lopez]

Comment on attachment 34361 [details]
patch with ChangeLog

> +AtkObject* webkit_accessible_get_focused_element(WebKitAccessible* accessible)
> +{
> +    if (!accessible->m_object)
> +        return 0;
> +
> +    // FIXME: update the backing store once we have one.

I'm not sure what this is.

> +AtkObject* webkit_web_frame_get_focused_accessible_element(WebKitWebFrame* frame)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);
> +
> +#if HAVE(ACCESSIBILITY)
> +    if (!AXObjectCache::accessibilityEnabled())
> +        AXObjectCache::enableAccessibility();
> +
> +    WebKitWebFramePrivate* priv = frame->priv;
> +    if (!priv->coreFrame || !priv->coreFrame->document())
> +        return NULL;
> +
> +    RenderView* root = static_cast<RenderView *>(priv->coreFrame->document()->renderer());

I think you should use the new toRenderView methods introduced recently here.


> +double AccessibilityUIElement::intValue()
> +{
> +    GValue value = { 0, };

You should use the full initializer here ( GValue value = { 0, { { 0 } } }), otherwise I think you'll get compiler warnings.

Looks good to me otherwise, please land with those things fixed.

[Jan Alonzo]

Fixed. Landed as http://trac.webkit.org/changeset/46952