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.
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)
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.
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.)
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.
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.
(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 :)
(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?
(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. :-(
(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 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.
(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. :-)
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.
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?
(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 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 on attachment 49619[details]
testatkroles: test form roles
Nothing to comment here othen than applying the suggested renaming of the previous patch.
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):
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):
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):
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):
2010-02-01 15:02 PST, Joanmarie Diggs
2010-02-01 17:42 PST, Joanmarie Diggs
2010-02-01 18:05 PST, Joanmarie Diggs
2010-02-01 18:51 PST, Joanmarie Diggs
2010-02-01 20:01 PST, Joanmarie Diggs
2010-02-01 21:10 PST, Joanmarie Diggs
2010-02-01 21:42 PST, Joanmarie Diggs
2010-02-26 13:02 PST, Diego Escalante Urrelo
2010-02-26 13:02 PST, Diego Escalante Urrelo
2010-02-26 13:02 PST, Diego Escalante Urrelo
2010-02-26 13:02 PST, Diego Escalante Urrelo
2010-02-26 13:02 PST, Diego Escalante Urrelo
2010-02-26 13:02 PST, Diego Escalante Urrelo
2010-02-26 13:02 PST, Diego Escalante Urrelo
2010-02-26 13:02 PST, Diego Escalante Urrelo
2010-04-11 19:44 PDT, Diego Escalante Urrelo
2010-04-11 19:44 PDT, Diego Escalante Urrelo
2010-04-11 19:44 PDT, Diego Escalante Urrelo
2010-04-11 19:44 PDT, Diego Escalante Urrelo
2010-04-11 19:44 PDT, Diego Escalante Urrelo
2010-04-11 19:46 PDT, Diego Escalante Urrelo
2010-04-11 19:46 PDT, Diego Escalante Urrelo
2010-04-11 19:46 PDT, Diego Escalante Urrelo