Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[AccessFu] Add preference for enabling/disabling AccessFu

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla15
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → eitan
OS: Linux → Android
Hardware: x86_64 → ARM
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Blocks: 749719
(Assignee)

Comment 1

5 years ago
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.
Attachment #620900 - Flags: review?(dbolter)
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 ?
Attachment #620900 - Flags: review?(dbolter) → review+
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
(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
(Assignee)

Comment 5

5 years ago
Created attachment 620930 [details] [diff] [review]
Use accessibility.accessfu to enable or disable accessfu.
Attachment #620900 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 620931 [details] [diff] [review]
Bug 750528 - Use accessibility.accessfu to enable or disable accessfu. r=davidb
Attachment #620930 - Attachment is obsolete: true
(Assignee)

Comment 7

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

Comment 8

5 years ago
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.
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.
https://hg.mozilla.org/mozilla-central/rev/7972b92116b0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

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

Wrong bug!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

5 years ago
Oops. my bad
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.