460926 - A11y hierarchy is broken on Ubuntu 8.10 (GNOME 2.24)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Ginn Chen]

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.

Comment 1

[Ginn Chen]

Attachment: patch

Comment 2

[Ginn Chen]

Attachment: patch v2

Use g_object_set instead of gtk_setting_set_string_property as Matthias recommended.

Comment 4

[Steve Lee]

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

Comment 5

[Willie Walker]

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

Comment 6

[Steve Lee]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[alexander :surkov (:asurkov)]

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.

Comment 9

[alexander :surkov (:asurkov)]

Thank you, Evan.

Comment 10

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/71c3a9d14712

Comment 11

[Benjamin Smedberg]

This apparently caused a crash regression, bug 468845. Please don't land this on the branch without resolving that issue.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

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

Comment 14

[Benjamin Smedberg]

Backed out per bug 468845 - http://hg.mozilla.org/mozilla-central/rev/9bc6733d81cc

Comment 15

[Ginn Chen]

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.

Comment 16

[Ginn Chen]

Attachment: patch v3

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 353657 [details] [diff] [review]
patch v3

after some irc discussion with ginn this approach looks right with me, r=me

Comment 18

[Ginn Chen]

Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/59508a1ac7dc

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 353657 [details] [diff] [review]
patch v3

a191=beltzner

Comment 20

[Ginn Chen]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/87f7605e2c5e
Fixed in 1.9.1

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

Ginn, I recommend chatting with Brad Taylor over on #mono-a11y, apparently they are dealing with similar accessibility bootstrapping isues.

Comment 23

[Steve Lee]

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

Comment 24

[Brad Taylor]

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.

Comment 25

[Ginn Chen]

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.

Comment 26

[Steve Lee]

Thanks Ginn. 
Anyone know who to prod at Ubuntu?

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 28

[Ginn Chen]

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.

Comment 29

[Marco Zehe (:MarcoZ)]

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!

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 31

[Brad Taylor]

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?

Comment 32

[Brad Taylor]

Attachment: Proposed Fix

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 33

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 358887 [details] [diff] [review]
Proposed Fix

Evan can you take a look at Brad's patch?

Comment 34

[Alexander Sack]

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?

Comment 35

[Brad Taylor]

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

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 37

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 38

[Ginn Chen]

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

Comment 39

[Alexander Sack]

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.

Comment 40

[Samuel Sidler (old account; do not CC)]

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.

Comment 41

[Alexander Sack]

yeah, i actually ment 1.9.0.10. Thanks!

Comment 42

[Samuel Sidler (old account; do not CC)]

Comment on attachment 358887 [details] [diff] [review]
Proposed Fix

Approved for 1.9.0.10. a=ss

Comment 43

[Ginn Chen]

Fix committed to CVS.

Comment 44

[Daniel Holbert [:dholbert]]

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

Comment 45

[David Bolter [:davidb] (NeedInfo me for attention)]

I think this is an unlikely suspect. Ginn?

Comment 46

[Daniel Holbert [:dholbert]]

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

Comment 47

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 48

[Marco Zehe (:MarcoZ)]

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.

Comment 49

[Ginn Chen]

The patch has no impact on Windows or GNOME prior to 2.24.2.

Comment 50

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 52

[David Bolter [:davidb] (NeedInfo me for attention)]

Ginn, can you provide a status update to the last commenter on https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/348443 ?

Comment 53

[Ginn Chen]

I've commented there. The fix is in 3.0.11 not 3.0.10.