705983 - Firefox a11y is not enabled on GNOME if config.use_system_prefs.accessibility is false (default)

Status: RESOLVED FIXED

Description

[Ginn Chen]

After landing Bug 451161, Firefox is no longer a11y enabled by default on GNOME even if GNOME a11y is enabled.

We should have the behavior as we did before.

Also this pref only used in ATK code, I think we should add #ifdef.

Comment 1

[Masatoshi Kimura [:emk]]

Strange. I was not meant to change the behavior by the patch.
Had Firefox enabled a11y even if both config.use_system_prefs and config.use_system_prefs.accessibility is false (default)?

Comment 2

[Masatoshi Kimura [:emk]]

And I doubt the usefulness of the pref because nobody cared about the brokenness for a long time.

Comment 3

[Ginn Chen]

(In reply to Masatoshi Kimura [:emk] from comment #1)
> Strange. I was not meant to change the behavior by the patch.
> Had Firefox enabled a11y even if both config.use_system_prefs and
> config.use_system_prefs.accessibility is false (default)?

(Have GNOME a11y enable,)
without bug 451161, yes
with bug 451161, no

The pref is just a mapping to GNOME conf /desktop/gnome/interface/accessibility
Since you have already killed extensions/pref/system-pref.
I think we can just remove it.

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Ginn Chen from comment #3)
> (In reply to Masatoshi Kimura [:emk] from comment #1)
> > Strange. I was not meant to change the behavior by the patch.
> > Had Firefox enabled a11y even if both config.use_system_prefs and
> > config.use_system_prefs.accessibility is false (default)?
> (Have GNOME a11y enable,)
> without bug 451161, yes
> with bug 451161, no
So, the prefs had not worked as expected. If config.use_system_prefs is false (default), config.use_system_prefs.accessibility is not supposed map /desktop/gnome/interface/accessibility. That is, a11y is supposed to be always disabled even before the patch.
> The pref is just a mapping to GNOME conf
> /desktop/gnome/interface/accessibility
> Since you have already killed extensions/pref/system-pref.
I updated a patch so that config.use_system_prefs* didn't rely on the system-pref. However, config.use_system_prefs.accessibility hadn't been respected because of system-pref's bug.
> I think we can just remove it.
I agree the conclusion because nobody hadn't noticed that the pref had not worked.

Comment 5

[Trevor Saunders (:tbsaunde)]

Created attachment 578428 [details] [diff] [review]
bug 705983 always check gconf for accessibility state when GNOME_ACCESSIBILITY not set

having firefox specific prefs may be a nice to have at some point but for know always using system prefs seems reasonable.

Comment 6

[Trevor Saunders (:tbsaunde)]

Created attachment 578474 [details] [diff] [review]
bug 705983 (part 2) - remove the config.use_system_prefs.accessibility pref

Comment 7

[alexander :surkov]

Comment on attachment 578428 [details] [diff] [review]
bug 705983 always check gconf for accessibility state when GNOME_ACCESSIBILITY not set

I prefer if Ginn takes a review

Comment 8

[alexander :surkov]

Comment on attachment 578474 [details] [diff] [review]
bug 705983 (part 2) - remove the config.use_system_prefs.accessibility pref

Ginn, I hope that's ok :)

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 578428 [details] [diff] [review]
bug 705983 always check gconf for accessibility state when GNOME_ACCESSIBILITY not set

Don't forget removing unused values from modules/libpref/src/init/all.js.

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 578474 [details] [diff] [review]
bug 705983 (part 2) - remove the config.use_system_prefs.accessibility pref

"config.use_system_prefs" is no longer used at all.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Masatoshi Kimura [:emk] from comment #10)
> Comment on attachment 578474 [details] [diff] [review] [diff] [details] [review]
> bug 705983 (part 2) - remove the config.use_system_prefs.accessibility pref
> 
> "config.use_system_prefs" is no longer used at all.

ok, that wasn't completely clear to me, thanks.

Ginn, do you want a second patch 2, or should I just remove config.use_system_prefs pref before landing?



(In reply to alexander :surkov from comment #8)
> Comment on attachment 578474 [details] [diff] [review] [diff] [details] [review]
> bug 705983 (part 2) - remove the config.use_system_prefs.accessibility pref
> 
> Ginn, I hope that's ok :)

I think comment 3 was suggesting this approach fwiw

Comment 12

[Ginn Chen]

Comment on attachment 578474 [details] [diff] [review]
bug 705983 (part 2) - remove the config.use_system_prefs.accessibility pref

Please remove config.use_system_prefs, too.

You can do it when you commit the code.

Comment 13

[Ginn Chen]

Comment on attachment 578428 [details] [diff] [review]
bug 705983 always check gconf for accessibility state when GNOME_ACCESSIBILITY not set

thanks for the patch!

Comment 14

[Trevor Saunders (:tbsaunde)]

inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/549b2d9592ce

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/549b2d9592ce

Comment 16

[Masatoshi Kimura [:emk]]

Comment on attachment 578428 [details] [diff] [review]
bug 705983 always check gconf for accessibility state when GNOME_ACCESSIBILITY not set

[Approval Request Comment]
Regression caused by (bug #451161): 
User impact if declined: 
Accessibility will be always disabled on GNOME by default.
Testing completed (on m-c and m-a): 
Risk to taking this patch (and alternatives if risky):
I believe the risk is very small because the pref was broken from the start.

This patch also needs an approval if the bug 451161 is approved.

Comment 17

[christian]

Comment on attachment 578428 [details] [diff] [review]
bug 705983 always check gconf for accessibility state when GNOME_ACCESSIBILITY not set

[Triage Comment]
Approving this for beta as we approved bug 451161  and this is a regression from it.