WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
38648
Ensure DRT loads GAIL (Gtk+ module), for a11y tests
https://bugs.webkit.org/show_bug.cgi?id=38648
Summary
Ensure DRT loads GAIL (Gtk+ module), for a11y tests
Mario Sanchez Prada
Reported
2010-05-06 08:50:42 PDT
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.
Attachments
Patch to fix this issue
(1.87 KB, patch)
2010-05-06 10:47 PDT
,
Mario Sanchez Prada
no flags
Details
Formatted Diff
Diff
Patch to fix this issue
(1.87 KB, patch)
2010-05-06 10:55 PDT
,
Mario Sanchez Prada
xan.lopez
: review-
Details
Formatted Diff
Diff
Patch to fix this issue
(1.85 KB, patch)
2010-05-07 03:09 PDT
,
Mario Sanchez Prada
no flags
Details
Formatted Diff
Diff
Patch to fix this issue
(1.93 KB, patch)
2010-06-30 01:38 PDT
,
Mario Sanchez Prada
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Mario Sanchez Prada
Comment 1
2010-05-06 09:08:06 PDT
Blocking
bug 31018
as this issue directly affects to the possibility of implementing the isSelected() method in Gtk's DRT
Mario Sanchez Prada
Comment 2
2010-05-06 10:47:58 PDT
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
Comment 3
2010-05-06 10:49:06 PDT
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
Comment 4
2010-05-06 10:55:04 PDT
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 5
2010-05-07 01:32:15 PDT
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
Comment 6
2010-05-07 03:09:29 PDT
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 7
2010-05-10 10:01:43 PDT
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 8
2010-05-10 10:17:22 PDT
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 9
2010-05-15 00:03:45 PDT
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 10
2010-05-15 11:26:57 PDT
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
Comment 11
2010-05-15 11:27:03 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 12
2010-05-15 11:45:51 PDT
http://trac.webkit.org/changeset/59544
might have broken GTK Linux 32-bit Release
Adam Barth
Comment 13
2010-05-15 11:49:26 PDT
editing/undo/4059423-1.html crashed. Same error on GTK Linux 64-bit Release. Rolling out.
Eric Seidel (no email)
Comment 14
2010-05-16 13:36:41 PDT
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
Comment 15
2010-05-25 09:34:27 PDT
(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
Comment 16
2010-06-10 08:09:55 PDT
(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 17
2010-06-29 04:51:27 PDT
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
Comment 18
2010-06-29 05:07:12 PDT
(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 19
2010-06-29 05:39:23 PDT
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
Comment 20
2010-06-29 05:39:29 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 21
2010-06-29 06:07:00 PDT
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
Comment 22
2010-06-29 09:17:55 PDT
(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
Comment 23
2010-06-29 09:25:29 PDT
(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
Comment 24
2010-06-30 01:38:05 PDT
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
Comment 25
2010-06-30 23:25:53 PDT
(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 26
2010-07-12 02:25:33 PDT
Comment on
attachment 60098
[details]
Patch to fix this issue For the horde.
Xan Lopez
Comment 27
2010-07-12 10:42:09 PDT
Reopen so the bot pushes the patch.
WebKit Commit Bot
Comment 28
2010-07-12 11:18:46 PDT
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
Comment 29
2010-07-12 11:18:52 PDT
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 30
2010-07-12 13:58:58 PDT
I think this caused Gtk API test failures.
Mario Sanchez Prada
Comment 31
2010-07-12 14:08:36 PDT
(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
Comment 32
2010-07-12 14:22:33 PDT
This was rolled out and the API test still fails, reopening.
Xan Lopez
Comment 33
2010-07-12 14:23:00 PDT
Comment on
attachment 60098
[details]
Patch to fix this issue Hopefully last time we do this :D
Mario Sanchez Prada
Comment 34
2010-07-12 14:42:06 PDT
(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 35
2010-07-12 15:51:53 PDT
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
Comment 36
2010-07-12 15:52:00 PDT
All reviewed patches have been landed. Closing bug.
Mario Sanchez Prada
Comment 37
2010-07-12 17:01:04 PDT
(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
Comment 38
2010-07-12 17:25:17 PDT
(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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug