693343 - accessibility always disabled in GNOME 3 unless GNOME_ACCESSIBILITY is set

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mike Gorse]

Currently, Firefox determines that accessibility is enabled if GNOME_ACCESSIBILITY is set to 1 in the environment or if /desktop/gnome/interface/accessibility is set to True in gconf.  The latter check successfully determines whether accessibility is enabled for GNOME 2 but not for GNOME 3.  Currently, a distribution would need to, ie, patch the firefox loader script to set GNOME_ACCESSIBILITY=1 for Firefox to be accessible under GNOME 3.

There is now a gsettings key (toolkit-accessibility in the org.gnome.desktop.interface schema), but this is specific to GNOME, and so using it may not be optimal with AT-SPI becoming usable in XFCE and eventually KDE.  In order to handle this, the AT-SPI bus launcher (org.a11y.Bus) has a org.a11y.Status.IsEnabled property on the bus object (/org/a11y/bus).  Under GNOME, this is a proxy for the GSettings key.  Xfce will likely have code in the future to ensure that this property is set correctly.  I would recommend that Firefox should check this property first, falling back to gconf if there is an error checking the property, but I'm not sure how best to handle this in the code.  Currently, there is a system pref (config.use_system_prefs.accessibility) which maps to the gconf key.  There is also code in several places that checks the value of GNOME_ACCESSIBILITY.  So I guess we want to somehow have custom handling for this system prefernce, if that is doable, or write a function somewhere to test whether a11y is enabled (making the D-Bus call and falling back to checking the system pref on error) and use that in the places where it is needed to check the state of accessibility.

Advice welcome.

Comment 1

[Mike Gorse]

Attachment: Proposed patch.

Check org.a11y.Status.IsEnabled to determine whether accessibility is enabled
before checking gconf.

This is adding more code that is in two places; I don't know if there is
now a better way to handle it (see bug 390761).

An alternate approach would be to have mozilla.sh call dbus-send and set
GNOME_ACCESSIBILITY=1 if the dbus property is set to true and
GNOME_ACCESSIBILITY is unset.

Comment 2

[Ginn Chen]

I don't know how much time dbus will take. I hope it will not slow down startup time.

Since libxul is always enabled now, i.e. nsWindows.cpp and nsApplicationAccessibleWrap.cpp are linked into libxul.so, I think you may not need to copy the code twice.

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to Ginn Chen from comment #2)
> I don't know how much time dbus will take. I hope it will not slow down
> startup time.

I'm a little concerned about this too, but I suspect doing the dbus call off the main thread will be enough work that we should get real numbers before deciding to do it.

> Since libxul is always enabled now, i.e. nsWindows.cpp and
> nsApplicationAccessibleWrap.cpp are linked into libxul.so, I think you may
> not need to copy the code twice.

I suspect this is correct, but I'm ok with not cleaning up the existing dupplicated code in this bug.  What I'd suggest is put the  function to check  the dbus  status in the a11y namespace and make it non-static and   use in the widget check.

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 568753 [details] [diff] [review]
Proposed patch.


>+static bool
>+test_a11y_dbus (bool *out)

I think I'd use  IsDBusA11yEnabled()

>+{
>+    // XXX following code is copied from widget/src/gtk2/nsWindow.cpp
>+    // we should put it somewhere that can be used from both modules
>+    // see bug 390761
>+    bool retval = FALSE;
>+#ifdef MOZ_ENABLE_DBUS
>+    DBusConnection *bus;
>+    DBusMessage *message = NULL, *reply = NULL;
>+    DBusMessageIter iter, iter_variant, iter_struct;
>+    dbus_bool_t d_result;
>+    DBusError error;

this  isn't ansi c89 ;)  please declare these closer to where they are used.
>+    reply = dbus_connection_send_with_reply_and_block(bus, message, 1000, &error);
>+    if (!reply ||
>+        dbus_message_get_type (reply) != DBUS_MESSAGE_TYPE_METHOD_RETURN ||
>+        strcmp (dbus_message_get_signature (reply), "v") != 0)
>+        goto exit;

blank line

btw what is "v" as a dbus signature?

Comment 5

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

Comment on attachment 568753 [details] [diff] [review]
Proposed patch.

Review of attachment 568753 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
@@ +618,5 @@
> +test_a11y_dbus (bool *out)
> +{
> +    // XXX following code is copied from widget/src/gtk2/nsWindow.cpp
> +    // we should put it somewhere that can be used from both modules
> +    // see bug 390761

Why not fix this now? You can just make the widget version nonstatic and call it from here.

@@ +625,5 @@
> +    DBusConnection *bus;
> +    DBusMessage *message = NULL, *reply = NULL;
> +    DBusMessageIter iter, iter_variant, iter_struct;
> +    dbus_bool_t d_result;
> +    DBusError error;

This is C++ code, just declare these where they're first assigned wherever possible

@@ +627,5 @@
> +    DBusMessageIter iter, iter_variant, iter_struct;
> +    dbus_bool_t d_result;
> +    DBusError error;
> +    const char *iface = "org.a11y.Status";
> +    const char *member = "IsEnabled";

static const char iface[] = ...;
static const char member[] = ...;

@@ +636,5 @@
> +        goto exit;
> +
> +    message = dbus_message_new_method_call ("org.a11y.Bus", "/org/a11y/bus",
> +                                            "org.freedesktop.DBus.Properties",
> +                                            "Get");

How fast is this? We're calling this on every widget creation, could this be slow?

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 568753 [details] [diff] [review] [diff] [details] [review]
> Proposed patch.
> 
> Review of attachment 568753 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
> @@ +618,5 @@
> > +test_a11y_dbus (bool *out)
> > +{
> > +    // XXX following code is copied from widget/src/gtk2/nsWindow.cpp
> > +    // we should put it somewhere that can be used from both modules
> > +    // see bug 390761
> 
> Why not fix this now? You can just make the widget version nonstatic and
> call it from here.
> 
> @@ +625,5 @@
> > +    DBusConnection *bus;
> > +    DBusMessage *message = NULL, *reply = NULL;
> > +    DBusMessageIter iter, iter_variant, iter_struct;
> > +    dbus_bool_t d_result;
> > +    DBusError error;
> 
> This is C++ code, just declare these where they're first assigned wherever
> possible
> 
> @@ +627,5 @@
> > +    DBusMessageIter iter, iter_variant, iter_struct;
> > +    dbus_bool_t d_result;
> > +    DBusError error;
> > +    const char *iface = "org.a11y.Status";
> > +    const char *member = "IsEnabled";
> 
> static const char iface[] = ...;
> static const char member[] = ...;
> 
> @@ +636,5 @@
> > +        goto exit;
> > +
> > +    message = dbus_message_new_method_call ("org.a11y.Bus", "/org/a11y/bus",
> > +                                            "org.freedesktop.DBus.Properties",
> > +                                            "Get");
> 
> How fast is this? We're calling this on every widget creation, could this be
> slow?

oh? it probably isn't the fastest thing in the world, but I'm not really sure.  If that code runs every time we create a widget (which I assume we do a lot) we should probably get rid of the gconf check there too.  I'm not sure how fast gconf is, but I can't see it being terriffic.

The really correct solution here would be  to call the dbus method once on startup, and then ask dbus to send us a signal  when it changes, but I for one have no idea how easy that will be to do (I don't know anything about how gecko currently interacts with dbus)

Comment 7

[Mike Gorse]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)

> > +    const char *iface = "org.a11y.Status";
> > +    const char *member = "IsEnabled";
> 
> static const char iface[] = ...;
> static const char member[] = ...;

If I do this, then I get a seg fault, whether I preface the reference with & or not. I need a char ** to pass to dbus_message_append_args.

> @@ +636,5 @@
> > +        goto exit;
> > +
> > +    message = dbus_message_new_method_call ("org.a11y.Bus", "/org/a11y/bus",
> > +                                            "org.freedesktop.DBus.Properties",
> > +                                            "Get");
> 
> How fast is this? We're calling this on every widget creation, could this be
> slow?

On my laptop (2.4ghz Core 2 Duo), it takes 0.6ms for the call plus an additional millisecond the first time a connection to the session bus is established. I presume it would take longer on a slower machine. The current code in widget/src/gtk2 records the result in a static variable, so there it would only make the call once, but, regardless, I agree that it is better to only have the code in one place if possible, so I'm testing with the duplicate code removed.

Comment 8

[Mike Gorse]

Attachment: Revised patch.

Remove duplicate code; add method in nsIAccessibilityService to test whether
a11y is enabled.
Also, unref the dbus connection when we're done with it.

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 10

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

Comment on attachment 572408 [details] [diff] [review]
patch

Review of attachment 572408 [details] [diff] [review]:
-----------------------------------------------------------------

Please don't forget to add mgorse as an author if you've built on his work.

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1845,5 @@
>  }
> +
> +#ifdef MOZ_ACCESSIBILITY_ATK
> +bool
> +ShouldA11yBeEnabled()

Would probably be better to move this into our atk layer no?

::: accessible/src/base/nsAccessibilityService.h
@@ +58,5 @@
>  
> +/**
> + * Is platform accessibility enabled.
> + */
> +bool ShouldA11yBeEnabled();

Despite the name is Linux/dbus specific right?

Comment 11

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

Try server doesn't seem to have debus?
../../../dist/system_wrappers/dbus/dbus.h:3:28: fatal error: dbus/dbus.h: No such file or directory

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 13

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> Created attachment 572525 [details] [diff] [review] [diff] [details] [review]
> patch

I can't compile with that patch. Can't seem to find the dbus/dbus.h header. There is probably a magic trick I forgot about.

Comment 14

[Hubert Figuiere [:hub]]

Comment on attachment 572525 [details] [diff] [review]
patch

Review of attachment 572525 [details] [diff] [review]:
-----------------------------------------------------------------

With this patch I can't compile.

we need to add access to the dbus headers in accessible/src/base/Makefile.in

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1837,5 @@
>  ////////////////////////////////////////////////////////////////////////////////
>  
> +namespace mozilla {
> +namespace a11y {
> +  mozilla::a11y::FocusManager*

We should remove the namespace qualifier here since we are inside it.

@@ +1842,1 @@
>  mozilla::a11y::FocusMgr()

and here

@@ +1842,5 @@
>  mozilla::a11y::FocusMgr()
>  {
>    return nsAccessibilityService::gAccessibilityService;
>  }
> +

We are missing 
}
}

to close the namespaces above.

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: PATCH

NO IDEA WHAT WENT WRONG WITH THE LAST PATCH, i WAS SURE IT  COMPILED LOCALLY, BUT THAT MAKES NO SENSE, i JUST HAVE NO IDEA.

Comment 16

[Trevor Saunders (:tbsaunde)]

Comment on attachment 572602 [details] [diff] [review]
PATCH

>+
>diff --git a/accessible/src/base/nsAccessibilityService.cpp b/accessible/src/base/nsAccessibilityService.cpp
>--- a/accessible/src/base/nsAccessibilityService.cpp
>+++ b/accessible/src/base/nsAccessibilityService.cpp
>@@ -81,6 +81,7 @@
> #include "nsTextFragment.h"
> #include "mozilla/Services.h"
> #include "nsEventStates.h"
>+#include "prenv.h"

artifact will remove before landing

> #ifdef MOZ_XUL
> #include "nsXULAlertAccessible.h"
>@@ -1832,8 +1833,9 @@
> // Services
> ////////////////////////////////////////////////////////////////////////////////
> 
>-mozilla::a11y::FocusManager*
>+  mozilla::a11y::FocusManager*
> mozilla::a11y::FocusMgr()

ugh, this not my day :(

Comment 17

[Hubert Figuiere [:hub]]

Comment on attachment 572602 [details] [diff] [review]
PATCH

Review of attachment 572602 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
@@ +912,5 @@
> +  switch (dbus_message_iter_get_arg_type (&iter_variant)) {
> +    case DBUS_TYPE_STRUCT:
> +      // at-spi2-core 2.2.0-2.2.1 had a bug where it returned a struct
> +      dbus_message_iter_recurse (&iter_variant, &iter_struct);
> +      if (dbus_message_iter_get_arg_type (&iter_struct) != DBUS_TYPE_BOOLEAN) {

Did you mean == DBUS_TYPE_BOOLEAN instead?

Comment 18

[Hubert Figuiere [:hub]]

Comment on attachment 572602 [details] [diff] [review]
PATCH

Review of attachment 572602 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsAccessibilityService.cpp
@@ +81,4 @@
>  #include "nsTextFragment.h"
>  #include "mozilla/Services.h"
>  #include "nsEventStates.h"
> +#include "prenv.h"

is this needed?

@@ +1833,4 @@
>  // Services
>  ////////////////////////////////////////////////////////////////////////////////
>  
> +  mozilla::a11y::FocusManager*

I don't see the need for the indentation. just nitpicking here.

::: widget/src/gtk2/nsWindow.cpp
@@ +1110,4 @@
>      }
>  
>  #ifdef ACCESSIBILITY
> +    if (aState && a11y::ShouldA11yBeEnabled())

in nsAccessibilityService.h, ShouldA11yBeEnabled() is declared only we use ATK, while here it seems to be for the ACESSIBILITY.

@@ +6475,4 @@
>  void
>  nsWindow::DispatchEventToRootAccessible(PRUint32 aEventType)
>  {
> +    if (!a11y::ShouldA11yBeEnabled())

same as above.

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #18)
> Comment on attachment 572602 [details] [diff] [review] [diff] [details] [review]
> PATCH
> 
> Review of attachment 572602 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessibilityService.cpp
> @@ +81,4 @@
> >  #include "nsTextFragment.h"
> >  #include "mozilla/Services.h"
> >  #include "nsEventStates.h"
> > +#include "prenv.h"
> 
> is this needed?
> 
> @@ +1833,4 @@
> >  // Services
> >  ////////////////////////////////////////////////////////////////////////////////
> >  
> > +  mozilla::a11y::FocusManager*
> 
> I don't see the need for the indentation. just nitpicking here.

yeah, see my earlier comment, these two are fixed locally, I can upload a new patch if you like.

> ::: widget/src/gtk2/nsWindow.cpp
> @@ +1110,4 @@
> >      }
> >  
> >  #ifdef ACCESSIBILITY
> > +    if (aState && a11y::ShouldA11yBeEnabled())
> 
> in nsAccessibilityService.h, ShouldA11yBeEnabled() is declared only we use
> ATK, while here it seems to be for the ACESSIBILITY.

I guess saying its only for atk is a little misleading, its really for desktop linux where we use atk / at-spi

However there is a sort of implicit assumption that the gtk backend will be related to using atk accessibility (which is sort of reasonable since gtk uses atk internally).  

Another thing is that for atk we need general accessibility to suport atk.

I'd be willing to improve the comment if you have ideas.

Comment 20

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

Comment on attachment 572602 [details] [diff] [review]
PATCH

Review of attachment 572602 [details] [diff] [review]:
-----------------------------------------------------------------

More dbus usage ...

Comment 21

[Trevor Saunders (:tbsaunde)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 572602 [details] [diff] [review] [diff] [details] [review]
> PATCH
> 
> Review of attachment 572602 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> More dbus usage ...

true... I'm not saying I like it, but I think we need to get used to linux a11y using it.  We haven't really had to deal with it since its all been hidden behind atk so far, but at-spi2 is all dbus and people have been using firefox a11y with at-spi2 for a while now, and we will need to drop support for at-spi over corba  for e10s because of atk plug / socket only being available in at-spi2.   A fall back of multiple accessible trees has been discused, but I'm not particularly interested personally.  Finally whatever we think of it I doubt there's much we can do to change that at-spi2 is the future.

All that said other ideas welcome!

Comment 22

[Hubert Figuiere [:hub]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 572602 [details] [diff] [review] [diff] [details] [review]
> PATCH
> 
> Review of attachment 572602 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> More dbus usage ...

To me it looks to be the saner way to do it without mandating Gnome3. Gsettings being worse.

Comment 23

[Trevor Saunders (:tbsaunde)]

Attachment: patch with nits fixed

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to Trevor Saunders (:tbsaunde) from comment #21)
(In reply to Hub Figuiere [:hub] from comment #22)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)

> > More dbus usage ...

I think roc was talking to me with this comment.

DBus definitely sounds preferable to GSettings.
(And DBus sounds like the right way to do GNOME 3 desktop settings stored with GSettings.)

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to Karl Tomlinson (:karlt) from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> (In reply to Hub Figuiere [:hub] from comment #22)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> 
> > > More dbus usage ...
> 
> I think roc was talking to me with this comment.
> 
> DBus definitely sounds preferable to GSettings.
> (And DBus sounds like the right way to do GNOME 3 desktop settings stored
> with GSettings.)

Oh, Ok, ftr I'm pretty confident in the dbus code at this point, I haven't really changed it much just try and  make it more gecko styley and it looks fine to me.  I beleive Hub has dealt with dbus some  it seems like he thinks its reasonable too.   Finally at-spi2 and the  atk-bridge for dbus is basically Mike's  at this point so I generally trust the dbus code he writes.
any chance you can review this soon?  It's a bit of a usability problem since currently as the bug says firefox appears to be inaccessible in modern linux installs.

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 572700 [details] [diff] [review]
patch with nits fixed

Can you provide function names and 8 lines of context with future patches,
please?  https://developer.mozilla.org/en/Installing_Mercurial#Configuration

>+  DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error);

dbus_bus_get() will sometimes call
dbus_connection_set_exit_on_disconnect(TRUE), so that needs to be undone with
another dbus_connection_set_exit_on_disconnect().

>+  reply = dbus_connection_send_with_reply_and_block(bus, message, 1000, &error);

We are actively trying to shorten the start-up path to reduce start-up time.
Blocking on DBus during creation of the first window wouldn't be helpful.

Is it feasible to initialize on receipt of an async reply?

Or I expect it would be possible to create a DBusPendingCall on window
creation, and then only block on it and steal_reply when it is really needed?

(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> It's a bit of a usability problem
> since currently as the bug says firefox appears to be inaccessible in modern
> linux installs.

I thought GNOME 3 distros were at least providing a read-only GConf wrapper.

Comment 27

[Trevor Saunders (:tbsaunde)]

(In reply to Karl Tomlinson (:karlt) from comment #26)
> Comment on attachment 572700 [details] [diff] [review] [diff] [details] [review]
> patch with nits fixed
> 
> Can you provide function names and 8 lines of context with future patches,
> please?  https://developer.mozilla.org/en/Installing_Mercurial#Configuration
> 
> >+  DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error);
> 
> dbus_bus_get() will sometimes call
> dbus_connection_set_exit_on_disconnect(TRUE), so that needs to be undone with
> another dbus_connection_set_exit_on_disconnect().

... great

> >+  reply = dbus_connection_send_with_reply_and_block(bus, message, 1000, &error);
> 
> We are actively trying to shorten the start-up path to reduce start-up time.
> Blocking on DBus during creation of the first window wouldn't be helpful.
> 
> Is it feasible to initialize on receipt of an async reply?

I'm not sure how bad this is in practice, but I  don't forsee any huge problem with using pending calls to do this async.

> (In reply to Trevor Saunders (:tbsaunde) from comment #25)
> > It's a bit of a usability problem
> > since currently as the bug says firefox appears to be inaccessible in modern
> > linux installs.
> 
> I thought GNOME 3 distros were at least providing a read-only GConf wrapper.

tbh I'm not sure, but I know we had several people confused about the problem

Comment 28

[Hubert Figuiere [:hub]]

Comment on attachment 572700 [details] [diff] [review]
patch with nits fixed

Review of attachment 572700 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 572700 [details] [diff] [review]
patch with nits fixed

Review of attachment 572700 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not looking into the platform specific logic since I bet you did that well, just integration part.
canceling review until comments are addressed

::: accessible/src/atk/nsApplicationAccessibleWrap.cpp
@@ +612,4 @@
>  bool
>  nsApplicationAccessibleWrap::Init()
>  {
> +    if (ShouldA11yBeEnabled()) {

if application accessible is initialized then accessibility must be enabled. What's a reason of this check?

@@ +862,5 @@
>      return NS_OK;
>  }
> +
> +namespace mozilla {
> +  namespace a11y {

nit: no indent for a11y namespace

@@ +865,5 @@
> +namespace mozilla {
> +  namespace a11y {
> +
> +bool
> +ShouldA11yBeEnabled()

that's weird something prototyped in nsAccessibilityService gets defined in nsApplicationAccessibleWrap. I can't think of better place but you should XXX comment in prototype I think

@@ +869,5 @@
> +ShouldA11yBeEnabled()
> +{
> +  static bool sChecked = false, sShouldEnable = false;
> +  if (sChecked)
> +    return sShouldEnable;

that's ok but curious why wouldn't use enum instead?

@@ +928,5 @@
> +    default:
> +      break;
> +  }
> +
> +  dbus_done:

goto is unusual style and it's not appreciated in c++ world in general. I'm fine if you're sure to keep it

@@ +955,5 @@
> +    do_GetService(sSysPrefService, &rv);
> +  if (NS_SUCCEEDED(rv) && sysPrefService)
> +    sysPrefService->GetBoolPref(sAccessibilityKey, &sShouldEnable);
> +
> +    return sShouldEnable;

nit: wrong indentation

::: accessible/src/base/nsAccessibilityService.h
@@ +57,5 @@
>  FocusManager* FocusMgr();
>  
> +/**
> + * Is platform accessibility enabled.
> + * only used on linux with atk for now.

nit: o -> O

@@ +61,5 @@
> + * only used on linux with atk for now.
> + */
> +#ifdef MOZ_ACCESSIBILITY_ATK
> +bool ShouldA11yBeEnabled();
> +#endif

nit: put comment into #ifdef please

::: widget/src/gtk2/nsWindow.cpp
@@ +6479,1 @@
>          return;

it appears nsWindow::Show() manages root accessible creation, so here you should check only if accessibility service instantiated.

and then you have unique consumer of ShouldA11yBeEnabled (nsWindow::Show), maybe it's worth to keep this method somewhere in gtk2 code

Comment 30

[alexander :surkov (:asurkov)]

(In reply to alexander surkov from comment #29)

> >  bool
> >  nsApplicationAccessibleWrap::Init()
> >  {
> > +    if (ShouldA11yBeEnabled()) {
> 
> if application accessible is initialized then accessibility must be enabled.
> What's a reason of this check?

I see you do that because of bug 390761.

Comment 31

[Mike Gorse]

Attachment: patch #7

Add a PreInit() function to be called on window creation, that establishes a
D-Bus connection and makes a pending call to determine whether accessibility
is enabled. Have ShouldA11yBeEnabled block on this call if needed. Hopefully
this will reduce start-up time slightly. (In theory we could probably
only enable a11y on a response to the pending call, but this would be much
more of an architectural change, and we would also need to keep in mind that
currently we are still supporting gconf as a callback, so this approach
is a lot simpler.)

Also, call dbus_connection_set_exit_on_disconnect, and fix some nits.

Comment 32

[Mike Gorse]

Attachment: Updated patch.

Update to apply against current source.
Add a couple of MOZ_A11Y_DBUS checks where they were missing.
Remove some includes that are no longer needed.

Comment 33

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Gorse from comment #32)
> Created attachment 577444 [details] [diff] [review] [diff] [details] [review]
> Updated patch.

You didn't get the merge  with bug 451161 right, you should probably check the use system pref first then dbus or gconf.

I'm also thinking it might be nice to put this stuff in a new file, Alex thoughts?

common nit, I tend to prefer

if (x)
  return;

foo();

to

if (x)
  return;
foo();

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #33)

> I'm also thinking it might be nice to put this stuff in a new file, Alex
> thoughts?

new file might be nice but actually I'd like to figure out general concept how we should handle initialization. I filed bug 706051 for that.

Comment 35

[Mike Gorse]

Attachment: Updated patch.

Merge with 451161/705983.

Comment 36

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581750 [details] [diff] [review]
Updated patch.


>+PreInit()
>+{
>+#ifdef MOZ_ENABLE_DBUS
>+  static bool sChecked = FALSE;
>+  if (sChecked)
>+    return;
>+  sChecked = TRUE;

nit, blank line before sChecked = true


I'm tempted to think we should bail if GNOME_ACCESSIBILITY is set since we should bail after this if GNOME_ACCESSIBILITY is set, otherwise nobody will ever check the return message.

>+  if (!bus)
>+    return;
>+  dbus_connection_set_exit_on_disconnect(bus, FALSE);

nit, blank line.

>+  if (a11yPendingCall) {

nit,if ( !a11yPendingCall) goto dbus_done;

also we don't usually put a11y in local variables.

>+        dbusSuccess = true;
>+      }
>+
>+      break;

nit, put break in block above.

>diff --git a/widget/src/gtk2/nsWindow.cpp b/widget/src/gtk2/nsWindow.cpp
> #ifdef ACCESSIBILITY
>-#include "nsIAccessibilityService.h"
>+#include "nsAccessibilityService.h"
> #include "nsIAccessibleDocument.h"
>-#include "prenv.h"
>-#include "stdlib.h"
> 
> using namespace mozilla;
> using namespace mozilla::widget;

btw while I suppose ti doesn't really happen since these are present later unconditionally why are these here?

Comment 37

[Trevor Saunders (:tbsaunde)]

> @@ +928,5 @@
> > +    default:
> > +      break;
> > +  }
> > +
> > +  dbus_done:
> 
> goto is unusual style and it's not appreciated in c++ world in general. I'm
> fine if you're sure to keep it

This code is pretty "Cish"  because of the dbus api, and its a reasonably common view that gotos like this are good style for the special case of error handling in C.

> ::: widget/src/gtk2/nsWindow.cpp
> @@ +6479,1 @@
> >          return;
> 
> it appears nsWindow::Show() manages root accessible creation, so here you
> should check only if accessibility service instantiated.

you mean nsAccessibilityService::GetAccService() is non-null right?

Comment 38

[Karl Tomlinson (:karlt)]

Comment on attachment 581750 [details] [diff] [review]
Updated patch.

Thank you for tidying this up and using the async API to do this in parallel.
This approach looks good to me.

I wonder whether the PreInit() in ShouldA11yBeEnabled() is really necessary or
could be replaced with an assertion to check that PreInit had already been
called.  Perhaps it is needed for unit tests?

>+static DBusPendingCall *a11yPendingCall = NULL;

I'm not familiar with the naming conventions in this particular code, but
usually static variables in Gecko have an "s" prefix.

>+  DBusError error;
>+  dbus_error_init(&error);
>+  DBusConnection* bus = dbus_bus_get(DBUS_BUS_SESSION, &error);
>+  if (!bus)
>+    return;

error is not actually used here, and so can be replaced with NULL.
"By convention, all functions allow NULL instead of a DBusError*, so callers who don't care about the error can ignore it."

>+  dbus_connection_send_with_reply(bus, message, &a11yPendingCall, 1000);
>+
>+dbus_done:
>+  if (message)
>+    dbus_message_unref(message);

I would move the dbus_message_unref to immediately after the send, before
dbus_done, so that the "if (message)" check is not required.

>+  if (bus)
>+    dbus_connection_unref(bus);

bus is always non-NULL here, so no need for the "if (bus)" check.

>+      strcmp(dbus_message_get_signature (reply), "v"))

Use DBUS_TYPE_VARIANT_AS_STRING instead of "v".

>+  dbus_done:
>+  if (reply)

Usually labels are "out"dented to make them stand out.

>-    if (aState && sAccessibilityEnabled) {
>+    if (aState && a11y::ShouldA11yBeEnabled())
>         CreateRootAccessible();
>-    }

Please don't unbrace the block here.  Convention in this file is heading
towards always brace except for jump statements, but usually leave existing code as is.

What does CreateRootAccessible() actually achieve?  It looks like
mRootAccessible is unused.  Is it holding a reference so that
DispatchEventToRootAccessible will somehow not need to create another
nsAccessible?

(In reply to Trevor Saunders (:tbsaunde) from comment #36)
> Comment on attachment 581750 [details] [diff] [review]
 
> >+        dbusSuccess = true;
> >+      }
> >+
> >+      break;
> 
> nit, put break in block above.

The "break" needs to be out of the block so as not to fall through to the DBUS_TYPE_BOOLEAN case when the type doesn't match, but the blank line there looks a bit odd to me.

Comment 39

[Trevor Saunders (:tbsaunde)]

landed
https://hg.mozilla.org/mozilla-central/rev/860fdd41cfed
https://hg.mozilla.org/mozilla-central/rev/887abec76bca

Comment 40

[Ed Morley [:emorley]]

Backed out per #developers:
https://hg.mozilla.org/mozilla-central/rev/6af20fb62fb7

https://tbpl.mozilla.org/php/getParsedLog.php?id=8060865&tree=Firefox

Comment 42

[Karl Tomlinson (:karlt)]

The tinderbox logs are no longer found at the link above.

If there is no DBus daemon already running, I've seen DBus spawn a child process that doesn't properly disconnect from ssh sessions; I wonder whether something similar might be happening here.

These IRC comments from http://bugzilla.glob.com.au/irc/?c=developers&a=date&s=2011-12-20&e=2011-12-21&h= may be helpful.

<Ms2ger	philor, fwiw, tbsaunde says he was green on try earlier
<tbsaunde	https://tbpl.mozilla.org/?tree=Try&rev=083fd66ad8f2 fwiw

* philor loads up the try log
<philor>	not that I don't trust try to run the same things that m-c runs, but, well, I don't trust to try to run the same things

<philor>	interesting: try still does a couple of rather foolish and pointless steps, that wind up creating a profile which isn't actually used, and that's apparently enough
<Ms2ger>	philor, the first word I can think of to describe that is "Mozilla"
<philor>	so my wild guess, having been awake for 20 minutes, is that the patch introduces a shutdown hang or at least a shutdown really-slow, and try survives it by being foolish

<philor>	"firefox-bin -no-remote -profile /builds/slave/try-lnx64-dbg/build/obj-firefox/_leaktest/leakprofile/ http://localhost:8888/bloatcycle.html -P default"
<philor>	no idea how you could be saved by that bit of foolishness, either

<philor>	tbsaunde: in theory, build with ac_add_options --enable-debug and ac_add_options --enable-trace-malloc, cd objdir/_leaktest/, python leaktest.py, then python leaktest.py --trace-malloc malloc.log --shutdown-leaks=sdleak.log should repro

<philor>	tbsaunde: no idea who would give you a stack, since I don't even know what's happening - leaktest.py starts up an http server, runs the browser, according to the log the browser closed, according to the exit code leaktest.py closed and closed happy, but when it tries to run again, it's still running from before

Comment 43

[Mike Gorse]

It might help to disable the dbus check if DBUS_SESSION_BUS_ADDRESS is unset, if Karl's guess is right.

Comment 44

[Karl Tomlinson (:karlt)]

That was really just guessing in comment 42, but would it make sense to skip the check if DBUS_SESSION_BUS_ADDRESS is unset anyway?
If there is no session, then I assume there will be no "org.a11y.Bus".

Comment 45

[Trevor Saunders (:tbsaunde)]

(In reply to Karl Tomlinson (:karlt) from comment #44)
> That was really just guessing in comment 42, but would it make sense to skip
> the check if DBUS_SESSION_BUS_ADDRESS is unset anyway?
> If there is no session, then I assume there will be no "org.a11y.Bus".

I was  thinking the same thing, but wasn't sure if that variable was required to have a useful dbus session.  Since I haven't reproduced this locally and it seemed worth a shot I pushed https://hg.mozilla.org/projects/accessibility/rev/e95cb5b98154  lets see how it does.

Comment 46

[Trevor Saunders (:tbsaunde)]

> I wonder whether the PreInit() in ShouldA11yBeEnabled() is really necessary
> or
> could be replaced with an assertion to check that PreInit had already been
> called.  Perhaps it is needed for unit tests?

I don't think I'm familiar enough with  when things happen in widget/ to be sure that call to PreInit() will come first.

> >-    if (aState && sAccessibilityEnabled) {
> >+    if (aState && a11y::ShouldA11yBeEnabled())
> >         CreateRootAccessible();
> >-    }
> 
> Please don't unbrace the block here.  Convention in this file is heading
> towards always brace except for jump statements, but usually leave existing
> code as is.

ok :(

> 
> What does CreateRootAccessible() actually achieve?  It looks like
> mRootAccessible is unused.  Is it holding a reference so that
> DispatchEventToRootAccessible will somehow not need to create another
> nsAccessible?

well, msot of that code came before me, but the only current purpose I can think of is to not fire the event if we don't need to since the root accessible is already created. Of course using a ref to the root accessible seems a silly way to do that, maybe Ginn or Alex knows more.  I suspect a spin off bug to clean up thi would make sense, I'll gues this code hasn't been worked on in a good while for the most part. 

I suppose its fairly clear though that the main purpose is to cause a11y to create a root accessible.
>

Comment 47

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #44)
> If there is no session, then I assume there will be no "org.a11y.Bus".

Actually there probably can still be a "org.a11y.Bus" if registered in /usr/share/dbus-1/services/
http://www.freedesktop.org/wiki/IntroductionToDBus#Activation

But it looks like the problem is resolved, so perhaps this workaround can be used until a better solution is found.

Comment 48

[Chris Coulson]

Yes, both libdbus and GDBus (in gio) will spawn a new bus with dbus-launch if it can't connect to a session bus (ie, if DBUS_SESSION_BUS_ADDRESS is unset).

I've just been bitten by this in Ubuntu with IPC xpcshell tests hanging our builds. I did some investigation of this today and found that the hangs were caused by plugin-container failing to start because it couldn't get an X connection - because of the bazillion or so dbus-launch processes (and associated buses!!) that had been spawned during the previous tests had used up all of the allowable X server connections :(

To work around this, we'll probably run all of the tests inside dbus-launch so that they have a bus already (eg, dbus-launch --exit-with-session)

Comment 49

[Trevor Saunders (:tbsaunde)]

(In reply to Karl Tomlinson (:karlt) from comment #47)
> (In reply to Karl Tomlinson (:karlt) from comment #44)
> > If there is no session, then I assume there will be no "org.a11y.Bus".
> 
> Actually there probably can still be a "org.a11y.Bus" if registered in
> /usr/share/dbus-1/services/
> http://www.freedesktop.org/wiki/IntroductionToDBus#Activation
> 
> But it looks like the problem is resolved, so perhaps this workaround can be
> used until a better solution is found.

well, the firstthing that comes to mind is arranging for build machines and test machines to have GNOME_ACCESSIBILITY set, do we have an idea how hard this is?

I'd expect though that in the vast majority of cases  that there is a session and this is not relavent. Karl would you  object if I merge that change set?  (with nits if you have any fixed), I should atleast look at the length of the check I added since it may well be over 80 chars.

Comment 50

[Karl Tomlinson (:karlt)]

(In reply to Trevor Saunders (:tbsaunde) from comment #49)
> well, the firstthing that comes to mind is arranging for build machines and
> test machines to have GNOME_ACCESSIBILITY set, do we have an idea how hard
> this is?

It would be good to ensure there is a session bus, because we'd like to test the dbus code.  Perhaps DBUS_SESSION_BUS_ADDRESS is not set because of the way our testing connects to the existing gnome session.  Or perhaps the CentOS 5.0 machines are just too old to have a session bus running.  On these systems the GConf code can provide information on whether accessibility is enabled.
If the test slaves were fine on comment 39's landing and it was only build machines that had failure, then that might be because the tests slaves are running a newer OS, one likely to have a DBus session, and DBUS_SESSION_BUS_ADDRESS may be set there.

You might need to ask someone from releng how our build/test slave starts up and whether that inherits environment variables from the gnome session.

If you only need accessibility enabled for accessibility tests then it probably makes sense to set GNOME_ACCESSIBILITY in the Makefile command that runs the accessiblity tests.

> I'd expect though that in the vast majority of cases  that there is a
> session and this is not relavent. Karl would you  object if I merge that
> change set?

No I would not object.  Please add a comment to explain why the DBUS_SESSION_BUS_ADDRESS environment variable is there though.

The situation with dbus when it can't connect to a session bus is unfortunate, and perhaps something worth avoiding anyway, if we can.

Comment 51

[Trevor Saunders (:tbsaunde)]

(In reply to Karl Tomlinson (:karlt) from comment #50)
> (In reply to Trevor Saunders (:tbsaunde) from comment #49)
> > well, the firstthing that comes to mind is arranging for build machines and
> > test machines to have GNOME_ACCESSIBILITY set, do we have an idea how hard
> > this is?
> 
> It would be good to ensure there is a session bus, because we'd like to test
> the dbus code.  Perhaps DBUS_SESSION_BUS_ADDRESS is not set because of the

We don't have any tests right now that make sure that  this code works properly afaik, or for that matter any of our  platform code works.

> way our testing connects to the existing gnome session.  Or perhaps the
> CentOS 5.0 machines are just too old to have a session bus running.  On

Is there even a gnome session to speak of?

> If you only need accessibility enabled for accessibility tests then it
> probably makes sense to set GNOME_ACCESSIBILITY in the Makefile command that
> runs the accessiblity tests.
I believe the current state to be that accessibility is only on for a11y tests where it is turned on through xpcom instead of using platform mechanisms

> > I'd expect though that in the vast majority of cases  that there is a
> > session and this is not relavent. Karl would you  object if I merge that
> > change set?
> 
> No I would not object.  Please add a comment to explain why the
> DBUS_SESSION_BUS_ADDRESS environment variable is there though.

sure

> 
> The situation with dbus when it can't connect to a session bus is
> unfortunate, and perhaps something worth avoiding anyway, if we can.

yeah, I'd expect we'd spend a bit of time blocking though that's a guess.

Comment 52

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/mozilla-central/rev/e95cb5b98154
https://hg.mozilla.org/mozilla-central/rev/39acd9b60ebc

Comment 53

[Mike Gorse]

Comment on attachment 581750 [details] [diff] [review]
Updated patch.

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Accessibility will not be enabled on a GNOME 3 system, unless the user manually sets the accessibility gconf key (used by GNOME 2 but not 3, and thus not generally set under GNOME 3).

Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
The old behavior of checking gconf will be used if the dbus call does not succeed (ie, if AT-SPI2 is not installed), so I think that the risk is small.
An alternative approach would be to modify the start-up script to call dbus-send and set GNOME_ACCESSIBILITY=1 if the call indicates that accessibility is enabled, but this would likely affect start-up time more than applying the patch would, so applying the patch seems preferable.

Comment 54

[Trevor Saunders (:tbsaunde)]

> [Approval Request Comment]
> Regression caused by (bug #): 

no bug, gnome's move away from gconf

> Testing completed (on m-c, etc.): 
> Risk to taking this patch (and alternatives if risky):
> The old behavior of checking gconf will be used if the dbus call does not
> succeed (ie, if AT-SPI2 is not installed), so I think that the risk is small.

The  it is theoretically possible this could somehow cause a11y to be enabled by accident which would cause a perf regression, however I would evaluate the probaility of this as low, and if it was a problem I would expect it to be a fairly general problem which we should have already heard about.

> An alternative approach would be to modify the start-up script to call
> dbus-send and set GNOME_ACCESSIBILITY=1 if the call indicates that
> accessibility is enabled, but this would likely affect start-up time more
> than applying the patch would, so applying the patch seems preferable.

The shell script went away,  so this might not be a possibility, and it would be completely new untested code.
The regression is pretty significant (we've seen mails from people wondering why firefox is no longer accessible), and since Fire Fox 10  will be an esr and so around for a while to some degree, I'll do my best to take Surkov's place (he's out)  and say I think we want to take this.

Comment 55

[Alex Keybl [:akeybl]]

Comment on attachment 581750 [details] [diff] [review]
Updated patch.

[Triage Comment]
When evaluating the risk (perf regressions) vs reward (fixing an issue affecting accessibility users on GNOME 3), we've decided to instead release note for FF10/11.

Comment 56

[Nolan Darilek]

Are you freakin' serious?!?

I was just thinking the other day about how Mozilla didn't seem to care much about Firefox Linux accessibility. As a blind Android developer and user, I find it difficult to impossible to use any of the advanced features on Google's web-based market. Similarly, Google Docs and other products are still very inaccessible under Linux, even with their screen reader modifications turned on.

In Firefox 9, I now discover that I can't reliably move focus. Focus gets stuck on a variety of page elements, just as it did in earlier versions. This is clearly a regression, and a huge one at that.

And now I learn that Firefox will remain inaccessible on default GNOME 3 installations for the next four and a half months?

"File bugs on your issues" is all well and good if I feel like they're cared about, but I don't. And we're now clearly seeing how a critical accessibility fix, which is already available on some Linux versions, is being pushed back on because apparently no accessibility for some users is better than the likelihood that said accessibility might not work completely.

It's interesting how Mozilla is about choice, yet under Linux I have none. As soon as Chrome/Chromium or any other WebKit browser becomes a viable option, I intend to exercise choice and pick another non-Mozilla browser so I can have a web experience that isn't bad on any advanced application. I keep up with Planet Mozilla and other news sources, and lots of those messages just seem empty and meaningless when I get news like this.

Let's get this in sooner, OK? And let's show people like me how Mozilla truly does care about its users who have chosen free operating systems. As it stands, Firefox loses accessibility ground steadily, and that Mozilla categorizes doing the right thing as a reward rather than a necessity is a pretty big sign that I'd better start using and advocating for another browser as soon as I am free to do so. It sucks when an organization gets too big to care that a significant number of users can't use their product at all.

Comment 57

[Karl Tomlinson (:karlt)]

If distributions are no longer supporting the means by which applications determine whether accessibility is enabled, then that is not a reason to vent frustration at the application developers.

Supporting the new methods for detecting settings is by definition new and therefore not thoroughly tested.  It seems reasonable to put this through the same QA process as any new feature.

Comment 58

[Nolan Darilek]

No longer supporting? I'd say they *are* being supported if every other application on my GNOME 3 system is identified as accessible. There is definitely a way to detect the availability of accessibility, and a way that is supported by the platform developers.

And I'd hardly call something that has been available and in use for nearly a year new, especially in the area of software.

This is not a new feature. This is a critical regression that makes it difficult to impossible for GNOME 3 users to access your application. It's hugely disheartening to see an app that already has some major accessibility issues ask its users to wait months without any access whatsoever when a fix is available and won't impact those who don't need it.

Comment 59

[alexander :surkov (:asurkov)]

(In reply to Karl Tomlinson (:karlt) from comment #57)
> Supporting the new methods for detecting settings is by definition new and
> therefore not thoroughly tested.  It seems reasonable to put this through
> the same QA process as any new feature.

It's not a feature that's serious regression. the patch is on trunk for a while and there's no any known regression. I suggest to ask for aurora approval.

Comment 60

[alexander :surkov (:asurkov)]

Comment on attachment 581750 [details] [diff] [review]
Updated patch.

serious regression, low risk, running on trunk for several days, no known regression from this patch. rerequest aurora approval.

Comment 61

[Alex Keybl [:akeybl]]

(In reply to alexander :surkov from comment #60)
> Comment on attachment 581750 [details] [diff] [review]
> Updated patch.
> 
> serious regression, low risk, running on trunk for several days, no known
> regression from this patch. rerequest aurora approval.

Is there any alternative for distros to work around this issue?

Comment 62

[Chris Coulson]

For distributions based on GNOME 3.4, I don't think there is really a good workaround.

For distributions based on GNOME 3.2, this shouldn't be a problem because there is a GSettings -> GConf bridge which works fine (well, it works fine in Ubuntu at the moment. I'm not sure about other distros). However, this got dropped for GNOME 3.4 here: http://git.gnome.org/browse/gnome-settings-daemon/commit/?id=f4ef02038a1f57edf7a23ba91829f15cddfc7eff

Comment 63

[Alex Keybl [:akeybl]]

Comment on attachment 581750 [details] [diff] [review]
Updated patch.

[Triage Comment]
Given the lack of a viable workaround in the FF11 timeframe, let's uplift to Aurora 11.

Comment 64

[Trevor Saunders (:tbsaunde)]

landed https://hg.mozilla.org/releases/mozilla-aurora/rev/c0df4333b5b7

Comment 65

[Maniac Vlad Florin (:vladmaniac)]

Verified with gnome-shell --version = GNOME Shell 3.2.1

Firefox is responsive to Accessibility apps -- See  https://launchpad.net/bugs/857153  	

GNOME_ACCESSIBILITY not manually set or any option in gconf