Closed Bug 705983 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Disability Access APIs, defect, major)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: ginnchen+exoracle, Assigned: tbsaunde)

References

Details

Attachments

(2 files)

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.
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)?
And I doubt the usefulness of the pref because nobody cared about the brokenness for a long time.
(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.
Summary: config.use_system_prefs.accessibility should be true by default → Firefox a11y is not enabled on GNOME if config.use_system_prefs.accessibility is false (default)
(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.
Blocks: 693343
having firefox specific prefs may be a nice to have at some point but for know always using system prefs seems reasonable.
Attachment #578428 - Flags: review?(surkov.alexander)
Attachment #578428 - Flags: review?(roc)
Severity: normal → major
Keywords: 4xp
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
Attachment #578428 - Flags: review?(surkov.alexander)
Attachment #578428 - Flags: review?(ginn.chen)
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 :)
Attachment #578474 - Flags: review?(surkov.alexander) → review?(ginn.chen)
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 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.
(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 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.
Attachment #578474 - Flags: review?(ginn.chen) → review+
Comment on attachment 578428 [details] [diff] [review]
bug 705983 always check gconf for accessibility state when GNOME_ACCESSIBILITY not set

thanks for the patch!
Attachment #578428 - Flags: review?(ginn.chen) → review+
https://hg.mozilla.org/mozilla-central/rev/549b2d9592ce
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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.
Attachment #578428 - Flags: approval-mozilla-beta?
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.
Attachment #578428 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.