Closed Bug 460926 Opened 16 years ago Closed 15 years ago

A11y hierarchy is broken on Ubuntu 8.10 (GNOME 2.24)

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
blocker

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

(Keywords: access, fixed1.9.0.11, verified1.9.1)

Attachments

(2 files, 2 obsolete files)

It was reported at
http://bugzilla.gnome.org/show_bug.cgi?id=535827#c35

The problem is gtk+ loads gail and atk-bridge modules no matter what is set in GTK_MODULES.
So our workaround in bug 312092 is broken.

To completely resolve this issue, we need to make at-spi work with 2 a11y toolkits situation, no matter which module is loaded and initialized firstly.
But currently it's not working, and shutdown atk-bridge and re-init it will cause a crash in libORBit.

So currently we have to use another workaround.
Flags: wanted1.8.1.x?
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attached patch patch v2 (obsolete) — Splinter Review
Use g_object_set instead of gtk_setting_set_string_property as Matthias recommended.
Attachment #344044 - Attachment is obsolete: true
Attachment #346214 - Flags: review?(chpe)
Ginn pointed out a work around 

In the gconf registry change the value of
/apps/gnome_settings_daemon/gtk-modules/gail:atk-bridge
from
/desktop/gnome/interface/accessibility
to be empty.

Works for me
(In reply to comment #4)
> Ginn pointed out a work around 
> 
> In the gconf registry change the value of
> /apps/gnome_settings_daemon/gtk-modules/gail:atk-bridge
> from
> /desktop/gnome/interface/accessibility
> to be empty.
> 
> Works for me

Keep in mind that this workaround ends up making the logout/shutdown dialog of GNOME 2.24 inaccessible.
I'm using Fast switcher applet which the upgrade offered to replace the old shutdown and that has a menu of items that work for me.
Attachment #346214 - Flags: superreview?(roc)
Attachment #346214 - Flags: review?(chpe)
Attachment #346214 - Flags: review?(aaronleventhal)
Comment on attachment 346214 [details] [diff] [review]
patch v2

+      nsCString gtkModulesSettingStr(gtkModules);
+      gtkModulesSettingStr.ReplaceSubstring("atk-bridge", "");
+	  g_object_set(settings, "gtk-modules", gtkModulesSettingStr.get(), NULL);
+	  g_free(gtk_modules_setting);

Fix indent.
Attachment #346214 - Flags: superreview?(roc) → superreview+
Attachment #346214 - Flags: review?(aaronleventhal) → review?(surkov.alexander)
Ginn, possibly it's better to ask review from Evan because I'm bit far from the code you are fixing? Just it will require some amount of time from me to make the review.
Attachment #346214 - Flags: review?(surkov.alexander) → review+
Thank you, Evan.
http://hg.mozilla.org/mozilla-central/rev/71c3a9d14712
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #346214 - Flags: approval1.9.1?
Depends on: 468845
This apparently caused a crash regression, bug 468845. Please don't land this on the branch without resolving that issue.
(Following up comment #12)

Oops, my mistake.  My bug was (like bug 468845) caused by this patch.  So my bug is a dup of bug 468845.
Attachment #346214 - Flags: approval1.9.1?
Backed out per bug 468845 - http://hg.mozilla.org/mozilla-central/rev/9bc6733d81cc
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 346214 [details] [diff] [review]
patch v2

This patch have several problems.
Also changing gtk_settings is really dangerous.

So I'd like to take another approach.
We can set an env variable to suppress atk-bridge init at startup with the patch at
http://bugzilla.gnome.org/show_bug.cgi?id=563943
The patch will be in GNOME 2.24.2.

It would be a much safer way to do this.
Attachment #346214 - Attachment is obsolete: true
Attached patch patch v3Splinter Review
Attachment #353657 - Flags: superreview?(roc)
Attachment #353657 - Flags: review?(surkov.alexander)
Comment on attachment 353657 [details] [diff] [review]
patch v3

after some irc discussion with ginn this approach looks right with me, r=me
Attachment #353657 - Flags: review?(surkov.alexander) → review+
Attachment #353657 - Flags: superreview?(roc) → superreview+
Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/59508a1ac7dc
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Attachment #353657 - Flags: approval1.9.1?
Comment on attachment 353657 [details] [diff] [review]
patch v3

a191=beltzner
Attachment #353657 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/87f7605e2c5e
Fixed in 1.9.1
Flags: wanted1.8.1.x?
Keywords: fixed1.9.1
Hi. Today, on Ubuntu Intrepid I tried FF trunk and couldn't get accessibility info using accerciser. I still required Steve's workaround from comment 4. Can someone confirm my findings?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ginn, I recommend chatting with Brad Taylor over on #mono-a11y, apparently they are dealing with similar accessibility bootstrapping isues.
(In reply to comment #21)
> Hi. Today, on Ubuntu Intrepid I tried FF trunk and couldn't get accessibility
> info using accerciser. I still required Steve's workaround from comment 4. Can
> someone confirm my findings?

I can confirm that I just rolled back the setting in comment 4 and the problem reappears (using trunk).
To fix this issue, disabling AT-SPI (with NO_AT_BRIDGE) is only part of the picture.  Let me walk you through what happens when a Gtk+ app loads (e.g.: FF)

   0. ...
   1. gtk_init ()
   2. gtk_init_check ()
   3. gdk_display_open_default_libgtk_only ()
   ...
   4. gtk/gtkmodules.c: display_opened_cb ()
      Searches XSetting for gtk-modules setting set by gnome-settings-daemon
      -> gail is loaded, waits looking for at-spi
      -> atk-bridge is loaded, sees NO_AT_BRIDGE, exits
   5. App inits it's own ATK root window, loads atk-bridge
   6. gail sees atk-bridge, activates, and the app's root window never gets seen.

The fix is basically to also disable gail via an env variable.  The upstream gnome bug bgo#565110, and once that's in upstream, you just need to set NO_GAIL when your app starts.

FWIW, we'll be shipping that fix in OpenSUSE 11.1 shortly.
David and Steve, the fix requires a fix in atk-bridge. bgo#563943. The fix is in at-spi 1.24.1.
You need to wait for distros update their library.

Brad,
For Firefox, we don't need to suppress the loading of gail.
libgail just sets some callbacks and wait atk-bridge's calling.
When Firefox a11y modules inits, we override these callbacks. So it's not a problem.
Status: REOPENED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
Thanks Ginn. 
Anyone know who to prod at Ubuntu?
Ginn, thanks for persevering with this painful bug.

At Marco's suggestion I also updated to atk trunk. So with very recent at-spi and atk I tried again. I could not get FF to be accessible without the workaround in comment 4.

Reopen?
You don't need to update atk.
Actually the only library required to update is libatk-bridge.so.

You need to make sure the library being loaded is your version not the old version.

On Ubuntu 8.10, you can use this command to config at-spi.
./autogen.sh --prefix=/usr --libexecdir=/usr/lib/at-spi
make && sudo make install

Logout and re-login, it should be fine.
I believe our part of this bug is fixed and stuck on 1.9.1. Ginn, if you think this should not yet be set as verified, please remove the keyword again. Thanks!
(In reply to comment #29)
> I believe our part of this bug is fixed and stuck on 1.9.1. Ginn, if you think
> this should not yet be set as verified, please remove the keyword again.
> Thanks!

I'd like to get Brad Taylor's sign off. There might be some wiggly bits left.
I have a checkout of 1.9.1 now, and I'm seeing the same behavior as previously.  Firefox is only accessible if I add the NO_AT_BRIDGE=1 variable when running.  I'll try to hook this up to GDB to get a better understanding of why.

Could someone confirm this for me?
Attached patch Proposed FixSplinter Review
Doing some digging, I realized that with Ginn's fix NO_AT_BRIDGE=1 is only set when GTK_MODULES is set.  With bgo#535827 and friends, the GTK_MODULES env var has been obsoleted by an XSetting.

The attached patch moves the PR_SetEnv("NO_AT_BRIDGE=1") outside of the GTK_MODULES check so that it's set regardless.

The only downside that I can see with this change is that the NO_AT_BRIDGE env var will be set to 1 and then unset even when a11y isn't enabled.  If this is a problem, Firefox will need a better way of detecting whether a11y is enabled than checking the GTK_MODULES env var.  Looking at the /desktop/gnome/interface/accessibility GConf key might be a good way to go.
Comment on attachment 358887 [details] [diff] [review]
Proposed Fix

Evan can you take a look at Brad's patch?
Attachment #358887 - Flags: review?(Evan.Yan)
disabling at-bridge will make most a11y desktop tools not work, right?  Can someone clarify, whether this is the case and if so why this is something we consider?
Alexander -- that is most certainly not what is proposed or happening.

The NO_AT_BRIDGE env variable is used _temporarily_ in the process-local environment by applications which need to inhibit atk-bridge's startup occuring due to initializing GTK+.  While I'm not clear on the specifics behind Firefox's need for this, this is most commonly done because application level a11y support hasn't been loaded yet.  Once the app is ready, NO_AT_BRIDGE is set to 0, and then Firefox and other applications will manually load the atk-bridge module.

Again, this is process-local, and only done for a short amount of time so as to not affect processes invoked by Firefox (which I'm assuming will share the environment).
(In reply to comment #35)

Right.

> The NO_AT_BRIDGE env variable is used _temporarily_ in the process-local
> environment by applications which need to inhibit atk-bridge's startup occuring
> due to initializing GTK+.  While I'm not clear on the specifics behind
> Firefox's need for this, this is most commonly done because application level
> a11y support hasn't been loaded yet.

I believe this is done so that we can do lazy instantiation of the added accessibility processing. While I believe FF to be the most accessible browser, we have to be cognizant that browser choice is often made on performance criteria. In this way, we actually stand a better chance of spreading accessibility if we only turn it on when needed.
Attachment #358887 - Flags: review?(Evan.Yan) → review?(ginn.chen)
Don't believe me yet though, I came into this late and I'm not sure of the reasons. When Ginn's back from vacation we should get the real deal :)
Thank you, Brad!

I just realized the problem was when I land the patch to 1.9.1, I made this mistake.
Thanks for pointing out.

mozilla-central is fine.

So I think, technically, think we just need to correct the landing.

I just did it.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b0ad3180b30
Attachment #358887 - Flags: review?(ginn.chen)
Comment on attachment 358887 [details] [diff] [review]
Proposed Fix

Would be great to have a11y fixed in our ubuntu main browser; requesting approval for 1.9 branch.
Attachment #358887 - Flags: approval1.9.0.9?
Comment on attachment 358887 [details] [diff] [review]
Proposed Fix

Too late for 1.9.0.9, which is code frozen. Will consider for 1.9.0.10.
Attachment #358887 - Flags: approval1.9.0.9? → approval1.9.0.10?
yeah, i actually ment 1.9.0.10. Thanks!
Comment on attachment 358887 [details] [diff] [review]
Proposed Fix

Approved for 1.9.0.10. a=ss
Attachment #358887 - Flags: approval1.9.0.10? → approval1.9.0.10+
Fix committed to CVS.
Keywords: fixed1.9.0.10
Four Windows unit-test cycles have gone by since this landed, and three appear to have had sporadic timeouts (probably unrelated this bug) before they got to any a11y tests.

However, in the one cycle that *didn't* timeout, we actually failed some a11y tests, which I'm guessing might be related to this bug's landing.  The failures were:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.0/1239715197.1239718368.18523.gz
*** 135 ERROR FAIL | Wrong startIndex value for ID emptyLink! | got 97, expected 98 | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleHyperLink.html
*** 136 ERROR FAIL | Wrong endIndex value for ID emptyLink! | got 98, expected 99 | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleHyperLink.html
*** 143 ERROR FAIL | Wrong startIndex value for ID LinkWithSpan! | got 123, expected 124 | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleHyperLink.html
*** 144 ERROR FAIL | Wrong endIndex value for ID LinkWithSpan! | got 124, expected 125 | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleHyperLink.html
*** 156 ERROR FAIL | Wrong startIndex value for ID namedAnchor! | got 201, expected 202 | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleHyperLink.html
*** 157 ERROR FAIL | Wrong endIndex value for ID namedAnchor! | got 202, expected 203 | chrome://mochikit/content/a11y/accessible/test_nsIAccessibleHyperLink.html

URL of failing test:
http://mxr.mozilla.org/seamonkey/source/accessible/tests/mochitest/test_nsIAccessibleHyperLink.html
I think this is an unlikely suspect. Ginn?
Yeah, it's just that it was the *only* possible patch that could be suspect, since the failure was new failure (it hadn't happened in the 12 hours before this change landed), and it's accessibility-related which is a bit suspicious.

However, during the time since I posted comment 44, another unittest cycle has completed, and it was green:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.0/1239715197.1239730052.17163.gz

So maybe this was just a sporadic failure.  I don't particularly trust these particular tinderboxes -- they've been sporadically red a lot recently (I just filed bug 488345 on that).
Cool and thanks Daniel, for the original comment and the followup. I'd rather hear about these things than not... it is the good kinda 'noise'.
I know where these failures come from, and they are not related to this bug. David, this is the bug where we load an external image map, which we fixed on m-c not too long ago.
The patch has no impact on Windows or GNOME prior to 2.24.2.
(In reply to comment #48)
> I know where these failures come from, and they are not related to this bug.
> David, this is the bug where we load an external image map, which we fixed on
> m-c not too long ago.

Ah right! Did the fix not propogate?
Ginn, can you provide a status update to the last commenter on https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/348443 ?
I've commented there. The fix is in 3.0.11 not 3.0.10.
You need to log in before you can comment on or make changes to this bug.