Bug 34449

Summary: [Gtk] Evaluate and create tests for all the AtkRole's implemented by WebKitGtk
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apinheiro, commit-queue, diegoe, walker.willie, xan.lopez, zecke
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 25531    
Attachments:
Description Flags
Create testatkrole.c, add tests for the non-form roles
none
add tests for the form-roles
none
implement ATK_ROLE_FORM (includes test) - PART 3.
none
add tests for the form-roles - THIS IS PART 2
xan.lopez: review-
Implement ATK_ROLE_SEPARATOR for <hr> (includes test)
none
Implement ATK_ROLE_SEPARATOR for <hr> (includes test) - revised
none
Implement ATK_ROLE_COMBO_BOX (includes test)
none
testatkroles: test non-form roles
xan.lopez: review-
testatkroles: test form roles
none
testatkroles: implement ATK_ROLE_FORM
none
testatkroles: test ATK_ROLE_FORM
none
testatkroles: implement ATK_ROLE_SEPARATOR
none
testatkroles: test ATK_ROLE_SEPARATOR
none
testatkroles: implement ATK_ROLE_COMBO_BOX
none
testatkroles: test ATK_ROLE_COMBO_BOX
none
2010-04-11 Diego Escalante Urrelo <descalante@igalia.com>
none
2010-04-11 Diego Escalante Urrelo <descalante@igalia.com>
none
2010-04-11 Diego Escalante Urrelo <descalante@igalia.com>
none
2010-04-11 Diego Escalante Urrelo <descalante@igalia.com>
none
2010-04-11 Diego Escalante Urrelo <descalante@igalia.com>
none
2010-04-11 Diego Escalante Urrelo <descalante@igalia.com>
none
2010-04-11 Diego Escalante Urrelo <descalante@igalia.com>
none
2010-04-11 Diego Escalante Urrelo <descalante@igalia.com> none

Description Joanmarie Diggs 2010-02-01 14:28:39 PST
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.
Comment 1 Joanmarie Diggs 2010-02-01 15:02:15 PST
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)
Comment 2 Joanmarie Diggs 2010-02-01 17:42:38 PST
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.
Comment 3 Joanmarie Diggs 2010-02-01 18:05:02 PST
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.)
Comment 4 Joanmarie Diggs 2010-02-01 18:33:38 PST
Comment on attachment 47889 [details]
add tests for the form-roles

Did something dumb. New patch forthcoming.
Comment 5 Joanmarie Diggs 2010-02-01 18:51:22 PST
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.
Comment 6 Joanmarie Diggs 2010-02-01 20:01:34 PST
Created attachment 47902 [details]
Implement ATK_ROLE_SEPARATOR for <hr> (includes test)

Another wayward AtkRole detected.
Comment 7 Joanmarie Diggs 2010-02-01 21:10:09 PST
Created attachment 47905 [details]
Implement ATK_ROLE_SEPARATOR for <hr> (includes test) - revised
Comment 8 Joanmarie Diggs 2010-02-01 21:42:41 PST
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.
Comment 9 Holger Freyther 2010-02-02 00:12:07 PST
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.
Comment 10 Xan Lopez 2010-02-02 01:01:11 PST
(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 :)
Comment 11 Holger Freyther 2010-02-02 01:17:59 PST
(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?
Comment 12 Joanmarie Diggs 2010-02-02 03:04:19 PST
(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. :-(
Comment 13 Holger Freyther 2010-02-02 06:16:53 PST
(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...
Comment 14 Xan Lopez 2010-02-02 12:59:47 PST
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.
Comment 15 Diego Escalante Urrelo 2010-02-25 00:28:07 PST
Just to leave a note that Xan and Joanmarie agreed that I should work on these tests, I'm already doing that :-)
Comment 16 Joanmarie Diggs 2010-02-26 00:10:03 PST
(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. :-)
Comment 17 Diego Escalante Urrelo 2010-02-26 13:02:27 PST
Created attachment 49618 [details]
testatkroles: test non-form roles

This corresponds to joanmarie's original patch.

Bug #34449
Comment 18 Diego Escalante Urrelo 2010-02-26 13:02:31 PST
Created attachment 49619 [details]
testatkroles: test form roles

Corresponds to joanmarie's second patch.

Bug #34449
Comment 19 Diego Escalante Urrelo 2010-02-26 13:02:36 PST
Created attachment 49620 [details]
testatkroles: implement ATK_ROLE_FORM

Corresponds to joanmarie's third patch

Bug #34449
Comment 20 Diego Escalante Urrelo 2010-02-26 13:02:40 PST
Created attachment 49621 [details]
testatkroles: test ATK_ROLE_FORM

Corresponds to joanmarie's third patch.

Bug #34449
Comment 21 Diego Escalante Urrelo 2010-02-26 13:02:45 PST
Created attachment 49622 [details]
testatkroles: implement ATK_ROLE_SEPARATOR

This corresponds to joanmarie's fourth patch.

Bug #34449
Comment 22 Diego Escalante Urrelo 2010-02-26 13:02:49 PST
Created attachment 49623 [details]
testatkroles: test ATK_ROLE_SEPARATOR

This corresponds to joanmarie's fourth patch.

Bug #34449
Comment 23 Diego Escalante Urrelo 2010-02-26 13:02:54 PST
Created attachment 49624 [details]
testatkroles: implement ATK_ROLE_COMBO_BOX

Corresponds to joanmarie's fifth patch.

Bug #34449
Comment 24 Diego Escalante Urrelo 2010-02-26 13:02:59 PST
Created attachment 49625 [details]
testatkroles: test ATK_ROLE_COMBO_BOX

Corresponds to joanmarie's fifth patch.

Bug #34449
Comment 25 Diego Escalante Urrelo 2010-02-26 13:10:23 PST
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.
Comment 26 Diego Escalante Urrelo 2010-03-29 19:01:17 PDT
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?
Comment 27 Joanmarie Diggs 2010-03-29 19:16:54 PDT
(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.
Comment 28 Xan Lopez 2010-04-07 09:07:15 PDT
Comment on attachment 49618 [details]
testatkroles: test non-form roles

>From 464999993ff6f20eaa76c4789cc607a5f4e9d84c Mon Sep 17 00:00:00 2001
>From: Diego Escalante Urrelo <descalante@igalia.com>
>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.
Comment 29 Xan Lopez 2010-04-07 09:09:46 PDT
Comment on attachment 49619 [details]
testatkroles: test form roles

Nothing to comment here othen than applying the suggested renaming of the previous patch.
Comment 30 Diego Escalante Urrelo 2010-04-11 19:44:13 PDT
Created attachment 53128 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

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):
Comment 31 Diego Escalante Urrelo 2010-04-11 19:44:30 PDT
Created attachment 53129 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

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):
Comment 32 Diego Escalante Urrelo 2010-04-11 19:44:40 PDT
Created attachment 53130 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

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):
Comment 33 Diego Escalante Urrelo 2010-04-11 19:44:48 PDT
Created attachment 53131 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

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):
Comment 34 Diego Escalante Urrelo 2010-04-11 19:44:59 PDT
Created attachment 53132 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

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):
Comment 35 Diego Escalante Urrelo 2010-04-11 19:46:11 PDT
Created attachment 53133 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

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):
Comment 36 Diego Escalante Urrelo 2010-04-11 19:46:24 PDT
Created attachment 53134 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

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):
Comment 37 Diego Escalante Urrelo 2010-04-11 19:46:35 PDT
Created attachment 53135 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

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):
Comment 38 Diego Escalante Urrelo 2010-04-11 19:48:42 PDT
Updated to Xan's comments. Hopefully I didn't overlook anything.
Comment 39 Xan Lopez 2010-04-19 08:29:29 PDT
Comment on attachment 53128 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Looks good to me.
Comment 40 Xan Lopez 2010-04-19 08:38:02 PDT
Comment on attachment 53129 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

OK.
Comment 41 Xan Lopez 2010-04-19 08:40:03 PDT
Comment on attachment 53130 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

OK, but I don't think it makes any sense to split this from its test.
Comment 42 Xan Lopez 2010-04-19 08:40:37 PDT
Comment on attachment 53131 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

LGTM.
Comment 43 Xan Lopez 2010-04-19 08:41:17 PDT
Comment on attachment 53132 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

OK.
Comment 44 Xan Lopez 2010-04-19 08:41:40 PDT
Comment on attachment 53133 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

OK, same comment than before.
Comment 45 Xan Lopez 2010-04-19 08:42:11 PDT
Comment on attachment 53134 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

OK.
Comment 46 Xan Lopez 2010-04-19 08:42:39 PDT
Comment on attachment 53135 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

OK, same thing.
Comment 47 WebKit Commit Bot 2010-04-19 10:38:20 PDT
Comment on attachment 53128 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 53128

Committed r57815: <http://trac.webkit.org/changeset/57815>
Comment 48 WebKit Commit Bot 2010-04-19 23:40:06 PDT
Comment on attachment 53129 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 53129

Committed r57874: <http://trac.webkit.org/changeset/57874>
Comment 49 WebKit Commit Bot 2010-04-20 12:08:24 PDT
Comment on attachment 53130 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 53130

Committed r57902: <http://trac.webkit.org/changeset/57902>
Comment 50 WebKit Commit Bot 2010-04-21 06:18:06 PDT
Comment on attachment 53131 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 53131

Committed r57973: <http://trac.webkit.org/changeset/57973>
Comment 51 WebKit Commit Bot 2010-04-21 18:46:43 PDT
Comment on attachment 53132 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 53132

Committed r58037: <http://trac.webkit.org/changeset/58037>
Comment 52 WebKit Commit Bot 2010-04-22 03:31:43 PDT
Comment on attachment 53133 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 53133

Committed r58083: <http://trac.webkit.org/changeset/58083>
Comment 53 WebKit Commit Bot 2010-04-22 07:36:28 PDT
Comment on attachment 53134 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 53134

Committed r58094: <http://trac.webkit.org/changeset/58094>
Comment 54 WebKit Commit Bot 2010-04-22 08:46:29 PDT
Comment on attachment 53135 [details]
2010-04-11  Diego Escalante Urrelo  <descalante@igalia.com>

Clearing flags on attachment: 53135

Committed r58098: <http://trac.webkit.org/changeset/58098>
Comment 55 WebKit Commit Bot 2010-04-22 08:46:39 PDT
All reviewed patches have been landed.  Closing bug.