Bug 21546 - [GTK] ATK accessibility enhancements
Summary: [GTK] ATK accessibility enhancements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Xan Lopez
URL:
Keywords: Gtk
Depends on: 16495
Blocks: 25531 25413 25414
  Show dependency treegraph
 
Reported: 2008-10-11 17:00 PDT by Alp Toker
Modified: 2009-05-20 10:01 PDT (History)
13 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2008-10-11 17:00:47 PDT
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 Alp Toker 2008-10-11 17:09:11 PDT
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(-)
Comment 2 Alp Toker 2008-10-11 17:17:28 PDT
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 Holger Freyther 2008-11-26 13:20:48 PST
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.
Comment 4 Holger Freyther 2008-11-26 13:56:08 PST
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 5 Holger Freyther 2009-01-30 15:32:37 PST
Comment on attachment 25529 [details]
Use stringValue for the AtkObject::name

Landed in r40426.
Comment 6 Willie Walker 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 Alejandro Piñeiro 2009-03-31 08:08:45 PDT
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.
Comment 8 Xan Lopez 2009-04-08 02:12:00 PDT
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.
Comment 9 Xan Lopez 2009-04-08 02:26:55 PDT
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 10 Holger Freyther 2009-04-08 02:56:25 PDT
Comment on attachment 29327 [details]
getorcreate.patch

Would be cool to get the a11y tests going too...
Comment 11 Holger Freyther 2009-04-08 03:04:48 PDT
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?!
Comment 12 Xan Lopez 2009-04-08 03:17:16 PDT
(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 Xan Lopez 2009-04-08 07:16:48 PDT
Created attachment 29335 [details]
accobj.patch

Move two AccessibilityObject methods to their proper file.
Comment 14 Xan Lopez 2009-04-08 08:03:20 PDT
Created attachment 29336 [details]
fallbackobject.patch

Move fallback object creation to its function, as it will be used elsewhere.
Comment 15 Holger Freyther 2009-04-08 09:23:22 PDT
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 ...
Comment 16 Holger Freyther 2009-04-08 09:28:19 PDT
(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 Xan Lopez 2009-04-08 09:29:24 PDT
(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 Xan Lopez 2009-04-08 09:31:20 PDT
(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 Xan Lopez 2009-04-09 02:45:53 PDT
Created attachment 29360 [details]
refstateset.patch

Implement AtkObject::ref_state_set
Comment 20 Xan Lopez 2009-04-09 03:20:26 PDT
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 21 Holger Freyther 2009-04-09 03:41:57 PDT
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 22 Holger Freyther 2009-04-09 03:44:07 PDT
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).
Comment 23 Xan Lopez 2009-04-09 03:45:48 PDT
(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 Holger Freyther 2009-04-09 03:47:05 PDT
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 25 Holger Freyther 2009-04-09 04:06:16 PDT
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.
Comment 26 Xan Lopez 2009-04-14 06:35:16 PDT
Created attachment 29463 [details]
statictextrole.patch

Implement AtkText interface for objects with StaticTextRole.
Comment 27 Xan Lopez 2009-04-14 06:35:37 PDT
Created attachment 29464 [details]
cleanup.patch

Small cleanups.
Comment 28 Xan Lopez 2009-04-14 06:36:07 PDT
Created attachment 29465 [details]
atktextmethods.patch

Working implementations for AtkText::get_text and AtkText::get_character_count.
Comment 29 Holger Freyther 2009-04-14 07:39:50 PDT
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 30 Holger Freyther 2009-04-14 07:44:16 PDT
Comment on attachment 29465 [details]
atktextmethods.patch

Looks like a improvement.
Comment 31 Xan Lopez 2009-04-14 08:51:14 PDT
(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 Xan Lopez 2009-04-15 03:24:01 PDT
Created attachment 29497 [details]
atkactioninterface.patch

Only implement AtkAction when we actually have an action.
Comment 33 Xan Lopez 2009-04-15 09:44:26 PDT
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.
Comment 34 Xan Lopez 2009-04-16 01:05:01 PDT
Created attachment 29530 [details]
atkimageiface.patch

Implement AtkImage interface.
Comment 35 Xan Lopez 2009-04-16 06:10:35 PDT
Created attachment 29536 [details]
covermoreroles.patch

Cover more WebCore role -> ATK role conversions.
Comment 36 Xan Lopez 2009-04-18 00:23:18 PDT
(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?
Comment 37 Xan Lopez 2009-04-18 00:57:03 PDT
Created attachment 29600 [details]
getters.patch

Do not call setters in the getters functions.
Comment 38 Xan Lopez 2009-04-20 05:42:45 PDT
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.
Comment 39 Xan Lopez 2009-04-20 05:44:32 PDT
Created attachment 29613 [details]
focus.patch

The focus patch, with style fixed.
Comment 40 Gustavo Noronha (kov) 2009-04-20 11:27:48 PDT
Comment on attachment 29497 [details]
atkactioninterface.patch

Looks good to me.
Comment 41 Gustavo Noronha (kov) 2009-04-20 11:58:12 PDT
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.
Comment 42 Joanmarie Diggs 2009-04-24 14:37:25 PDT
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 Xan Lopez 2009-04-24 23:56:38 PDT
(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 Xan Lopez 2009-04-24 23:58:05 PDT
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 Joanmarie Diggs 2009-04-25 17:32:08 PDT
(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 Xan Lopez 2009-04-26 00:24:50 PDT
(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 Gustavo Noronha (kov) 2009-04-27 14:26:08 PDT
Comment on attachment 29530 [details]
atkimageiface.patch

r=me
Comment 48 Gustavo Noronha (kov) 2009-04-27 14:34:07 PDT
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 49 Gustavo Noronha (kov) 2009-04-27 14:35:52 PDT
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 50 Gustavo Noronha (kov) 2009-04-27 14:40:41 PDT
Comment on attachment 29613 [details]
focus.patch

Interesting. Are Mac, Win, GTK+ the only ports implementing a11y up to now?
Comment 51 Gustavo Noronha (kov) 2009-04-27 15:10:26 PDT
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.
Comment 52 Xan Lopez 2009-05-20 08:41:40 PDT
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 53 Gustavo Noronha (kov) 2009-05-20 08:55:30 PDT
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.
Comment 54 Xan Lopez 2009-05-20 10:01:14 PDT
(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!