Needs to get accessibility settings from GSettings

Bug #857153 reported by Chris Coulson
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Mozilla Firefox
Fix Released
High
firefox (Ubuntu)
Fix Released
High
Chris Coulson
Oneiric
Fix Released
High
Chris Coulson
Precise
Fix Released
High
Chris Coulson

Bug Description

Luke mentioned to me in Dublin that the screen reader doesn't work with Firefox when accessibility is enabled. This is because Firefox is still using GConf to check if accessibility is enabled, when it really needs to be using GSettings. I've just realized this is still the case, so I assume that the screen reader still won't work by default

description: updated
Changed in firefox (Ubuntu Oneiric):
importance: Undecided → High
status: New → Triaged
assignee: nobody → Chris Coulson (chrisccoulson)
milestone: none → ubuntu-11.10
Changed in firefox (Ubuntu Oneiric):
milestone: ubuntu-11.10 → oneiric-updates
tags: added: rls-mgr-o-tracking
Revision history for this message
Luke Yelavich (themuso) wrote :

Its worth noting that users who install with the screen reader accessibility profile will still have a working firefox, as the profile enables accessibility in gconf as well.

Revision history for this message
In , Mgorse (mgorse) wrote :

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.

Revision history for this message
In , Mgorse (mgorse) wrote :

Created attachment 568753
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.

Revision history for this message
In , Ginn-chen-r (ginn-chen-r) wrote :

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.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

Comment on attachment 568753
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?

Revision history for this message
In , Roc-ocallahan (roc-ocallahan) wrote :

Comment on attachment 568753
Proposed patch.

Review of attachment 568753:
-----------------------------------------------------------------

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 568753 [diff] [details] [review]
> Proposed patch.
>
> Review of attachment 568753 [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)

Revision history for this message
In , Mgorse (mgorse) wrote :

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

Revision history for this message
In , Mgorse (mgorse) wrote :

Created attachment 569898
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.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

Created attachment 572408
patch

Revision history for this message
In , Bolterbugz (bolterbugz) wrote :

Comment on attachment 572408
patch

Review of attachment 572408:
-----------------------------------------------------------------

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?

Revision history for this message
In , Bolterbugz (bolterbugz) wrote :

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

Created attachment 572525
patch

Revision history for this message
In , Hub-g (hub-g) wrote :

(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> Created attachment 572525 [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.

Revision history for this message
In , Hub-g (hub-g) wrote :

Comment on attachment 572525
patch

Review of attachment 572525:
-----------------------------------------------------------------

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.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

Created attachment 572602
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.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

Comment on attachment 572602
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 :(

Revision history for this message
In , Hub-g (hub-g) wrote :

Comment on attachment 572602
PATCH

Review of attachment 572602:
-----------------------------------------------------------------

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

Revision history for this message
In , Hub-g (hub-g) wrote :

Comment on attachment 572602
PATCH

Review of attachment 572602:
-----------------------------------------------------------------

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

(In reply to Hub Figuiere [:hub] from comment #18)
> Comment on attachment 572602 [diff] [details] [review]
> PATCH
>
> Review of attachment 572602 [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.

Revision history for this message
In , Roc-ocallahan (roc-ocallahan) wrote :

Comment on attachment 572602
PATCH

Review of attachment 572602:
-----------------------------------------------------------------

More dbus usage ...

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 572602 [diff] [details] [review]
> PATCH
>
> Review of attachment 572602 [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!

Revision history for this message
In , Hub-g (hub-g) wrote :

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

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

Created attachment 572700
patch with nits fixed

Revision history for this message
In , Karlt (karlt) wrote :

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

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

Revision history for this message
In , Karlt (karlt) wrote :

Comment on attachment 572700
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.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

(In reply to Karl Tomlinson (:karlt) from comment #26)
> Comment on attachment 572700 [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

Revision history for this message
In , Hub-g (hub-g) wrote :

Comment on attachment 572700
patch with nits fixed

Review of attachment 572700:
-----------------------------------------------------------------

This looks good to me.

Revision history for this message
In , Alexander Surkov (surkov-alexander) wrote :

Comment on attachment 572700
patch with nits fixed

Review of attachment 572700:
-----------------------------------------------------------------

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

Revision history for this message
In , Alexander Surkov (surkov-alexander) wrote :

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

Revision history for this message
In , Mgorse (mgorse) wrote :

Created attachment 575724
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.

Revision history for this message
In , Mgorse (mgorse) wrote :

Created attachment 577444
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.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

(In reply to Mike Gorse from comment #32)
> Created attachment 577444 [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();

Revision history for this message
In , Alexander Surkov (surkov-alexander) wrote :

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

Martin Pitt (pitti)
Changed in firefox (Ubuntu Precise):
milestone: oneiric-updates → ubuntu-12.04-beta-1
Revision history for this message
In , Mgorse (mgorse) wrote :

Created attachment 581750
Updated patch.

Merge with 451161/705983.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

Comment on attachment 581750
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?

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

> @@ +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?

Revision history for this message
In , Karlt (karlt) wrote :

Comment on attachment 581750
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

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :
Revision history for this message
In , Bmo-edmorley (bmo-edmorley) wrote :
Revision history for this message
In , Chris Coulson (chrisccoulson) wrote :

Currently, there is code in accessible/src/atk/nsApplicationAccessibleWrap.cpp and widget/src/gtk2/nsWindow.cpp to check if a11y is enabled by looking at the "/desktop/gnome/interface/accessibility" key in GConf. This doesn't work in GNOME 3 because of the transition to GSettings**.

This seems to be the last feature depending on GConf now.

** It may still work on some systems due to the GSettings -> GConf bridge, but this has been removed already (http://git.gnome.org/browse/gnome-settings-daemon/commit/?id=f4ef02038a1f57edf7a23ba91829f15cddfc7eff)

Revision history for this message
In , Chris Coulson (chrisccoulson) wrote :

Created attachment 584884
Look up accessibility settings from GSettings if available

Revision history for this message
In , Chris Coulson (chrisccoulson) wrote :

I'm not sure who to ask for review for this :)

Changed in firefox:
importance: Unknown → Medium
status: Unknown → Confirmed
Revision history for this message
In , Reed Loden (reed) wrote :

probably karlt for the gtk2 widget stuff, and maybe surkov or dbolter for the a11y stuff... or maybe just karlt alone.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

(In reply to Reed Loden [:reed] (very busy) from comment #3)
> probably karlt for the gtk2 widget stuff, and maybe surkov or dbolter for
> the a11y stuff... or maybe just karlt alone.

actually, me or Ginn Chen are probably the better a11y people here, and I think one of us should look at that sort of thing.

That said we're already working on this in bug 693343, if you'd like to help figure out the shutdown hang were having trouble with there that would be great!

*** This bug has been marked as a duplicate of bug 693343 ***

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

*** Bug 714198 has been marked as a duplicate of this bug. ***

Revision history for this message
In , Karlt (karlt) wrote :

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

Changed in firefox:
status: Confirmed → Invalid
Micah Gersten (micahg)
Changed in firefox:
importance: Medium → Unknown
status: Invalid → Unknown
Changed in firefox:
importance: Unknown → High
status: Unknown → Confirmed
Revision history for this message
In , Mgorse (mgorse) wrote :

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

Revision history for this message
In , Karlt (karlt) wrote :

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

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

Revision history for this message
In , Karlt (karlt) wrote :

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

Revision history for this message
In , Chris Coulson (chrisccoulson) wrote :

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)

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

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

Revision history for this message
In , Karlt (karlt) wrote :

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :
Changed in firefox:
status: Confirmed → Fix Released
Changed in firefox (Ubuntu Precise):
status: Triaged → Fix Committed
Changed in firefox (Ubuntu Oneiric):
status: Triaged → Fix Committed
Revision history for this message
In , Mgorse (mgorse) wrote :

Comment on attachment 581750
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.

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :

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

Revision history for this message
In , Akeybl (akeybl) wrote :

Comment on attachment 581750
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.

Revision history for this message
In , Nolan Darilek (nolan-thewordnerd) wrote :

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.

Revision history for this message
In , Karlt (karlt) wrote :

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.

Revision history for this message
In , Nolan Darilek (nolan-thewordnerd) wrote :

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.

Revision history for this message
In , Alexander Surkov (surkov-alexander) wrote :

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

Revision history for this message
In , Alexander Surkov (surkov-alexander) wrote :

Comment on attachment 581750
Updated patch.

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

Revision history for this message
In , Akeybl (akeybl) wrote :

(In reply to alexander :surkov from comment #60)
> Comment on attachment 581750
> 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?

Revision history for this message
In , Chris Coulson (chrisccoulson) wrote :

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

Revision history for this message
In , Akeybl (akeybl) wrote :

Comment on attachment 581750
Updated patch.

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

Revision history for this message
In , Trevor Saunders (trev-saunders) wrote :
Revision history for this message
Launchpad Janitor (janitor) wrote :
Download full text (4.1 KiB)

This bug was fixed in the package firefox - 11.0~b1+build1-0ubuntu1

---------------
firefox (11.0~b1+build1-0ubuntu1) precise; urgency=low

  * New upstream release from the beta channel (FIREFOX_11_0b1_BUILD1)
    - Fix LP: #875266 - Firefox ignores GNOME3 proxy settings
    - Fix LP: #857153 - Needs to get accessibility settings from GSettings

  * Update globalmenu-extension to 2.0.3
  * Ensure that the crash reporter is disabled if rebuilt by Ubuntu
    derivatives, as there will be no crash symbols for those
    - update debian/rules
  * Only add "Ubuntu" to the UA string when being built for Ubuntu
    - update debian/rules
  * Drop obsolete Debian menu file
    - remove debian/firefox.menu.in
    - don't create a 32x32 xpm icon in debian/rules
    - drop the imagemagick build-depend in debian/control
  * Temporarily disable ipdl tests due to build failures. These aren't
    enabled upstream, anyway
    - update debian/config/mozconfig.in
  * Always set the update channel - not setting it at build-time on release
    builds breaks the extensions.checkCompatibility pref. The only things
    using it at runtime are nsBlocklistService, Test Pilot (beta + aurora)
    and the about dialog (where the channel is hidden anyway)
    - update debian/rules
    - update debian/firefox.install.in
  * Don't declare an extra DEB_ENABLE_THUMB2 variable, as it's only used
    for the mozconfig. Just do the "if DEB_HOST_ARCH == armel" check
    directly there instead
    - update debian/rules
    - update debian/config/mozconfig.in
  * Fix LP: #898883 - IPC xpcshell tests hang the buildd's. Give all
    xpcshell tests an X display, as plugin-container won't work without one
    - update debian/build/testsuite.mk
  * Turn on all IPC xpcshell tests again
    - update debian/build/testsute.mk
  * Drop the default-apps xml file from lucid and maverick - there is
    already one provided by gnome-control-center, which means that ours
    is only relevant for nightly builds. It's not worth the extra
    complexity for this
    - remove debian/firefox.xml.in
    - update debian/firefox-gnome-support.install.in
    - update debian/rules
  * Ship Test Pilot as a distribution addon, like upstream. This means
    that the addon manager can update it. It does also mean that it will
    remain installed in users profiles if they try the beta or aurora
    builds, but the Feedback button is disabled on release builds
    - update debian/firefox.install.in
    - fixes LP: #913357
  * Drop patches fixed upstream
    - remove debian/patches/fix-build-failure-without-yarr-jit.patch
    - remove debian/patches/fix-cursor-handling.patch
    - update debian/patches/series
  * Keep the firefox-kde-support suggest on releases older than precise
    for now
    - update debian/rules
  * Ensure that we suggest kmozillahelper on lucid
    - update debian/rules
  * Ensure that we replace kubuntu-firefox-installer on lucid
    - update debian/rules
  * Don't build with --disable-gconf on precise and newer. There won't be
    a hard runtime requirement on this from Firefox 12 anyway, and this
    keeps us closer to the upstream configuration
    - update debian/config/mozconfig.in
...

Read more...

Changed in firefox (Ubuntu Precise):
status: Fix Committed → Fix Released
Revision history for this message
In , Vlad-mozbugs (vlad-mozbugs) wrote :

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

Revision history for this message
Launchpad Janitor (janitor) wrote :
Download full text (4.2 KiB)

This bug was fixed in the package firefox - 11.0+build1-0ubuntu0.11.10.1

---------------
firefox (11.0+build1-0ubuntu0.11.10.1) oneiric-security; urgency=low

  * New upstream stable release (FIREFOX_11_0_BUILD1)
    - Fix LP: #875266 - Firefox ignores GNOME3 proxy settings
    - Fix LP: #857153 - Needs to get accessibility settings from GSettings
    - see LP: #951250 for USN information

  * Update globalmenu-extension to 2.0.3
  * Ensure that the crash reporter is disabled if rebuilt by Ubuntu
    derivatives, as there will be no crash symbols for those
    - update debian/rules
  * Only add "Ubuntu" to the UA string when being built for Ubuntu
    - update debian/rules
  * Temporarily disable ipdl tests due to build failures. These aren't
    enabled upstream, anyway
    - update debian/config/mozconfig.in
  * Always set the update channel - not setting it at build-time on release
    builds breaks the extensions.checkCompatibility pref. The only things
    using it at runtime are nsBlocklistService, Test Pilot (beta + aurora)
    and the about dialog (where the channel is hidden anyway)
    - update debian/rules
    - update debian/firefox.install.in
  * Fix LP: #898883 - IPC xpcshell tests hang the buildd's. Give all
    xpcshell tests an X display, as plugin-container won't work without one
    - update debian/build/testsuite.mk
  * Turn on all IPC xpcshell tests again
    - update debian/build/testsute.mk
  * Ship Test Pilot as a distribution addon on aurora + beta, like upstream.
    This means that the addon manager can update it. It does also mean that it
    will remain installed in users profiles if they try the beta or aurora
    builds, but the Feedback button is disabled on release builds
    - update debian/firefox.install.in
    - fixes LP: #913357
  * Drop patches fixed upstream
    - remove debian/patches/fix-cursor-handling.patch
    - update debian/patches/series
  * Call xvfb-run with "-a" in case there are other servers running on the
    builder
    - update debian/build/testsuite.mk
  * Really fix LP: #898883 - IPC xpcshell tests hang the build. What was
    actually happening is plugin-container would fail to start because all
    available X connections had been used up by many instances of dbus-launch,
    spawned each time an xpcshell tried to talk to the session bus. Because
    we run all of the xpcshell tests with one Xvfb instance, the buses
    accumulate until the available X connections all run out. To fix this, run
    all tests requiring a display inside dbus-launch, so we create just a
    single bus for all xpcshell tests
    - update debian/build/testsuite.mk
    - update debian/control{,.in}
  * Add Ligurian to locale blacklist, as we don't support this in Ubuntu
    - update debian/config/locales.blacklist
  * Refresh patches
    - update debian/patches/ubuntu-ua-string-changes.patch
    - update debian/patches/mozilla-kde.patch
    - update debian/patches/firefox-kde.patch
  * Fix LP: #915895 - Just set autoDisableScopes to 0. Other distributions
    are already doing this, and we already made this feature pretty much
    useless by allowing extensions in the application directory, so that our
   ...

Read more...

Changed in firefox (Ubuntu Oneiric):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.