Bug 21546 – [GTK] ATK accessibility enhancements

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

[Alp Toker]

The ATK accessibility code in trunk lags behind what was in the WebKit 1.0.1 release and users aren't having much success with it. We should merge the current accessibility code into WebKit SVN.

[Alp Toker]

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(-)

[Alp Toker]

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).

[Holger Freyther]

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.

[Holger Freyther]

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.

[Holger Freyther]

Comment on attachment 25529 [details]
Use stringValue for the AtkObject::name

Landed in r40426.

[Willie Walker]

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

[Alejandro Piñeiro]

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.

[Xan Lopez]

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.

[Xan Lopez]

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.

[Holger Freyther]

Comment on attachment 29327 [details]
getorcreate.patch

Would be cool to get the a11y tests going too...

[Holger Freyther]

Comment on attachment 29326 [details]
a11ydynamictypes.patch

> From d12ce0edaf1741fad1166f8ffe08c13aee86b7d2 Mon Sep 17 00:00:00 2001
> From: Xan Lopez <[e-mail]>
> Date: Wed, 8 Apr 2009 11:59:15 +0300
> Subject: [PATCH] 2009-04-08  Xan Lopez  <[e-mail]>
> 
>         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?!

[Xan Lopez]

(In reply to comment #11)
> (From update of attachment 29326 [details] [review])
> > From d12ce0edaf1741fad1166f8ffe08c13aee86b7d2 Mon Sep 17 00:00:00 2001
> > From: Xan Lopez <[e-mail]>
> > Date: Wed, 8 Apr 2009 11:59:15 +0300
> > Subject: [PATCH] 2009-04-08  Xan Lopez  <[e-mail]>
> > 
> >         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.

[Xan Lopez]

Created attachment 29335 [details]
accobj.patch

Move two AccessibilityObject methods to their proper file.

[Xan Lopez]

Created attachment 29336 [details]
fallbackobject.patch

Move fallback object creation to its function, as it will be used elsewhere.

[Holger Freyther]

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 ...

[Holger Freyther]

(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.

[Xan Lopez]

(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.

[Xan Lopez]

(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.

[Xan Lopez]

Created attachment 29360 [details]
refstateset.patch

Implement AtkObject::ref_state_set

[Xan Lopez]

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.

[Holger Freyther]

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?

[Holger Freyther]

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).

[Xan Lopez]

(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.

[Holger Freyther]

Comment on attachment 29360 [details]
refstateset.patch

Logic seems to be right. I have no idea if we have TODO entries for every state.

[Holger Freyther]

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.

[Xan Lopez]

Created attachment 29463 [details]
statictextrole.patch

Implement AtkText interface for objects with StaticTextRole.

[Xan Lopez]

Created attachment 29464 [details]
cleanup.patch

Small cleanups.

[Xan Lopez]

Created attachment 29465 [details]
atktextmethods.patch

Working implementations for AtkText::get_text and AtkText::get_character_count.

[Holger Freyther]

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)

[Holger Freyther]

Comment on attachment 29465 [details]
atktextmethods.patch

Looks like a improvement.

[Xan Lopez]

(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.

[Xan Lopez]

Created attachment 29497 [details]
atkactioninterface.patch

Only implement AtkAction when we actually have an action.

[Xan Lopez]

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.

[Xan Lopez]

Created attachment 29530 [details]
atkimageiface.patch

Implement AtkImage interface.

[Xan Lopez]

Created attachment 29536 [details]
covermoreroles.patch

Cover more WebCore role -> ATK role conversions.

[Xan Lopez]

(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?

[Xan Lopez]

Created attachment 29600 [details]
getters.patch

Do not call setters in the getters functions.

[Xan Lopez]

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.

[Xan Lopez]

Created attachment 29613 [details]
focus.patch

The focus patch, with style fixed.

[Gustavo Noronha (kov)]

Comment on attachment 29497 [details]
atkactioninterface.patch

Looks good to me.

[Gustavo Noronha (kov)]

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.

[Joanmarie Diggs]

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!

[Xan Lopez]

(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.

[Xan Lopez]

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.

[Joanmarie Diggs]

(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!

[Xan Lopez]

(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.

[Gustavo Noronha (kov)]

Comment on attachment 29530 [details]
atkimageiface.patch

r=me

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

Comment on attachment 29600 [details]
getters.patch

Getters which change stuff is the kind of thing bdash likes, isn't it? =D /me hides

[Gustavo Noronha (kov)]

Comment on attachment 29613 [details]
focus.patch

Interesting. Are Mac, Win, GTK+ the only ports implementing a11y up to now?

[Gustavo Noronha (kov)]

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.

[Xan Lopez]

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.

[Gustavo Noronha (kov)]

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.

[Xan Lopez]

(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!