Bug 30500 - [Gtk] Find a way for WebKit to "announce" itself so that ATs can readily distinguish it from true Gtk+/Gail
Summary: [Gtk] Find a way for WebKit to "announce" itself so that ATs can readily dist...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-10-18 20:55 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2010-07-02 07:02 PDT (History)
8 users (show)

See Also:


Attachments
new object attribute - includes layout test (10.68 KB, patch)
2010-03-13 15:14 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
new object attribute - includes layout test (10.64 KB, patch)
2010-03-17 12:59 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2009-10-18 20:55:08 PDT
See http://library.gnome.org/devel/atk/unstable/AtkUtil.html for more information.
We'll need to ensure that we claim to be WebKit when within the WebView and let the app handle the rest of the cases.

(I'd provide more details now, but I'm a tad sleepy and I plan to tackle this one myself. I just didn't want to forget about it.)
Comment 1 Alejandro Piñeiro 2009-10-19 03:49:35 PDT
I haven't tested it, but I suppose that right now, it is returning as toolkit name, GAIL. Right?

As far as I know, the gtk accessibility support in WebKit is not defining a new AtkUtil at all. It is just a new AtkObject that works as a wrapper of the internal a11y support in Webkit.

It is just the a11y support for a specific Gtk Widget, so I don't understand why this require to explicitly say that the toolkit is Webkit (as I really think that we are not in a different toolkit).

I think that you are asking it because in the Gecko implementation, it returns as toolkit "Gecko". But this is different:
  * Gecko defines a new AtkUtil implementation (see nsAppRootAccessible)
  * It redefines get_root, returning a different root (gail_root if not possible)
     * AFAIK, this doesn't happens with Webkit, Webkit always return the gail root.
  * It wraps the event management, as the "window:" signal can also be emitted by MaiAtkObject.
  * Other issues

So, in this case Gecko is defining a new toolkit (the root is different) that wraps part of their functionality on Gail.

But this is not the case with Webkit. The Webkit a11y support is just implement the Atk interfaces in a new Gtk widget. Technically (IMHO) this doesn't mean a new toolkit, it is just a "library" that defines a new gtk widget that requires a11y support. The main point of that is that Webkit is not defining a new get_root method. So I think that the correct toolkit is GAIL.

I'm missing any reason to consider that we are in a different toolkit?
Comment 2 Joanmarie Diggs (irc: joanie) 2009-10-19 04:02:27 PDT
(In reply to comment #1)
> I'm missing any reason to consider that we are in a different toolkit?

Yes. :-)

For any app for which there is not a specific, custom script in Orca, Orca queries the app to determine what toolkit is being used. How WebKit implements things is in some cases quite different from how the same things would be implemented by a Gtk+ app. If WebKit claims to be Gail, Orca's default script will handle things, and it will handle them wrong.

Does this make sense??
Comment 3 Alejandro Piñeiro 2009-10-19 04:37:27 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > I'm missing any reason to consider that we are in a different toolkit?
> 
> Yes. :-)
> 
> For any app for which there is not a specific, custom script in Orca, Orca
> queries the app to determine what toolkit is being used. How WebKit implements
> things is in some cases quite different from how the same things would be
> implemented by a Gtk+ app. If WebKit claims to be Gail, Orca's default script
> will handle things, and it will handle them wrong.
> 
> Does this make sense??

Yes, know I understand the reasons. Thanks.

Anyway, and probably just a nitpick, my concern about redefine toolkit_name and toolkit_version is because, as I said in my previous comment, there isn't a new toolkit here, so redefine the toolkit_name as "Webkit" is just a hack hint for the AT applications (like Orca).

But I also understand that from the POV of Orca would be more easy to implement a lot of things, as Webkit will not be tied to one specific application (it would be used for epiphany, devhelp, and so on) so it has sense to share it. The other option, quick-thought, would be have a epiphany, devhelp and so on scripts, all of them calling common python scripts, but the problem is that if it appears a new Gtk+ using WebKitWebView, it wouldn't use the specific Orca scripts without updating Orca.

Probably the best option to solve that is just redefine toolkit_name as you suggest, perhaps any others (Xan?) have their own opinion.
Comment 4 Joanmarie Diggs (irc: joanie) 2009-10-19 04:50:22 PDT
(In reply to comment #3)
> The other option, quick-thought, would be have a epiphany, devhelp and so on
> scripts, all of them calling common python scripts,

Yup, we do that sort of thing in Orca now.

> but the problem is that if
> it appears a new Gtk+ using WebKitWebView, it wouldn't use the specific Orca
> scripts without updating Orca.

Exactly.

> Probably the best option to solve that is just redefine toolkit_name as you
> suggest, perhaps any others (Xan?) have their own opinion.

The other option is to make WebKit act more Gailish. The biggest issue is the fact that text is rendered as a child (and in some cases multiple children) of HTMLElement.
Comment 5 Xan Lopez 2009-10-19 04:53:12 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > I'm missing any reason to consider that we are in a different toolkit?
> 
> Yes. :-)
> 
> For any app for which there is not a specific, custom script in Orca, Orca
> queries the app to determine what toolkit is being used. How WebKit implements
> things is in some cases quite different from how the same things would be
> implemented by a Gtk+ app. If WebKit claims to be Gail, Orca's default script
> will handle things, and it will handle them wrong.
> 
> Does this make sense??

I think the only thing that matters here is what the de-facto expected behavior is, so if Orca and others expect widgets within the WebView hierarchy to report something other than GTK+ as toolkit name, I'm fine with it.
Comment 6 Alejandro Piñeiro 2009-10-19 06:08:35 PDT
I was thinking a little about that, and, although probably I should check this deeply, right now I think that the problem is that this is not a easy change, and this would have several consequences.

In order to redefine this two methods you will need to create a new AtkUtil implementation. This is not just implement a new interface, is a subclass of AtkUtil, and it is designed to be a singleton class (so you can't implement it on the current atk wrapper, a new object is required).

This means that probably you will need to redefine all the AtkUtil methods in order to wrap properly the gail_util implementation of AtkUtil, and then redefine this two methods.

And of course, this class needs to be loaded in order to redefine all this methods, and this is required to be done *after* GAIL to overwrite properly the methods. We all know all the problems we had to load the specific a11y thingies on Firefox, ie: 

http://mail.gnome.org/archives/gnome-accessibility-list/2009-January/msg00030.html

How this class will be loaded?
  * There is something like webkit_init ()? Could be used?
  * A specific module for webkit a11y (WAIL)?
  * On the application side? (Check nsApplicationAccessibleWrap::Init() on nsAppRootAccessible.cpp)
     * This would be a mess with several applications using Webkit
Comment 7 Joanmarie Diggs (irc: joanie) 2009-10-19 06:32:21 PDT
Note to self: Given what a mess this is starting to sound like, we could alternatively cause WebKit to "announce itself" via an object attribute placed on its top-level object. Or something along those lines. I'll give it some more thought.

Resummarizing.
Comment 8 Joanmarie Diggs (irc: joanie) 2009-10-19 09:01:09 PDT
I was just chatting with Will Walker about this issue. He suggested such an object attribute be added to the Atk API. Enhancement request filed here: https://bugzilla.gnome.org/show_bug.cgi?id=598952. Once we know what the attribute's name is, we can add it to WebKit. :-)
Comment 9 Joanmarie Diggs (irc: joanie) 2010-03-13 15:14:12 PST
Created attachment 50663 [details]
new object attribute - includes layout test

Nothing has happened to the GNOME bug I filed; in the meantime, Orca (and potentially other ATs) still have the need to know if a given object is a WebKitGtk object. 

I discussed this further with Will, and he suggested an object attribute of 'toolkit:WebKitGtk'. Other toolkits (e.g. Gecko) can follow our lead. :-)

Note: This has no impact on the reported toolkit for the application; merely gives ATs a hint about where the object is coming from.
Comment 10 Joanmarie Diggs (irc: joanie) 2010-03-16 15:27:13 PDT
Comment on attachment 50663 [details]
new object attribute - includes layout test

Clearing the review? flag. Xan pointed out how my list iteration could be done better. (Thanks!)
Comment 11 Joanmarie Diggs (irc: joanie) 2010-03-17 12:59:47 PDT
Created attachment 50943 [details]
new object attribute - includes layout test

Hopefully this is the right way to do things. :-)
Comment 12 Xan Lopez 2010-05-10 09:45:23 PDT
Comment on attachment 50943 [details]
new object attribute - includes layout test

Thanks, this looks great now.
Comment 13 WebKit Commit Bot 2010-05-10 10:04:37 PDT
Comment on attachment 50943 [details]
new object attribute - includes layout test

Rejecting patch 50943 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 50943, '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
0943&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=30500&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 50943 from bug 30500.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 14 Xan Lopez 2010-05-10 10:11:15 PDT
Comment on attachment 50943 [details]
new object attribute - includes layout test

Pretty please?
Comment 15 WebKit Commit Bot 2010-05-10 10:28:12 PDT
Comment on attachment 50943 [details]
new object attribute - includes layout test

Rejecting patch 50943 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 50943, '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
0943&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=30500&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 50943 from bug 30500.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 16 Xan Lopez 2010-05-14 11:45:21 PDT
Comment on attachment 50943 [details]
new object attribute - includes layout test

Try again.
Comment 17 WebKit Commit Bot 2010-05-15 02:56:02 PDT
Comment on attachment 50943 [details]
new object attribute - includes layout test

Clearing flags on attachment: 50943

Committed r59532: <http://trac.webkit.org/changeset/59532>
Comment 18 WebKit Commit Bot 2010-05-15 02:56:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2010-05-15 04:09:36 PDT
http://trac.webkit.org/changeset/59532 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/59530
http://trac.webkit.org/changeset/59531
http://trac.webkit.org/changeset/59532
http://trac.webkit.org/changeset/59533
Comment 20 Joanmarie Diggs (irc: joanie) 2010-05-15 07:24:36 PDT
(In reply to comment #19)
> http://trac.webkit.org/changeset/59532 might have broken GTK Linux 32-bit Release
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/59530
> http://trac.webkit.org/changeset/59531
> http://trac.webkit.org/changeset/59532
> http://trac.webkit.org/changeset/59533

Maybe.... But I doubt it. 

* This change adds an attribute onto the AtkObject

* (To my knowledge) none of our existing tests were checking object attributes, so this isn't introducing anything new which might break a test.

* This change implements a previously-unimplemented function in our DRT

* (To my knowledge) none of our existing tests were calling that function because it wasn't implemented. :-)

Having said that, I've been known to be wrong before. If it's my fault, let me know.