Bug 21546

Created attachment 24294 [details] Current ATK changes This is the WIP accessibility code (aiming to have it ready for WebKit 1.0.2). Things that still need work: * Text elements need to be hidden and folded into the parent element instead of being listed individually as they are now. * Need support for replaced objects. * Need to support more accessible relations. * Support for more DOM events. * Conversion of text formatting attributes to and from ATK styles. * Instantiate accessible types with only the interfaces they implement. dom/Document.cpp | 2 page/gtk/AXObjectCacheAtk.cpp | 73 ++ page/gtk/AccessibilityObjectAtk.cpp | 22 page/gtk/AccessibilityObjectWrapperAtk.cpp | 1025 +++++++++++++++++++++++++---- 4 files changed, 996 insertions(+), 126 deletions(-)

For anyone familiar with the Mozilla accessibility implementation wondering where most of the DOM nodes have gone, I should mention that we now do hiding of redundant nodes. The aim is to represent the functionality of the page rather than its hypertext structure. Hiding is currently broken for text nodes (as I mentioned earlier).

Created attachment 25529 [details] Use stringValue for the AtkObject::name Minor change. According to the documentation atk_get_object_name may/should return the textual content. Do that.

Created attachment 25534 [details] Create separate objects implemeting separate interfaces This is mostly here to start discussing. The problem: - AccessibilityRenderObject::textLength and selection handling is only valid for Text-Controls. When using a debug build one is running into asserts when using tools like accerciser. First approach: - Create separate GType's and not everyone is implementing AtkTextInterface (and implemented with this patch) to not run into the asserts. Next steps: - Implement AtkTextInterface on a paragraph of text around the ::stringValue - Some how dynamically add AtkTextInterface and other implementations of a GObject* or construct these types dynamically... I will try to push your other patches forward as well.

Comment on attachment 25529 [details] Use stringValue for the AtkObject::name Landed in r40426.

Many thanks! We hope to try to test this when our schedule eases up a little. In the meantime, have you looked at things in Accerciser from both the hierarchy perspective as well as the event perspective? A side-by-side comparison of the same web content shown in WebKit and Firefox might yield a lot of insight. In addition, there's also Speclenium, which might be useful: http://monotonous.org/specular/ http://www.youtube.com/watch?v=UHTd8c8jY8Y

Created attachment 29115 [details] Update the first patch When I tried to use the patch at the comment 1, this doesn't apply directly, and in the same way, it doesn't compile, so I needed to fix these problems. I suppose that it was a experimental patch. I also made the minor change suggested at comment 3. I don't apply the changes at comment 4 as they are mostly structural, but almost with no additional functionality. In the same way I made some minor changes: * I commented some parts of code, to avoid "not used definition" compile warnings * I change the call to AXObjectCache->get for AXObjectCache->getOrCreate on some places. The first one was on webkitwebview.cpp, when you try to get the AccessibleObject you are wrapping. I needed to do that because the call to ->get returned NULL, so no DOM tree was showed on Accerciser. I needed to do that as well in other places, in order to avoid crashes. * As far as I see, the only place where the cache is updated is on getOrCreate. In the same way, I think that has sense that a cache should work always in this way, checking if you have the object yet, and if not, create the object, so getOrCreate should be the most called function. Probably I'm wrong, but in this case, we should add NULL checks on the ->get return, as can cause crashes. * About the functionality, currently the position is not computed properly. You can see it easily with the Accerciser, and see that the html objects are not positionated properly (although it seems that computes properly the size). The AtkComponent implementation were added with the patch at comment 1, so probably it needs to be reviewed. I don't know if this affect to Orca or other screen readers, but should be reviewed. * I experiment other crashes, but I can't conclude if was again for the get/getOrCreate problem yet.

Created attachment 29326 [details] a11ydynamictypes.patch After talking with Holger, we decided the best idea to fix the types issue was to follow Mozilla and generate the right types at runtime based on the interfaces they should implement. This patch does this, based on the implementation in nsAccessibleWrap.cpp in Mozilla. It does not really improve the situation too much, but I think it's a reasonable first step to clean up the code. Next I'll try to land Alp's patch in pieces.

Created attachment 29327 [details] getorcreate.patch Push one-liner by Alejandro Piñeiro to actually create the wrapper object the first time we need it. With this we now get some stuff on Accerciser.

Comment on attachment 29327 [details] getorcreate.patch Would be cool to get the a11y tests going too...

Comment on attachment 29326 [details] a11ydynamictypes.patch > From d12ce0edaf1741fad1166f8ffe08c13aee86b7d2 Mon Sep 17 00:00:00 2001 > From: Xan Lopez <xlopez@igalia.com> > Date: Wed, 8 Apr 2009 11:59:15 +0300 > Subject: [PATCH] 2009-04-08 Xan Lopez <xlopez@igalia.com> > > Reviewed by NOBODY (OOPS!). > > https://bugs.webkit.org/show_bug.cgi?id=21546 > [GTK] ATK accessibility enhancements > > Rework accessibility type generation code, based on Mozilla a11y > implementation. If it is based on Mozilla a11y we should include their copyright header and maybe even keep the MPL header for these parts?!

(In reply to comment #11) > (From update of attachment 29326 [details] [review]) > > From d12ce0edaf1741fad1166f8ffe08c13aee86b7d2 Mon Sep 17 00:00:00 2001 > > From: Xan Lopez <xlopez@igalia.com> > > Date: Wed, 8 Apr 2009 11:59:15 +0300 > > Subject: [PATCH] 2009-04-08 Xan Lopez <xlopez@igalia.com> > > > > Reviewed by NOBODY (OOPS!). > > > > https://bugs.webkit.org/show_bug.cgi?id=21546 > > [GTK] ATK accessibility enhancements > > > > Rework accessibility type generation code, based on Mozilla a11y > > implementation. > > If it is based on Mozilla a11y we should include their copyright header and > maybe even keep the MPL header for these parts?! > All their code is triple licensed, so we just choose to have it as LGPL 2.1. AFAIK there's no need at all to copy the triple license header. About putting their copyright header, it says "The contens of this file..." which is not true. Maybe we can add a note saying that some parts of the file are based on Mozilla code.

Created attachment 29335 [details] accobj.patch Move two AccessibilityObject methods to their proper file.

Created attachment 29336 [details] fallbackobject.patch Move fallback object creation to its function, as it will be used elsewhere.

Comment on attachment 29335 [details] accobj.patch Maybe you should say based on work of Alp Toker? Or is this ChangeLog entry coming from him? Created by Alp Toker, Landed by ...

(In reply to comment #12) > (In reply to comment #11) > All their code is triple licensed, so we just choose to have it as LGPL 2.1. > AFAIK there's no need at all to copy the triple license header. > > About putting their copyright header, it says "The contens of this file..." > which is not true. Maybe we can add a note saying that some parts of the file > are based on Mozilla code. > Right, there are some more dimensions to it. E.g. if we improve the version they can not integrate the changes into their tree. For the Gtk Drawing code we copied their file and kept the triple license... it should allow them to pick any fixes in the future. If there isn't a strong reason not doing it I would be very happy to follow the gtk drawing approach here.

(In reply to comment #15) > (From update of attachment 29335 [details] [review]) > Maybe you should say based on work of Alp Toker? Or is this ChangeLog entry > coming from him? Created by Alp Toker, Landed by ... > Well, in this case the code is 100% from Alp's patch, I just separated it in a single patch, so I thought it made sense to just attribute it to him in toto. I don't really know if we have specific rules about this.

(In reply to comment #16) > (In reply to comment #12) > > (In reply to comment #11) > > > All their code is triple licensed, so we just choose to have it as LGPL 2.1. > > AFAIK there's no need at all to copy the triple license header. > > > > About putting their copyright header, it says "The contens of this file..." > > which is not true. Maybe we can add a note saying that some parts of the file > > are based on Mozilla code. > > > > Right, there are some more dimensions to it. E.g. if we improve the version > they can not integrate the changes into their tree. For the Gtk Drawing code we > copied their file and kept the triple license... it should allow them to pick > any fixes in the future. If there isn't a strong reason not doing it I would be > very happy to follow the gtk drawing approach here. > Well, dunno, this is not really the same code, I'm just using the same idea, so I don't think it's exactly the same situation than the GTK+ drawing code.

Created attachment 29360 [details] refstateset.patch Implement AtkObject::ref_state_set

Created attachment 29361 [details] componentiface.patch Implement AtkComponent interface. I've modified the webcore -> atk coordinates calculation in PLATFORM(GTK) to make the workaround more obvious.

Comment on attachment 29326 [details] a11ydynamictypes.patch > +static GType GetAtkInterfaceTypeFromWAIType(WAIType type) > +{ > + switch (type) { > + case WAI_ACTION: > + return ATK_TYPE_ACTION; > + case WAI_STREAMABLE: > + return ATK_TYPE_STREAMABLE_CONTENT; > + case WAI_EDITABLE_TEXT: > + return ATK_TYPE_EDITABLE_TEXT; > + case WAI_TEXT: > + return ATK_TYPE_TEXT; > + } > + > + return G_TYPE_INVALID; > +} style foobar... four spaces please and the case labels should be on the same height as the switch (consult the style guide I might be wrong about the switch). > +#define WAI_TYPE_NAME_LEN (30) /* Enough for prefix + 5 hex characters (max) */ > + static char name[WAI_TYPE_NAME_LEN + 1]; > + > + g_sprintf(name, "WAIType%x", interfaceMask); > + name[WAI_TYPE_NAME_LEN] = '\0'; nice improvement. I have no idea where W should come from AI == a11y here?

Comment on attachment 29336 [details] fallbackobject.patch Okay, I know Alp is the author of the patch and you are simply splitting his patch, I think it could qualify as a reason to put Alp's name into the changelog entry (even if the description is not coming from him).

(In reply to comment #21) > (From update of attachment 29326 [details] [review]) > > > +static GType GetAtkInterfaceTypeFromWAIType(WAIType type) > > +{ > > + switch (type) { > > + case WAI_ACTION: > > + return ATK_TYPE_ACTION; > > + case WAI_STREAMABLE: > > + return ATK_TYPE_STREAMABLE_CONTENT; > > + case WAI_EDITABLE_TEXT: > > + return ATK_TYPE_EDITABLE_TEXT; > > + case WAI_TEXT: > > + return ATK_TYPE_TEXT; > > + } > > + > > + return G_TYPE_INVALID; > > +} > > style foobar... four spaces please and the case labels should be on the same > height as the switch (consult the style guide I might be wrong about the > switch). > Yep, I fixed this in another patch. > > > +#define WAI_TYPE_NAME_LEN (30) /* Enough for prefix + 5 hex characters (max) */ > > + static char name[WAI_TYPE_NAME_LEN + 1]; > > + > > + g_sprintf(name, "WAIType%x", interfaceMask); > > + name[WAI_TYPE_NAME_LEN] = '\0'; > > nice improvement. I have no idea where W should come from AI == a11y here? > WAI = WebKit Accessibility Interface. Mozilla has MAI, so we get WAI ;) At first I was writing the whole thing, but it really is just too long IMHO, especially for API.

Comment on attachment 29360 [details] refstateset.patch Logic seems to be right. I have no idea if we have TODO entries for every state.

Comment on attachment 29361 [details] componentiface.patch > +#if PLATFORM(GTK) > +#include "HostWindow.h" > +#include <gtk/gtk.h> > +#endif I don't think we need this conditional. > + if (frameView) { > +#if PLATFORM(GTK) > + // FIXME: This is a workaround for confusing coordinate conversions > + // that don't work in WebCore maybe explain in which it is confusing? > + GtkWidget* widget = frameView->hostWindow()->platformWindow(); > + GdkWindow* win = widget->window; > + rect = frameView->contentsToWindow(rect); > + rect.move(widget->allocation.x, widget->allocation.y); > + > + if (coordType == ATK_XY_SCREEN) { > + gint originX, originY; > + gdk_window_get_origin(win, &originX, &originY); > + rect.move(originX, originY); > + } Let us assume Atk is only used with the Gtk+ platform for now. > > +static IntPoint contentsFromAtk(AccessibilityObject* coreObject, AtkCoordType coordType, gint x, gint y) > +{ > + IntPoint pos(x, y); > + > + FrameView* frameView = coreObject->documentFrameView(); > + if (frameView) { > + switch (coordType) { > + case ATK_XY_SCREEN: > + return frameView->screenToContents(pos); > + case ATK_XY_WINDOW: > + return frameView->windowToContents(pos); > + } So the confusion is not symmetric or is this part not yet tested? > + > switch (type) { > - case WAI_ACTION: > + case WAI_ACTION: > return ATK_TYPE_ACTION; > - case WAI_STREAMABLE: > + case WAI_STREAMABLE: > return ATK_TYPE_STREAMABLE_CONTENT; > - case WAI_EDITABLE_TEXT: > + case WAI_EDITABLE_TEXT: > return ATK_TYPE_EDITABLE_TEXT; > - case WAI_TEXT: > + case WAI_TEXT: > return ATK_TYPE_TEXT; > + case WAI_COMPONENT: > + return ATK_TYPE_COMPONENT; > } please ix this when landing the dynamic types patch.

Created attachment 29463 [details] statictextrole.patch Implement AtkText interface for objects with StaticTextRole.

Created attachment 29464 [details] cleanup.patch Small cleanups.

Created attachment 29465 [details] atktextmethods.patch Working implementations for AtkText::get_text and AtkText::get_character_count.

Comment on attachment 29463 [details] statictextrole.patch Did you test this? Could you do a debug build before landing this and check accerciser is not causing us to hit an assert? But it looks good (so r=+ if we don't hit an assert)

Comment on attachment 29465 [details] atktextmethods.patch Looks like a improvement.

(In reply to comment #29) > (From update of attachment 29463 [details] [review]) > Did you test this? Could you do a debug build before landing this and check > accerciser is not causing us to hit an assert? But it looks good (so r=+ if we > don't hit an assert) > Does not hit any ASSERT in a debug build.

Created attachment 29497 [details] atkactioninterface.patch Only implement AtkAction when we actually have an action.

Created attachment 29500 [details] atkcomponentifacev2.patch Implement AtkComponent interface. I have moved the screenToWindow windowToScreen code to ChromeClient, where it belongs, so there are no hacks in the accesibility wrapper now. The coordinates seem to be OK in Accerciser, both relative and absolute.

Created attachment 29530 [details] atkimageiface.patch Implement AtkImage interface.

Created attachment 29536 [details] covermoreroles.patch Cover more WebCore role -> ATK role conversions.

(In reply to comment #3) > Created an attachment (id=25529) [review] > Use stringValue for the AtkObject::name > > Minor change. According to the documentation atk_get_object_name may/should > return the textual content. Do that. > Looking at the implementation of stringValue() it does not seem to me that what it returns is guaranteed to be always brief or short. Are you sure this change is OK?

Created attachment 29600 [details] getters.patch Do not call setters in the getters functions.

Created attachment 29612 [details] focus.patch Implement AtkObject::focus-event and AtkObject::state-changed:focused signal emission. Couldn't figure out a way to do it without changing handleFocusedUIElementChanged to get as parameters the old and new focused nodes (if they exist). The parameters are unused in Mac and Windows.

Created attachment 29613 [details] focus.patch The focus patch, with style fixed.

Comment on attachment 29497 [details] atkactioninterface.patch Looks good to me.

Comment on attachment 29500 [details] atkcomponentifacev2.patch > * page/gtk/AccessibilityObjectWrapperAtk.cpp: > (core): > [...] > > * WebCoreSupport/ChromeClientGtk.cpp: > (WebKit::widgetScreenPosition): > (WebKit::ChromeClient::windowToScreen): > (WebKit::ChromeClient::screenToWindow): This looks good to me, too. I would split these two parts, though. I understand the WebKit/gtk changes are more general, and the code is used, for instance, in scroll view, so I'd prefer if you make two commits here.

Looking at the ChangeLog, I'm assuming that the above patches (with the exception of the patch referred to in comment #5) have not been committed to trunk. If so, and given that I've set aside time this weekend to start implementing support in Orca for WebKit, which of the above patches should I be applying? :-) Thanks!

(In reply to comment #42) > Looking at the ChangeLog, I'm assuming that the above patches (with the > exception of the patch referred to in comment #5) have not been committed to > trunk. > > If so, and given that I've set aside time this weekend to start implementing > support in Orca for WebKit, which of the above patches should I be applying? > :-) > > Thanks! > All of them! :) The last one is the most important though, since it implements the focus tracking (the state-changed::focus stuff we went through in the hackfest). The same thing applies for the three patches in https://bugs.webkit.org/show_bug.cgi?id=16135, all of them should be applied.

Mmm, maybe I was not clear enough. What needs to be committed is everything that has 'xan.lopez: review?' in the Flags column. Ie, from 'atkimageiface.patch' onwards.

(In reply to comment #44) > Mmm, maybe I was not clear enough. What needs to be committed is everything > that has 'xan.lopez: review?' in the Flags column. Ie, from > 'atkimageiface.patch' onwards. > Awesome. That's what I thought, but with so many patches I wanted to be sure. :-) Things are coming along quite nicely. (Thanks!!) But there are things that ultimately be implemented (e.g. exposing the levels of headings, exposing list items with ROLE_LIST_ITEM, some expected states missing from accessibles). What's the best way to proceed on filing these? My opinion is that if we keep tacking on issues here, the list of patches will continue to grow and this bug will never get closed. :-) If we open new bugs now, it might be confusing because of the dependency of getting the patches from this bug (and possibly bug #16135 and possibly some of the newly opened bugs). So *I* think it makes sense to wait until these patches are committed and then open new bugs. But I'll do whatever you all think is best. Thanks!

(In reply to comment #45) > My opinion is that if we keep tacking on issues here, the list of patches will > continue to grow and this bug will never get closed. :-) If we open new bugs > now, it might be confusing because of the dependency of getting the patches > from this bug (and possibly bug #16135 and possibly some of the newly opened > bugs). So *I* think it makes sense to wait until these patches are committed > and then open new bugs. But I'll do whatever you all think is best. I hope we'll be able to clear the patch list in this bug pretty soon, so I don't think there's a problem in opening new bugs while this one is open. At worst I can choose not do any work on the new ones until this is done ;) Thanks for the testing, and please make sure to open bugs for any small detail you see, the more the better.

Comment on attachment 29530 [details] atkimageiface.patch r=me

Comment on attachment 29536 [details] covermoreroles.patch > + case ColumnRole: > + //return ATK_ROLE_TABLE_COLUMN_HEADER; //? > + return ATK_ROLE_UNKNOWN; // Matches Mozilla [...] > + case BusyIndicatorRole: > + return ATK_ROLE_PROGRESS_BAR; //? > + case ProgressIndicatorRole: > + //return ATK_ROLE_SPIN_BUTTON; //Some confusion here in AccessibilityRenderObject.cpp [...] > + //case LabelRole: // TODO: add LabelRole? > + // return ATK_ROLE_LABEL; The only problem I have with this patch is the comments are confusing. That TODO is about adding LabelRole to ATK, or to WebCore? If you can improve the comments, and add a small explanation to the // ? ones, that would be great.

Comment on attachment 29600 [details] getters.patch Getters which change stuff is the kind of thing bdash likes, isn't it? =D /me hides

Comment on attachment 29613 [details] focus.patch Interesting. Are Mac, Win, GTK+ the only ports implementing a11y up to now?

Comment on attachment 29613 [details] focus.patch I'm changing the review flag back to ?, because Xan wants to find out if there is another way to implement this, but it would be good to have input from someone from Apple, who knows this code.

Created attachment 30510 [details] focussignals.patch After talking with Chris Feizach from Apple we have agreed that instead of modifying the old method we should add a new one specific to the GTK port, so I'm doing that now.

Comment on attachment 30510 [details] focussignals.patch > @@ -110,6 +113,9 @@ namespace WebCore { > inline void AXObjectCache::selectedChildrenChanged(RenderObject*) { } > inline void AXObjectCache::postNotification(RenderObject*, const String&) { } > inline void AXObjectCache::postNotificationToElement(RenderObject*, const String&) { } > +#if PLATFORM(GTK) > + inline void handleFocusedUIElementChangedWithRenderers(RenderObject*, RenderObject*) { } > +#endif > #endif You're missing AXObjectCache:: here. I believe that won't build with ACCESSIBILITY disabled, but then again, is it even possible to build with no accessibility for GTK+? I think we don't need to provide that empty implementation at all. Otherwise good.

(In reply to comment #53) > (From update of attachment 30510 [details] [review]) > > @@ -110,6 +113,9 @@ namespace WebCore { > > inline void AXObjectCache::selectedChildrenChanged(RenderObject*) { } > > inline void AXObjectCache::postNotification(RenderObject*, const String&) { } > > inline void AXObjectCache::postNotificationToElement(RenderObject*, const String&) { } > > +#if PLATFORM(GTK) > > + inline void handleFocusedUIElementChangedWithRenderers(RenderObject*, RenderObject*) { } > > +#endif > > #endif > > You're missing AXObjectCache:: here. I believe that won't build with > ACCESSIBILITY disabled, but then again, is it even possible to build with no > accessibility for GTK+? I think we don't need to provide that empty > implementation at all. Otherwise good. > Committed as r43920, closing!