Last Comment Bug 705983 - Firefox a11y is not enabled on GNOME if config.use_system_prefs.accessibility is false (default)
: Firefox a11y is not enabled on GNOME if config.use_system_prefs.accessibility...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Linux
: -- major (vote)
: mozilla11
Assigned To: Trevor Saunders (:tbsaunde)
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 451161 693343
  Show dependency treegraph
 
Reported: 2011-11-28 21:18 PST by Ginn Chen
Modified: 2012-01-11 09:35 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug 705983 always check gconf for accessibility state when GNOME_ACCESSIBILITY not set (4.99 KB, patch)
2011-12-01 15:34 PST, Trevor Saunders (:tbsaunde)
ginn.chen: review+
roc: review+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review
bug 705983 (part 2) - remove the config.use_system_prefs.accessibility pref (1.01 KB, patch)
2011-12-01 18:40 PST, Trevor Saunders (:tbsaunde)
ginn.chen: review+
Details | Diff | Splinter Review

Description Ginn Chen 2011-11-28 21:18:24 PST
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] 2011-11-28 22:23:35 PST
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] 2011-11-28 22:26:12 PST
And I doubt the usefulness of the pref because nobody cared about the brokenness for a long time.
Comment 3 Ginn Chen 2011-11-28 23:48:35 PST
(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] 2011-11-29 04:34:13 PST
(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) 2011-12-01 15:34:30 PST
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) 2011-12-01 18:40:03 PST
Created attachment 578474 [details] [diff] [review]
bug 705983 (part 2) - remove the config.use_system_prefs.accessibility pref
Comment 7 alexander :surkov 2011-12-01 20:55:22 PST
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 2011-12-01 20:56:19 PST
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] 2011-12-02 04:31:21 PST
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] 2011-12-02 04:33:15 PST
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) 2011-12-05 21:30:09 PST
(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 2011-12-07 00:20:07 PST
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 2011-12-07 00:26:53 PST
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) 2011-12-09 00:28:12 PST
inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/549b2d9592ce
Comment 15 Ed Morley [:emorley] 2011-12-10 20:42:38 PST
https://hg.mozilla.org/mozilla-central/rev/549b2d9592ce
Comment 16 Masatoshi Kimura [:emk] 2012-01-10 17:06:45 PST
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 2012-01-11 09:35:19 PST
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.

Note You need to log in before you can comment on or make changes to this bug.