Bug 38648 – Ensure DRT loads GAIL (Gtk+ module), for a11y tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

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.

[Mario Sanchez Prada]

Blocking bug 31018 as this issue directly affects to the possibility of implementing the isSelected() method in Gtk's DRT

[Mario Sanchez Prada]

Created attachment 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.

[WebKit Review Bot]

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.

[Mario Sanchez Prada]

Created attachment 55267 [details]
Patch to fix this issue

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

Sorry, forgot to check style. Uploading new patch

[Xan Lopez]

Comment on attachment 55267 [details]
Patch to fix this issue

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.

[Mario Sanchez Prada]

Created attachment 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 :-)

[Xan Lopez]

Comment on attachment 55354 [details]
Patch to fix this issue

Looks good to me, we just need to remember to update the new script when it supports GTK+.

[WebKit Commit Bot]

Comment on attachment 55354 [details]
Patch to fix this issue

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).

[Adam Barth]

Comment on attachment 55354 [details]
Patch to fix this issue

Eric said this rejection was caused by a bug in svn-apply, which hopefully has been fixed.

[WebKit Commit Bot]

Comment on attachment 55354 [details]
Patch to fix this issue

Clearing flags on attachment: 55354

Committed r59544: <http://trac.webkit.org/changeset/59544>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59544 might have broken GTK Linux 32-bit Release

[Adam Barth]

editing/undo/4059423-1.html crashed.  Same error on GTK Linux 64-bit Release.  Rolling out.

[Eric Seidel (no email)]

re-opening since rolled out.  We need to fix the commit-queue to re-open the original bug when landing rollout patches.

[Mario Sanchez Prada]

(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?

[Mario Sanchez Prada]

(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

[Xan Lopez]

Comment on attachment 55354 [details]
Patch to fix this issue

I can't reproduce the crash locally either, so let's try to get this in again.

[Mario Sanchez Prada]

(In reply to comment #17)
> (From update of attachment 55354 [details])
> I can't reproduce the crash locally either, so let's try to get this in again.

Cross fingers! :-)

[WebKit Commit Bot]

Comment on attachment 55354 [details]
Patch to fix this issue

Clearing flags on attachment: 55354

Committed r62106: <http://trac.webkit.org/changeset/62106>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Mario Sanchez Prada]

(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.

[Mario Sanchez Prada]

(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

[Mario Sanchez Prada]

Created attachment 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

[Mario Sanchez Prada]

(In reply to comment #24)
> 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:

Btw, shouldn't we reopen this bug then?

[Xan Lopez]

Comment on attachment 60098 [details]
Patch to fix this issue

For the horde.

[Xan Lopez]

Reopen so the bot pushes the patch.

[WebKit Commit Bot]

Comment on attachment 60098 [details]
Patch to fix this issue

Clearing flags on attachment: 60098

Committed r63101: <http://trac.webkit.org/changeset/63101>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

I think this caused Gtk API test failures.

[Mario Sanchez Prada]

(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 :-(

[Xan Lopez]

This was rolled out and the API test still fails, reopening.

[Xan Lopez]

Comment on attachment 60098 [details]
Patch to fix this issue

Hopefully last time we do this :D

[Mario Sanchez Prada]

(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...

[WebKit Commit Bot]

Comment on attachment 60098 [details]
Patch to fix this issue

Clearing flags on attachment: 60098

Committed r63133: <http://trac.webkit.org/changeset/63133>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mario Sanchez Prada]

(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

[Mario Sanchez Prada]

(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