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.
Blocking bug 31018 as this issue directly affects to the possibility of implementing the isSelected() method in Gtk's DRT
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.
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.
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
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.
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 :-)
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+.
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).
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.
Comment on attachment 55354 [details] Patch to fix this issue Clearing flags on attachment: 55354 Committed r59544: <http://trac.webkit.org/changeset/59544>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/59544 might have broken GTK Linux 32-bit Release
editing/undo/4059423-1.html crashed. Same error on GTK Linux 64-bit Release. Rolling out.
re-opening since rolled out. We need to fix the commit-queue to re-open the original bug when landing rollout patches.
(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?
(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 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.
(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! :-)
Comment on attachment 55354 [details] Patch to fix this issue Clearing flags on attachment: 55354 Committed r62106: <http://trac.webkit.org/changeset/62106>
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
(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.
(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
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
(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?
Comment on attachment 60098 [details] Patch to fix this issue For the horde.
Reopen so the bot pushes the patch.
Comment on attachment 60098 [details] Patch to fix this issue Clearing flags on attachment: 60098 Committed r63101: <http://trac.webkit.org/changeset/63101>
I think this caused Gtk API test failures.
(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 :-(
This was rolled out and the API test still fails, reopening.
Comment on attachment 60098 [details] Patch to fix this issue Hopefully last time we do this :D
(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 on attachment 60098 [details] Patch to fix this issue Clearing flags on attachment: 60098 Committed r63133: <http://trac.webkit.org/changeset/63133>
(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
(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