Bug 38648 - Ensure DRT loads GAIL (Gtk+ module), for a11y tests
: Ensure DRT loads GAIL (Gtk+ module), for a11y tests
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
: 39165 41346 42103
: 31018
  Show dependency treegraph
 
Reported: 2010-05-06 08:50 PST by
Modified: 2010-07-12 17:25 PST (History)


Attachments
Patch to fix this issue (1.87 KB, patch)
2010-05-06 10:47 PST, Mario Sanchez Prada
no flags Review Patch | Details | Formatted Diff | Diff
Patch to fix this issue (1.87 KB, patch)
2010-05-06 10:55 PST, Mario Sanchez Prada
xan.lopez: review-
Review Patch | Details | Formatted Diff | Diff
Patch to fix this issue (1.85 KB, patch)
2010-05-07 03:09 PST, Mario Sanchez Prada
no flags Review Patch | Details | Formatted Diff | Diff
Patch to fix this issue (1.93 KB, patch)
2010-06-30 01:38 PST, Mario Sanchez Prada
no flags 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 2010-05-06 08:50:42 PST
After some investigation while working for fixing bug 31018, I found that the AtkObject* returned by AccessibilityController::rootElement() -in AccessibilityControllerGtk.cpp- is *not a valid AtkObject* at all (ATK_ROLE_INVALID), which makes impossible to later access to its only child and therefore any AccessibleUIElement associated to the DOM's elements.

As a consecuence of this, it was not possible to implement the isSelected() method in DRT to make the accessibility/aria-controls-with-tabs.html test success, as it was not possible to access to any AccessibleUIElement in the DOM starting from the root element.

After some investigation I found the problem was that the GAIL Gtk module was not being loaded along with the execution of DRT (even if the environment variable was already set), and that's the reason behind those AtkObjects returned by rootElement() not being valid.

Therefore, we need to make sure GAIL is always loaded with DRT if we want this kind of a11y tests (using the AccessibleUIElements starting from the root element) work properly.

As side note: it seems that some a11y apps like Orca and Accerciser manually make sure the GTK_MODULES envvar is properly loaded along with their executions, so perhaps that's the way to go here, providing it would only affect to the GTK implementation of the DRT.
------- Comment #1 From 2010-05-06 09:08:06 PST -------
Blocking bug 31018 as this issue directly affects to the possibility of implementing the isSelected() method in Gtk's DRT
------- Comment #2 From 2010-05-06 10:47:58 PST -------
Created an attachment (id=55265) [details]
Patch to fix this issue

This patch makes sure that the Gtk version of the DRT always set the GTK_MODULES variable to the proper value ("gail") before calling to gtk_init(), in a similar fashion to how Orca an Accerciser does it.
------- Comment #3 From 2010-05-06 10:49:06 PST -------
Attachment 55265 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:912:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #4 From 2010-05-06 10:55:04 PST -------
Created an attachment (id=55267) [details]
Patch to fix this issue

(In reply to comment #3)
> Attachment 55265 [details] [details] did not pass style-queue:
> 

Sorry, forgot to check style. Uploading new patch
------- Comment #5 From 2010-05-07 01:32:15 PST -------
(From update of attachment 55267 [details])
We agreed to spend some time investigating why GTK_MODULES is being thrown away by the test scripts, and whether or not it makes sense to try to be more careful when setting it in the application.
------- Comment #6 From 2010-05-07 03:09:29 PST -------
Created an attachment (id=55354) [details]
Patch to fix this issue

According to comment #5 I've been spending some time to find out why DRT is ran without considering the GTK_MODULES envvar (if already set) and I found the reason:

In WebKitTools/Scripts/old-run-webkit-tests > openDumpTool() a clean environment is prepared before calling to DRT, which basically creates a hashtable with the strictly needed envvars and nothing else, and that obviously doesn't include GTK_MODULES at this point.

So, I think the current patch properly addresses the problem by just setting the GTK_MODULES envvar to "gail" when running the tests for the Gtk port, as that would be a clear requirement for the a11y tests to run ok under that scenario.

Wrt the doubts also commented with xan about whether to append 'gail' to the already present (if so) GTK_MODULES instead of just *setting* it to a specific value, I'd say that the right thing to do in this case is just to set it, since we don't want to put in that clean environment anything else but the strictly needed things, which in this case means (at least afaik) to load Gail as the only Gtk module.

Hope this patch looks better :-)
------- Comment #7 From 2010-05-10 10:01:43 PST -------
(From update of attachment 55354 [details])
Looks good to me, we just need to remember to update the new script when it supports GTK+.
------- Comment #8 From 2010-05-10 10:17:22 PST -------
(From update of attachment 55354 [details])
Rejecting patch 55354 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', 55354, '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
5354&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=38648&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 55354 from bug 38648.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebKitTools/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebKitTools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
------- Comment #9 From 2010-05-15 00:03:45 PST -------
(From update of attachment 55354 [details])
Eric said this rejection was caused by a bug in svn-apply, which hopefully has been fixed.
------- Comment #10 From 2010-05-15 11:26:57 PST -------
(From update of attachment 55354 [details])
Clearing flags on attachment: 55354

Committed r59544: <http://trac.webkit.org/changeset/59544>
------- Comment #11 From 2010-05-15 11:27:03 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #12 From 2010-05-15 11:45:51 PST -------
http://trac.webkit.org/changeset/59544 might have broken GTK Linux 32-bit Release
------- Comment #13 From 2010-05-15 11:49:26 PST -------
editing/undo/4059423-1.html crashed.  Same error on GTK Linux 64-bit Release.  Rolling out.
------- Comment #14 From 2010-05-16 13:36:41 PST -------
re-opening since rolled out.  We need to fix the commit-queue to re-open the original bug when landing rollout patches.
------- Comment #15 From 2010-05-25 09:34:27 PST -------
(In reply to comment #14)
> re-opening since rolled out.  We need to fix the commit-queue to re-open the original bug when landing rollout patches.

I can't reproduce this in my 32-bit compilation of WebKit (Gtk version), with that patch applied:

$ run-webkit-tests --gtk editing/undo/4059423-1.html
Running build-dumprendertree
Running tests from /home/mario/work/gnome2/WebKit/LayoutTests
Testing 1 test cases.
editing/undo .
0.38s total testing time

all 1 test cases succeeded


Am I missing something, perhaps?
------- Comment #16 From 2010-06-10 08:09:55 PST -------
(In reply to comment #15)
> (In reply to comment #14)
> > re-opening since rolled out.  We need to fix the commit-queue to re-open the original bug when landing rollout patches.
> 
> I can't reproduce this in my 32-bit compilation of WebKit (Gtk version), with that patch applied:

Strike two:

I have checked this several times again, and didn't find anything wrong with that. Moreover, considering the nature of this patch (which just adds a envvar to the script for running the tests) I'm a bit confused about how this could be causing a problem at all. It looks to me more like an issue of flackyness or something like that than this patch breaking the build.

Could someone help me to clarify this? Perhaps I'm not seeing anything or not doing the proper tests, and that's all. But it would be nice to have this issue fixed as it's blocking some a11y tests in the GTK port since some stuff from DRT won't work properly without it.

Thanks
------- Comment #17 From 2010-06-29 04:51:27 PST -------
(From update of attachment 55354 [details])
I can't reproduce the crash locally either, so let's try to get this in again.
------- Comment #18 From 2010-06-29 05:07:12 PST -------
(In reply to comment #17)
> (From update of attachment 55354 [details] [details])
> I can't reproduce the crash locally either, so let's try to get this in again.

Cross fingers! :-)
------- Comment #19 From 2010-06-29 05:39:23 PST -------
(From update of attachment 55354 [details])
Clearing flags on attachment: 55354

Committed r62106: <http://trac.webkit.org/changeset/62106>
------- Comment #20 From 2010-06-29 05:39:29 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #21 From 2010-06-29 06:07:00 PST -------
http://trac.webkit.org/changeset/62106 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/62105
http://trac.webkit.org/changeset/62106
------- Comment #22 From 2010-06-29 09:17:55 PST -------
(In reply to comment #21)
> http://trac.webkit.org/changeset/62106 might have broken GTK Linux 32-bit Release
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/62105
> http://trac.webkit.org/changeset/62106

The problem behind this broken build is not this bug, but the fact that the new tests added along with bug 36128 need to be updated to match the new code added in patch for bug 40009 (now a11y title and descriptions are prefixed by "AXTitle: " and "AXDescription: ", respectively).

Hence, now reopening bug 40009 and attaching the needed patch to updated the failing test and its expected results: platform/gtk/accesibility/name-for-label.
------- Comment #23 From 2010-06-29 09:25:29 PST -------
(In reply to comment #22)
> [...]
> Hence, now reopening bug 40009 and attaching the needed patch to updated the failing test and its expected results: platform/gtk/accesibility/name-for-label.

Much better, filed a new bug to fix this small issue:
https://bugs.webkit.org/show_bug.cgi?id=41355
------- Comment #24 From 2010-06-30 01:38:05 PST -------
Created an attachment (id=60098) [details]
Patch to fix this issue

It looks like this patch has been rolled out because it seems it broke some tests:

https://bugs.webkit.org/show_bug.cgi?id=41346
http://trac.webkit.org/changeset/62110

I had no idea what could went wrong there apart that there was a missing ";" in the previous patch (I didn't realize of this since it worked in my local environment), so I'm now attaching a new patch with that simple modification to see if it helps, but no many hopes put down on there...

Btw, any tip on this would be welcome, since this bug is blocking other a11y fixes and if I can't reproduce the problem locally I'm not sure I'll be able to find the proper solution for that.

Thanks
------- Comment #25 From 2010-06-30 23:25:53 PST -------
(In reply to comment #24)
> Created an attachment (id=60098) [details] [details]
> Patch to fix this issue
> 
> It looks like this patch has been rolled out because it seems it broke some tests:

Btw, shouldn't we reopen this bug then?
------- Comment #26 From 2010-07-12 02:25:33 PST -------
(From update of attachment 60098 [details])
For the horde.
------- Comment #27 From 2010-07-12 10:42:09 PST -------
Reopen so the bot pushes the patch.
------- Comment #28 From 2010-07-12 11:18:46 PST -------
(From update of attachment 60098 [details])
Clearing flags on attachment: 60098

Committed r63101: <http://trac.webkit.org/changeset/63101>
------- Comment #29 From 2010-07-12 11:18:52 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #30 From 2010-07-12 13:58:58 PST -------
I think this caused Gtk API test failures.
------- Comment #31 From 2010-07-12 14:08:36 PST -------
(In reply to comment #30)
> I think this caused Gtk API test failures.

I'm currently trying to reproduce the error in my local machine but no success so far. Moreover, I've to recognize I'm a bit lost with this issue (which seems to be doomed :-)) since I can't see why a change in old-run-webkit-tests script can affect to the API tests which are supposed not to use this script, afaik.

Now rebasing the patch against last commit from trunk in my local machine, to see if I can reproduce it... sorry for all the trouble coming all the time with this patch :-(
------- Comment #32 From 2010-07-12 14:22:33 PST -------
This was rolled out and the API test still fails, reopening.
------- Comment #33 From 2010-07-12 14:23:00 PST -------
(From update of attachment 60098 [details])
Hopefully last time we do this :D
------- Comment #34 From 2010-07-12 14:42:06 PST -------
(In reply to comment #32)
> This was rolled out and the API test still fails, reopening.

I can reproduce the API fail with last trunk, regardless of this patch being applied or not, so the problem is somewhere else for sure...
------- Comment #35 From 2010-07-12 15:51:53 PST -------
(From update of attachment 60098 [details])
Clearing flags on attachment: 60098

Committed r63133: <http://trac.webkit.org/changeset/63133>
------- Comment #36 From 2010-07-12 15:52:00 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #37 From 2010-07-12 17:01:04 PST -------
(In reply to comment #36)
> All reviewed patches have been landed.  Closing bug.

JFTR, I've been bisecting for a while and found that the guilty commit breaking the API test was this one:

http://trac.webkit.org/changeset/63100

I don't know whether that commit actually broke something in API tests or just unveiled a problem somewhere else, but git bisect doesn't lie and before that commit everything worked fine wrt those tests.

My 2 cents so far
------- Comment #38 From 2010-07-12 17:25:17 PST -------
(In reply to comment #37)
> (In reply to comment #36)
> > All reviewed patches have been landed.  Closing bug.
> 
> JFTR, I've been bisecting for a while and found that the guilty commit breaking the API test was this one:
> 
> http://trac.webkit.org/changeset/63100
> 
> I don't know whether that commit actually broke something in API tests or just unveiled a problem somewhere else, but git bisect doesn't lie and before that commit everything worked fine wrt those tests.
> 
> My 2 cents so far

Hmmm.. looks like I was too slow, as Martin already filed a bug for this: 

https://bugs.webkit.org/show_bug.cgi?id=42114

Sorry for the spam