750528 - [AccessFu] Add preference for enabling/disabling AccessFu

Status: RESOLVED FIXED

Description

[Eitan Isaacson [:eeejay]]

I am thinking accessibility.accessfu (int)
0: Disabled
1: Enabled
2: Auto

Auto is the default and it will sniff platform a11y and enabled or disable AccessFu.

On platforms that either don't have platform sniffing enabled in AccessFu (Mac, Windows, Linux), or platforms that don't have "platform a11y" outside of Firefox (B2G), auto is synonymous with disabled.

Comment 1

[Eitan Isaacson [:eeejay]]

Created attachment 620900 [details] [diff] [review]
Use accessibility.accessfu to enable or disable accessfu.

This was a bit too easy. But it seems to work.

Comment 2

[David Bolter [:davidb]]

Comment on attachment 620900 [details] [diff] [review]
Use accessibility.accessfu to enable or disable accessfu.

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

r=me

Please consider my thoughts below:

BTW Do you run "gjslint --nojsdoc my_file.js" ? (Just curious)

::: accessible/src/jsat/AccessFu.jsm
@@ +17,5 @@
>  Cu.import('resource://gre/modules/accessibility/VirtualCursorController.jsm');
>  
> +const ACCESSFU_DISABLED = 0;
> +const ACCESSFU_ENABLED = 1;
> +const ACCESSFU_AUTO = 2;

Note, to keep my brain happier I'd consider these prefs rather than modes throughout (as in ACCESSFU_DISABLE, ACCESSFU_ENABLE, ACCESSFU_AUTO throughout).

@@ +41,5 @@
> +    this.prefsBranch = Cc["@mozilla.org/preferences-service;1"]
> +      .getService(Ci.nsIPrefService).getBranch("accessibility.");
> +    this.prefsBranch.addObserver('accessfu', this, false);
> +
> +    let accessfuMode = ACCESSFU_DISABLED;

let accessPref = ACCESSFU_DISABLE; ?

@@ +49,3 @@
>      }
>  
> +    if (this.checkAccessibility(accessfuMode))

Optional: since it returns a boolean I'd prefer an "is" name like "isWanted" or "amINeeded" (note: in 3 places).

@@ +91,5 @@
>      this.chromeWin.removeEventListener('scroll', this);
>      this.chromeWin.removeEventListener('TabOpen', this);
>    },
>  
> +  checkAccessibility: function(aMode) {

s/aMode/aPref ?

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #2)
> Comment on attachment 620900 [details] [diff] [review]
> @@ +49,3 @@
> >      }
> >  
> > +    if (this.checkAccessibility(accessfuMode))
> 
> Optional: since it returns a boolean I'd prefer an "is" name like "isWanted"
> or "amINeeded" (note: in 3 places).
> 

thanks! I changed it to isEnabled. All the rest of the names are adjusted too.

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #2)
> BTW Do you run "gjslint --nojsdoc my_file.js" ? (Just curious)
> 

yes, although i think i missed it in the text event patch from the other day.
I run:
gjslint --additional_extensions=jsm --nojsdoc *.jsm

Comment 5

[Eitan Isaacson [:eeejay]]

Created attachment 620930 [details] [diff] [review]
Use accessibility.accessfu to enable or disable accessfu.

Comment 6

[Eitan Isaacson [:eeejay]]

Created attachment 620931 [details] [diff] [review]
Bug 750528 - Use accessibility.accessfu to enable or disable accessfu. r=davidb

Comment 7

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7972b92116b0

Comment 8

[alexander :surkov]

Comment on attachment 620931 [details] [diff] [review]
Bug 750528 - Use accessibility.accessfu to enable or disable accessfu. r=davidb

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

::: accessible/src/jsat/AccessFu.jsm
@@ +49,3 @@
>      }
>  
> +    if (this.amINeeded(accessPref))

at some point David should try to write lyrics instead code ;) alternatively named functions aren't always great for code reading.

@@ +52,1 @@
>        this.enable();

I'd keep enable a part of amINeeded since it seems nobody need to turn it on/off explicitly.

Comment 9

[David Bolter [:davidb]]

Eitan if you happen to follow Alexander and move the enable call inside the boolean function that checks preferences please rename it so that the side effect (yuck) is obvious.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7972b92116b0

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to Ed Morley [:edmorley] from comment #10)
> https://hg.mozilla.org/mozilla-central/rev/7972b92116b0

Wrong bug!

Comment 12

[Eitan Isaacson [:eeejay]]

Oops. my bad