Bug 21546 - [GTK] ATK accessibility enhancements
: [GTK] ATK accessibility enhancements
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: Gtk
: 16495
: 25413 25414 25531
  Show dependency treegraph
 
Reported: 2008-10-11 17:00 PST by
Modified: 2009-05-20 10:01 PST (History)


Attachments
Current ATK changes (50.58 KB, patch)
2008-10-11 17:09 PST, Alp Toker
no flags Review Patch | Details | Formatted Diff | Diff
Use stringValue for the AtkObject::name (1.23 KB, patch)
2008-11-26 13:20 PST, Holger Freyther
no flags Review Patch | Details | Formatted Diff | Diff
Create separate objects implemeting separate interfaces (8.63 KB, patch)
2008-11-26 13:56 PST, Holger Freyther
no flags Review Patch | Details | Formatted Diff | Diff
Update the first patch (53.24 KB, patch)
2009-03-31 08:08 PST, Alejandro Piñeiro
no flags Review Patch | Details | Formatted Diff | Diff
a11ydynamictypes.patch (11.64 KB, patch)
2009-04-08 02:12 PST, Xan Lopez
zecke: review+
Review Patch | Details | Formatted Diff | Diff
getorcreate.patch (2.02 KB, patch)
2009-04-08 02:26 PST, Xan Lopez
zecke: review+
Review Patch | Details | Formatted Diff | Diff
accobj.patch (3.45 KB, patch)
2009-04-08 07:16 PST, Xan Lopez
zecke: review+
Review Patch | Details | Formatted Diff | Diff
fallbackobject.patch (3.00 KB, patch)
2009-04-08 08:03 PST, Xan Lopez
zecke: review+
Review Patch | Details | Formatted Diff | Diff
refstateset.patch (4.42 KB, patch)
2009-04-09 02:45 PST, Xan Lopez
zecke: review+
Review Patch | Details | Formatted Diff | Diff
componentiface.patch (6.61 KB, patch)
2009-04-09 03:20 PST, Xan Lopez
no flags Review Patch | Details | Formatted Diff | Diff
statictextrole.patch (1.99 KB, patch)
2009-04-14 06:35 PST, Xan Lopez
no flags Review Patch | Details | Formatted Diff | Diff
cleanup.patch (5.00 KB, patch)
2009-04-14 06:35 PST, Xan Lopez
zecke: review+
Review Patch | Details | Formatted Diff | Diff
atktextmethods.patch (3.28 KB, patch)
2009-04-14 06:36 PST, Xan Lopez
zecke: review+
Review Patch | Details | Formatted Diff | Diff
atkactioninterface.patch (2.00 KB, patch)
2009-04-15 03:24 PST, Xan Lopez
no flags Review Patch | Details | Formatted Diff | Diff
atkcomponentifacev2.patch (8.60 KB, patch)
2009-04-15 09:44 PST, Xan Lopez
no flags Review Patch | Details | Formatted Diff | Diff
atkimageiface.patch (4.06 KB, patch)
2009-04-16 01:05 PST, Xan Lopez
gns: review+
Review Patch | Details | Formatted Diff | Diff
covermoreroles.patch (5.03 KB, patch)
2009-04-16 06:10 PST, Xan Lopez
gns: review+
Review Patch | Details | Formatted Diff | Diff
getters.patch (2.49 KB, patch)
2009-04-18 00:57 PST, Xan Lopez
gns: review+
Review Patch | Details | Formatted Diff | Diff
focus.patch (6.66 KB, patch)
2009-04-20 05:42 PST, Xan Lopez
no flags Review Patch | Details | Formatted Diff | Diff
focus.patch (6.66 KB, patch)
2009-04-20 05:44 PST, Xan Lopez
no flags Review Patch | Details | Formatted Diff | Diff
focussignals.patch (5.42 KB, patch)
2009-05-20 08:41 PST, Xan Lopez
gns: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-10-11 17:00:47 PST
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.
------- Comment #1 From 2008-10-11 17:09:11 PST -------
Created an attachment (id=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(-)
------- Comment #2 From 2008-10-11 17:17:28 PST -------
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).
------- Comment #3 From 2008-11-26 13:20:48 PST -------
Created an attachment (id=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.
------- Comment #4 From 2008-11-26 13:56:08 PST -------
Created an attachment (id=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 #5 From 2009-01-30 15:32:37 PST -------
(From update of attachment 25529 [details])
Landed in r40426.
------- Comment #6 From 2009-02-05 08:21:54 PST -------
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
------- Comment #7 From 2009-03-31 08:08:45 PST -------
Created an attachment (id=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.
------- Comment #8 From 2009-04-08 02:12:00 PST -------
Created an attachment (id=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.
------- Comment #9 From 2009-04-08 02:26:55 PST -------
Created an attachment (id=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 #10 From 2009-04-08 02:56:25 PST -------
(From update of attachment 29327 [details])
Would be cool to get the a11y tests going too...
------- Comment #11 From 2009-04-08 03:04:48 PST -------
(From update of attachment 29326 [details])
> 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?!
------- Comment #12 From 2009-04-08 03:17:16 PST -------
(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.
------- Comment #13 From 2009-04-08 07:16:48 PST -------
Created an attachment (id=29335) [details]
accobj.patch

Move two AccessibilityObject methods to their proper file.
------- Comment #14 From 2009-04-08 08:03:20 PST -------
Created an attachment (id=29336) [details]
fallbackobject.patch

Move fallback object creation to its function, as it will be used elsewhere.
------- Comment #15 From 2009-04-08 09:23:22 PST -------
(From update of attachment 29335 [details])
Maybe you should say based on work of Alp Toker? Or is this ChangeLog entry coming from him? Created by Alp Toker, Landed by ...
------- Comment #16 From 2009-04-08 09:28:19 PST -------
(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.
------- Comment #17 From 2009-04-08 09:29:24 PST -------
(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.
------- Comment #18 From 2009-04-08 09:31:20 PST -------
(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.
------- Comment #19 From 2009-04-09 02:45:53 PST -------
Created an attachment (id=29360) [details]
refstateset.patch

Implement AtkObject::ref_state_set
------- Comment #20 From 2009-04-09 03:20:26 PST -------
Created an attachment (id=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 #21 From 2009-04-09 03:41:57 PST -------
(From update of attachment 29326 [details])

> +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 #22 From 2009-04-09 03:44:07 PST -------
(From update of attachment 29336 [details])
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).
------- Comment #23 From 2009-04-09 03:45:48 PST -------
(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 #24 From 2009-04-09 03:47:05 PST -------
(From update of attachment 29360 [details])
Logic seems to be right. I have no idea if we have TODO entries for every state.
------- Comment #25 From 2009-04-09 04:06:16 PST -------
(From update of attachment 29361 [details])

> +#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.
------- Comment #26 From 2009-04-14 06:35:16 PST -------
Created an attachment (id=29463) [details]
statictextrole.patch

Implement AtkText interface for objects with StaticTextRole.
------- Comment #27 From 2009-04-14 06:35:37 PST -------
Created an attachment (id=29464) [details]
cleanup.patch

Small cleanups.
------- Comment #28 From 2009-04-14 06:36:07 PST -------
Created an attachment (id=29465) [details]
atktextmethods.patch

Working implementations for AtkText::get_text and AtkText::get_character_count.
------- Comment #29 From 2009-04-14 07:39:50 PST -------
(From update of attachment 29463 [details])
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 #30 From 2009-04-14 07:44:16 PST -------
(From update of attachment 29465 [details])
Looks like a improvement.
------- Comment #31 From 2009-04-14 08:51:14 PST -------
(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.
------- Comment #32 From 2009-04-15 03:24:01 PST -------
Created an attachment (id=29497) [details]
atkactioninterface.patch

Only implement AtkAction when we actually have an action.
------- Comment #33 From 2009-04-15 09:44:26 PST -------
Created an attachment (id=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.
------- Comment #34 From 2009-04-16 01:05:01 PST -------
Created an attachment (id=29530) [details]
atkimageiface.patch

Implement AtkImage interface.
------- Comment #35 From 2009-04-16 06:10:35 PST -------
Created an attachment (id=29536) [details]
covermoreroles.patch

Cover more WebCore role -> ATK role conversions.
------- Comment #36 From 2009-04-18 00:23:18 PST -------
(In reply to comment #3)
> Created an attachment (id=25529) [details] [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?
------- Comment #37 From 2009-04-18 00:57:03 PST -------
Created an attachment (id=29600) [details]
getters.patch

Do not call setters in the getters functions.
------- Comment #38 From 2009-04-20 05:42:45 PST -------
Created an attachment (id=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.
------- Comment #39 From 2009-04-20 05:44:32 PST -------
Created an attachment (id=29613) [details]
focus.patch

The focus patch, with style fixed.
------- Comment #40 From 2009-04-20 11:27:48 PST -------
(From update of attachment 29497 [details])
Looks good to me.
------- Comment #41 From 2009-04-20 11:58:12 PST -------
(From update of attachment 29500 [details])
>         * 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.
------- Comment #42 From 2009-04-24 14:37:25 PST -------
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!
------- Comment #43 From 2009-04-24 23:56:38 PST -------
(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.
------- Comment #44 From 2009-04-24 23:58:05 PST -------
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.
------- Comment #45 From 2009-04-25 17:32:08 PST -------
(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!
------- Comment #46 From 2009-04-26 00:24:50 PST -------
(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 #47 From 2009-04-27 14:26:08 PST -------
(From update of attachment 29530 [details])
r=me
------- Comment #48 From 2009-04-27 14:34:07 PST -------
(From update of attachment 29536 [details])
> +    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 #49 From 2009-04-27 14:35:52 PST -------
(From update of attachment 29600 [details])
Getters which change stuff is the kind of thing bdash likes, isn't it? =D /me hides
------- Comment #50 From 2009-04-27 14:40:41 PST -------
(From update of attachment 29613 [details])
Interesting. Are Mac, Win, GTK+ the only ports implementing a11y up to now?
------- Comment #51 From 2009-04-27 15:10:26 PST -------
(From update of attachment 29613 [details])
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.
------- Comment #52 From 2009-05-20 08:41:40 PST -------
Created an attachment (id=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 #53 From 2009-05-20 08:55:30 PST -------
(From update of attachment 30510 [details])
> @@ -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.
------- Comment #54 From 2009-05-20 10:01:14 PST -------
(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!