Bug 72390 - [GTK] Do not hide accessibility root object from AT's
Summary: [GTK] Do not hide accessibility root object from AT's
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-11-15 08:55 PST by Mario Sanchez Prada
Modified: 2011-11-16 01:46 PST (History)
3 users (show)

See Also:


Attachments
Patch proposal (21.49 KB, patch)
2011-11-15 09:02 PST, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2011-11-15 08:55:13 PST
As per bug 51932, we made a change some time ago to bypass the root AccessibilityObject when exposing the a11y hierarchy to ATK based Assistive Technologies, in order to keep exposing the webArea object as the root a11y object, instead of the new one: an scrollview (whose child would be the old -well known- webArea object).

To fix this, we needed to tweak a bit the code in WebKitGtk's WebView widget to bypass that scrollview and keep returning the webArea object, as well as to make some changes in the AccessibilityObjectWrapper to fix bottom-up navigation as well (which turned out to be a wrong implementation, as I found out today!), which was a bit hackish and not clean, to say the least. In addition, now we're working in adding a11y support for WebKit2GTK+, we found out that we'd need to make the same bypass hack in the WebProcess, which means duplicating code (and maybe bugs), so we sit down (Joanie and me), talk a bit about it and came to the conclusion that the best thing to do here is to remove the hack, expose the a11y hierarchy as it is now (so the root object is the scrollview, and its child the webArea) and make Orca do the additional tweaks it might need to ignore that scrollview since it doesn't need it (but it could be that other ATs would do).

Of course, making this change would mean shifting the a11y hierarchy and therefore slightly tweaking the ATK unit tests to reflect that, but that seems not a problem at all. It would have been worse if Layout tests were affected by this change, but they are not, since Layout Tests use DRT to get the root object and DRT's implementation already returns the scrollview (no bypass there).

So, wrapping up, the intention of this change is two-fold:

  * Properly expose the true a11y hierarchy (ATs might still choose to ignore the scrollview as the root object)
         ==> Match better WebKit's a11y hierarchy

  * Avoid the need of keeping hacks in WebKit and WebKit2 to artificially bypass the root object (which proved to be buggy, btw)
        ==> Cleaner and less error-prone code
Comment 1 Mario Sanchez Prada 2011-11-15 09:02:09 PST
Created attachment 115174 [details]
Patch proposal
Comment 2 Martin Robinson 2011-11-15 09:06:42 PST
Comment on attachment 115174 [details]
Patch proposal

Does this mean we now depend on a newer version of Orca?
Comment 3 Mario Sanchez Prada 2011-11-15 09:21:33 PST
(In reply to comment #2)
> (From update of attachment 115174 [details])
> Does this mean we now depend on a newer version of Orca?

Nope, it just means that Orca will have to do some extra work from this version of WebKit on (already agreed with Joanmarie), but we do not depend on Orca by any means :-)
Comment 4 Joanmarie Diggs 2011-11-15 09:29:28 PST
(In reply to comment #2)
> (From update of attachment 115174 [details])
> Does this mean we now depend on a newer version of Orca?

Did you have a dependency on Orca before now, and didn't tell me? ;)

(In reply to comment #3)

> Nope, it just means that Orca will have to do some extra work from this version of WebKit on (already agreed with Joanmarie)

For the record, and as discussed via IRC, I was *willing* to do the work should it be needed. It is not needed. It's all good.
Comment 5 Martin Robinson 2011-11-15 09:38:01 PST
(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 115174 [details] [details])
> > Does this mean we now depend on a newer version of Orca?
> 
> Did you have a dependency on Orca before now, and didn't tell me? ;)

by this I didn't mean a link depedency, but whether if you used an old version of Orca and a new version of WebKitGTK+ (with this change) that it would be broken.
Comment 6 Mario Sanchez Prada 2011-11-15 09:43:45 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 115174 [details] [details] [details])
> > > Does this mean we now depend on a newer version of Orca?
> > 
> > Did you have a dependency on Orca before now, and didn't tell me? ;)
> 
> by this I didn't mean a link depedency, but whether if you used an old version of Orca and a new version of WebKitGTK+ (with this change) that it would be broken.

As per last Joanie's comment, I'd say "no, there shouldn't be any problem" :)
Comment 7 Martin Robinson 2011-11-15 14:57:50 PST
Comment on attachment 115174 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=115174&action=review

> Source/WebKit/gtk/tests/testatk.c:85
> +      return 0;
> +
> +    AtkObject* webAreaObject = atk_object_ref_accessible_child(rootObject, 0);
> +    if (!webAreaObject)
> +      return 0;
> +

In C files you should use NULL instead of 0. (I realize this is incredibly arbitrary, but the style guide specifies this!)
Comment 8 Mario Sanchez Prada 2011-11-16 01:46:03 PST
(In reply to comment #7)
> (From update of attachment 115174 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115174&action=review
> 
> > Source/WebKit/gtk/tests/testatk.c:85
> > +      return 0;
> > +
> > +    AtkObject* webAreaObject = atk_object_ref_accessible_child(rootObject, 0);
> > +    if (!webAreaObject)
> > +      return 0;
> > +
> 
> In C files you should use NULL instead of 0. (I realize this is incredibly arbitrary, but the style guide specifies this!)

Thanks for the review. I will change this before pushing
Comment 9 Mario Sanchez Prada 2011-11-16 01:46:56 PST
Committed r100424: <http://trac.webkit.org/changeset/100424>