Bug 34449 – [Gtk] Evaluate and create tests for all the AtkRole's implemented by WebKitGtk

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

We need to:

1. Ensure that all the AtkRole's that we believe were implemented have actually been implemented.

2. Create tests so that we don't have to go back and do #1 ever again. :-)

3. Implement any roles that we've missed.

4. Be on the lookout for extraneous objects of ATK_ROLE_PANEL which we might've missed.

[Joanmarie Diggs]

Created attachment 47873 [details]
Create testatkrole.c, add tests for the non-form roles

I'll add tests for the form roles in a separate patch to make things easier to review.

(Style Queue Bot will no-doubt spit up on the lack of space, but this is consistent with what we've done in testatk.c)

[Joanmarie Diggs]

Created attachment 47889 [details]
add tests for the form-roles

This patch adds tests for the form-roles. It requires the previous patch.

Note that this patch does *not* include ATK_ROLE_FORM itself. To do so requires a minor code change and will be done in the next patch for this bug.

[Joanmarie Diggs]

Created attachment 47893 [details]
implement ATK_ROLE_FORM (includes test) - PART 3.

Turns out that we (ok, I) spaced on implementing ATK_ROLE_FORM. This patch takes care of that and adds a new test to the set from the previous two patches. (Thus it depends on at least the first patch; it might apply cleanly without the second.)

[Joanmarie Diggs]

Comment on attachment 47889 [details]
add tests for the form-roles

Did something dumb. New patch forthcoming.

[Joanmarie Diggs]

Created attachment 47899 [details]
add tests for the form-roles - THIS IS PART 2

My apologies for catching a mistake in part 2 after attaching part 3.

[Joanmarie Diggs]

Created attachment 47902 [details]
Implement ATK_ROLE_SEPARATOR for <hr> (includes test)

Another wayward AtkRole detected.

[Joanmarie Diggs]

Created attachment 47905 [details]
Implement ATK_ROLE_SEPARATOR for <hr> (includes test) - revised

[Joanmarie Diggs]

Created attachment 47906 [details]
Implement ATK_ROLE_COMBO_BOX (includes test)

We have an open bug for the full implementation of combo boxes; this fix merely gets the objects appearing in the accessible hierarchy with the correct AtkRoles.

[Holger Freyther]

I fear the unit tests are the wrong way. You should implement WebKitTools/DumpRenderTree/gtk/Accessibility* for atk and then unskip the Accessibility tests from LayoutTests/platform/gtk/Skipped.

[Xan Lopez]

(In reply to comment #9)
> I fear the unit tests are the wrong way. You should implement
> WebKitTools/DumpRenderTree/gtk/Accessibility* for atk and then unskip the
> Accessibility tests from LayoutTests/platform/gtk/Skipped.

I don't agree. There might be some overlap between DRT and the unit tests, but DRT won't exercise thoroughly the whole ATK APIs, which is what applications will be using.

I guess you could extend DRT until you are satisfied with its coverage, but I suppose you'd need to be careful to not make it too ATK-centric and you'd still not be using the ATK APIs directly, which again is what we expose.

IMHO unit tests and DRT is an AND topic, not an OR one, but I guess I can be wrong :)

[Holger Freyther]

(In reply to comment #10)
> (In reply to comment #9)
> > I fear the unit tests are the wrong way. You should implement
> > WebKitTools/DumpRenderTree/gtk/Accessibility* for atk and then unskip the
> > Accessibility tests from LayoutTests/platform/gtk/Skipped.
> 
> I don't agree. There might be some overlap between DRT and the unit tests, but
> DRT won't exercise thoroughly the whole ATK APIs, which is what applications
> will be using.


> IMHO unit tests and DRT is an AND topic, not an OR one, but I guess I can be
> wrong.

My understanding is that we create a unit test when the specific thing can not
be tested with a LayoutTest and extending the LayoutTestController or its child
objects is too complicated.

In this case we have 73 Accessibility Layout Tests waiting for us. From a quick
glance they are testing the basics that will be tested with the Gtk+ specific unit
tests. E.g. the Layout Tests should gurantee that "#1 of Joanmarie" is not
happening ever again.

So personally I would have loved to see the implementation of the DRT methods
first and then cover everything with unit tests. Well, Joanmarie has invested
quite some time in the ATK impl and the test cases so asking for them to be
thrown away is obviously not nice as well.

Is it asked too much if we would work on the DRT support first and then land
the test cases?

[Joanmarie Diggs]

(In reply to comment #11)

> Is it asked too much if we would work on the DRT support first and then land
> the test cases?

Personally, what I think we should work on *first* is implementing the support needed by actual end users with disabilities who are trying to access WebKitGtk. (And who are now coming to us on the Orca list and bugzilla to inform us that Ubuntu has switched its Yelp to WebKit and now they can no longer access help content.)

The attached patches implement three (and technically five if you count the descendants of the combo boxes) AtkRoles that are not currently implemented and which need to be implemented so that users with disabilities can access WebKitGtk content that includes objects which (should) have those roles.

If you'd like me to separate out the roles that were implemented from the unit tests, I'd be happy to.

And once things get under control a11y-wize, I'll even be happy to help you all work on the DRT support. I think DRT support is important. Really. It's just an issue of not enough time and not enough people and way too much that still needs to be done. :-(

[Holger Freyther]

(In reply to comment #12)
> (In reply to comment #11)
> 
> > Is it asked too much if we would work on the DRT support first and then land
> > the test cases?
> 
> Personally, what I think we should work on *first* is implementing the support
> needed by actual end users with disabilities who are trying to access
> WebKitGtk. (And who are now coming to us on the Orca list and bugzilla to
> inform us that Ubuntu has switched its Yelp to WebKit and now they can no
> longer access help content.)

Let me state first you work is greatly appreciated. This argument works the other
way around as well. By not knowing about the state of the layout tests the work
that Apple has put into supporting end users with disabilities who are trying to
access the web/websites is not utilized. :)


> If you'd like me to separate out the roles that were implemented from the unit
> tests, I'd be happy to.

Well, personally I would favor this as this will allow to determine the overlap
between unit tests and LayoutTests but it is up to Xan. I will try to start with
the Accessibility controller probably my tomorrow afternoon...

[Xan Lopez]

Comment on attachment 47899 [details]
add tests for the form-roles - THIS IS PART 2

First of all, thanks a lot for working of this. No doubt we should also work in improving our DRT coverage, but I think there's no reason at all to not have our own ATK unit tests, especially if someone has gone through the effort already of writing them.

I just have a couple of general comments that I believe apply to all your patches:

- You should use g_test_add, which allows you to pass a couple of functions to setup/finalize things. This should allow you to reuse a bunch of repeated code you have all over to place. As an example you can check how it's done in testloading.c

- Do not use a timeout to bail out of the test. You can add queue an idle, run the mainloop, and in the idle callback do your stuff and then quit. Otherwise the 100ms keeps adding up and the tests would take a lot longer to run than needed.

[Diego Escalante Urrelo]

Just to leave a note that Xan and Joanmarie agreed that I should work on these tests, I'm already doing that :-)

[Joanmarie Diggs]

(In reply to comment #15)
> Just to leave a note that Xan and Joanmarie agreed that I should work on these
> tests, I'm already doing that :-)

You totally rock Diego! Thanks so much for taking this on!!

On the to-do list, we'll need to add tables. It doesn't make much sense to do this until we get bug 30895 fixed (which is the next item on my to-do list). However, once that is fixed, we'll want objects of ATK_ROLE_TABLE and ATK_ROLE_TABLE_CELL similarly tested. Since I'm absent-minded, I'm mentioning it here. :-)

[Diego Escalante Urrelo]

Created attachment 49618 [details]
testatkroles: test non-form roles

This corresponds to joanmarie's original patch.

Bug #34449

[Diego Escalante Urrelo]

Created attachment 49619 [details]
testatkroles: test form roles

Corresponds to joanmarie's second patch.

Bug #34449

[Diego Escalante Urrelo]

Created attachment 49620 [details]
testatkroles: implement ATK_ROLE_FORM

Corresponds to joanmarie's third patch

Bug #34449

[Diego Escalante Urrelo]

Created attachment 49621 [details]
testatkroles: test ATK_ROLE_FORM

Corresponds to joanmarie's third patch.

Bug #34449

[Diego Escalante Urrelo]

Created attachment 49622 [details]
testatkroles: implement ATK_ROLE_SEPARATOR

This corresponds to joanmarie's fourth patch.

Bug #34449

[Diego Escalante Urrelo]

Created attachment 49623 [details]
testatkroles: test ATK_ROLE_SEPARATOR

This corresponds to joanmarie's fourth patch.

Bug #34449

[Diego Escalante Urrelo]

Created attachment 49624 [details]
testatkroles: implement ATK_ROLE_COMBO_BOX

Corresponds to joanmarie's fifth patch.

Bug #34449

[Diego Escalante Urrelo]

Created attachment 49625 [details]
testatkroles: test ATK_ROLE_COMBO_BOX

Corresponds to joanmarie's fifth patch.

Bug #34449

[Diego Escalante Urrelo]

I just attached an insane amount of patches, with love (and git-bz).
Please take a look at them and let me know what you think.

Yes, I didn't include a changelog intentionally. Let me know if you think they are ready and I'll re-post with changelogs.

PS: I can't set joanmarie's patches as obsolete.

[Diego Escalante Urrelo]

Back to this bug...

Last time Xan agreed we were in the right path, so I think we can expand coverage with Joanmarie and have Xan review without rush since this patches doesn't risk bitrot. Right?

[Joanmarie Diggs]

(In reply to comment #26)
> Back to this bug...
> 
> Last time Xan agreed we were in the right path, so I think we can expand
> coverage with Joanmarie and have Xan review without rush since this patches
> doesn't risk bitrot. Right?

Mostly right. :-)

We need to implement combo boxes sooner rather than later. The other two roles implemented here (form and separator) also should be in place (though less rush). The tests themselves can wait longer.

[Xan Lopez]

Comment on attachment 49618 [details]
testatkroles: test non-form roles

>From 464999993ff6f20eaa76c4789cc607a5f4e9d84c Mon Sep 17 00:00:00 2001
>From: Diego Escalante Urrelo <[e-mail]>
>Date: Thu, 25 Feb 2010 17:47:56 -0500
>Subject: [PATCH] testatkroles: test non-form roles
>
>This corresponds to joanmarie's original patch.
>
>Bug #34449

We need an actual ChangeLog entry.


>+/* vim: set sw=4 ts=4 sts=4 et: */

WebKit style does not allow to have this stuff.

>+
>+#include <errno.h>
>+#include <unistd.h>

Do you actually use these?

>+static gboolean _get_child_and_test_role(AtkObject *obj, gint pos, AtkRole role)

The '*' are wrong, and there's no need at all to prefix the function with '_' since it's already static.

>+{
>+    AtkObject *child;

Wrong '*'... happens in many other places, so just go through the file again.

>+    AtkRole child_role;
>+
>+    child = atk_object_ref_accessible_child(obj, pos);
>+    g_assert(child);
>+    child_role = atk_object_get_role(child);
>+    g_assert(child_role == role);
>+
>+    g_object_unref(child);
>+
>+    return TRUE;

Seems a bit pointless to make this a gboolean returning function since a) it always returns TRUE b) you ignore it anyway?

Other than this, looks good.

[Xan Lopez]

Comment on attachment 49619 [details]
testatkroles: test form roles

Nothing to comment here othen than applying the suggested renaming of the previous patch.

[Diego Escalante Urrelo]

Created attachment 53128 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Reviewed by NOBODY (OOPS!).

        [Gtk] Evaluate and create tests for all the AtkRole's implemented by
        WebKitGtk
        https://bugs.webkit.org/show_bug.cgi?id=34449

        Add and enable the build of testatkroles to test ATK non form roles.
        * GNUmakefile.am:
        * tests/testatkroles.c: Added.
        (finish_loading):
        (atk_roles_fixture_setup):
        (atk_roles_fixture_teardown):
        (get_child_and_test_role):
        (test_webkit_atk_get_role_document_frame):
        (test_webkit_atk_get_role_heading):
        (test_webkit_atk_get_role_image):
        (test_webkit_atk_get_role_link):
        (test_webkit_atk_get_role_list_and_item):
        (test_webkit_atk_get_role_paragraph):
        (test_webkit_atk_get_role_section):
        (test_webkit_atk_get_role_table):
        (main):

[Diego Escalante Urrelo]

Created attachment 53129 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Reviewed by NOBODY (OOPS!).

        [Gtk] Evaluate and create tests for all the AtkRole's implemented by
        WebKitGtk
        https://bugs.webkit.org/show_bug.cgi?id=34449

        Expand testatkroles to test ATK form roles.

        * tests/testatkroles.c:
        (test_webkit_atk_get_role_check_box):
        (test_webkit_atk_get_role_entry):
        (test_webkit_atk_get_role_label):
        (test_webkit_atk_get_role_listbox):
        (test_webkit_atk_get_role_password_text):
        (test_webkit_atk_get_role_push_button):
        (test_webkit_atk_get_role_radio_button):
        (main):

[Diego Escalante Urrelo]

Created attachment 53130 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Reviewed by NOBODY (OOPS!).

        [Gtk] Evaluate and create tests for all the AtkRole's implemented by
        WebKitGtk
        https://bugs.webkit.org/show_bug.cgi?id=34449

        Implement ATK_ROLE_FORM.

        * accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:
        (webkit_accessible_get_role):

[Diego Escalante Urrelo]

Created attachment 53131 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Reviewed by NOBODY (OOPS!).

        [Gtk] Evaluate and create tests for all the AtkRole's implemented by
        WebKitGtk
        https://bugs.webkit.org/show_bug.cgi?id=34449

        Expand testatkroles to test ATK_ROLE_FORM.

        * tests/testatkroles.c:
        (test_webkit_atk_get_role_form):
        (main):

[Diego Escalante Urrelo]

Created attachment 53132 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Reviewed by NOBODY (OOPS!).

        [Gtk] Evaluate and create tests for all the AtkRole's implemented by
        WebKitGtk
        https://bugs.webkit.org/show_bug.cgi?id=34449

        Implement ATK_ROLE_SEPARATOR.

        * accessibility/AccessibilityRenderObject.cpp:
        (WebCore::AccessibilityRenderObject::determineAccessibilityRole):
        * accessibility/gtk/AccessibilityObjectAtk.cpp:
        (WebCore::AccessibilityObject::accessibilityPlatformIncludesObject):

[Diego Escalante Urrelo]

Created attachment 53133 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Reviewed by NOBODY (OOPS!).

        [Gtk] Evaluate and create tests for all the AtkRole's implemented by
        WebKitGtk
        https://bugs.webkit.org/show_bug.cgi?id=34449

        Expand testatkroles to test ATK_ROLE_SEPARATOR.

        * tests/testatkroles.c:
        (test_webkit_atk_get_role_separator):
        (main):

[Diego Escalante Urrelo]

Created attachment 53134 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Reviewed by NOBODY (OOPS!).

        [Gtk] Evaluate and create tests for all the AtkRole's implemented by
        WebKitGtk
        https://bugs.webkit.org/show_bug.cgi?id=34449

        Implement ATK_ROLE_COMBO_BOX.

        * accessibility/gtk/AccessibilityObjectAtk.cpp:
        (WebCore::AccessibilityObject::accessibilityPlatformIncludesObject):
        * accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:
        (atkRole):

[Diego Escalante Urrelo]

Created attachment 53135 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Reviewed by NOBODY (OOPS!).

        [Gtk] Evaluate and create tests for all the AtkRole's implemented by
        WebKitGtk
        https://bugs.webkit.org/show_bug.cgi?id=34449

        Expand testatkroles to test ATK_ROLE_COMBO_BOX.

        * tests/testatkroles.c:
        (test_webkit_atk_get_role_combobox):
        (main):

[Diego Escalante Urrelo]

Updated to Xan's comments. Hopefully I didn't overlook anything.

[Xan Lopez]

Comment on attachment 53128 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Looks good to me.

[Xan Lopez]

Comment on attachment 53129 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

OK.

[Xan Lopez]

Comment on attachment 53130 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

OK, but I don't think it makes any sense to split this from its test.

[Xan Lopez]

Comment on attachment 53131 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

LGTM.

[Xan Lopez]

Comment on attachment 53132 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

OK.

[Xan Lopez]

Comment on attachment 53133 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

OK, same comment than before.

[Xan Lopez]

Comment on attachment 53134 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

OK.

[Xan Lopez]

Comment on attachment 53135 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

OK, same thing.

[WebKit Commit Bot]

Comment on attachment 53128 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 53128

Committed r57815: <http://trac.webkit.org/changeset/57815>

[WebKit Commit Bot]

Comment on attachment 53129 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 53129

Committed r57874: <http://trac.webkit.org/changeset/57874>

[WebKit Commit Bot]

Comment on attachment 53130 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 53130

Committed r57902: <http://trac.webkit.org/changeset/57902>

[WebKit Commit Bot]

Comment on attachment 53131 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 53131

Committed r57973: <http://trac.webkit.org/changeset/57973>

[WebKit Commit Bot]

Comment on attachment 53132 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 53132

Committed r58037: <http://trac.webkit.org/changeset/58037>

[WebKit Commit Bot]

Comment on attachment 53133 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 53133

Committed r58083: <http://trac.webkit.org/changeset/58083>

[WebKit Commit Bot]

Comment on attachment 53134 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 53134

Committed r58094: <http://trac.webkit.org/changeset/58094>

[WebKit Commit Bot]

Comment on attachment 53135 [details]
2010-04-11  Diego Escalante Urrelo  <[e-mail]>

Clearing flags on attachment: 53135

Committed r58098: <http://trac.webkit.org/changeset/58098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.