Closed Bug 1081720 Opened 10 years ago Closed 10 years ago

[AccessFu] Use prefs service to store current quicknav mode

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file)

Storing it as a pref, and proxying it to content settings API will give us a consistent state across gaia and chrome to work with.
Comment on attachment 8504426 [details] [diff] [review]
Use prefs service to store quicknav state and proxy quicknav prefs to b2g settings.

Adding Fabrice as reviewer for non-accessfu bits (namely settings.js)
Attachment #8504426 - Flags: review?(fabrice)
Attachment #8504426 - Flags: review?(fabrice) → review+
Comment on attachment 8504426 [details] [diff] [review]
Use prefs service to store quicknav state and proxy quicknav prefs to b2g settings.

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

a couple of nits, otherwise looks good.

::: accessible/jsat/AccessFu.jsm
@@ +20,5 @@
>  const ACCESSFU_AUTO = 2;
>  
>  const SCREENREADER_SETTING = 'accessibility.screenreader';
> +const QUICKNAV_MODES_PREF = 'accessibility.accessfu.quicknav_modes';
> +const QUICKNAV_CURRENT_MODE_PREF = 'accessibility.accessfu.quicknav_current_mode';

Nit: Here and below ..._mode_index or just ..._index

@@ +976,5 @@
>  
>      next: function quickNavMode_next() {
> +      Services.prefs.setIntPref(QUICKNAV_CURRENT_MODE_PREF,
> +        this._currentIndex + 1 >= this.modes.length ?
> +          0: this._currentIndex + 1);

Nit: whitespace

::: accessible/tests/mochitest/jsat/jsatcommon.js
@@ +44,5 @@
>          // Ignore unexpected messages.
>          if (!(aMessage instanceof Components.interfaces.nsIConsoleMessage)) {
>            return;
>          }
> +        //info('comparing ' + aMessage.message + ' : ' + aWaitForMessage);

Nit: whitespace

@@ +157,5 @@
>        Logger.logLevel = Logger.DEBUG;
>      };
>  
> +    var prefs = [['accessibility.accessfu.notify_output', 1],
> +      ['dom.mozSettings.enabled', true]]

Nit: ;

@@ +158,5 @@
>      };
>  
> +    var prefs = [['accessibility.accessfu.notify_output', 1],
> +      ['dom.mozSettings.enabled', true]]
> +    Array.prototype.push.apply(prefs, aAdditionalPrefs);

prefs.push.apply

::: accessible/tests/mochitest/jsat/test_quicknav_modes.html
@@ +23,5 @@
> +      return function() {
> +        is(AccessFu.Input.quickNavMode.current, aCurrentMode,
> +          'initial current mode is correct');
> +        AccessFu.Input.quickNavMode.next();
> +        _expectMode(aNextMode, AccessFuTest.nextTest);

I think we can move this before AccessFu.Input.quickNavMode.next();
This should let you get rid of lines 57 to 60

@@ +32,5 @@
> +      return function() {
> +        is(AccessFu.Input.quickNavMode.current, aCurrentMode,
> +          'initial current mode is correct');
> +        AccessFu.Input.quickNavMode.previous();
> +        _expectMode(aNextMode, AccessFuTest.nextTest);

Same as above

@@ +40,5 @@
> +    function setMode(aModeIndex, aExpectedMode) {
> +      return function() {
> +        SpecialPowers.setIntPref(
> +          'accessibility.accessfu.quicknav_current_mode', aModeIndex);
> +        _expectMode(aExpectedMode, AccessFuTest.nextTest);

Same as above

@@ +49,5 @@
> +      SpecialPowers.setCharPref('accessibility.accessfu.quicknav_modes',
> +        'Landmark,Button,Entry,Graphic');
> +      // When the modes are reconfigured, the current mode should
> +      // be set to the first in the new list.
> +      _expectMode('Landmark', AccessFuTest.nextTest);

Same as above.

@@ +52,5 @@
> +      // be set to the first in the new list.
> +      _expectMode('Landmark', AccessFuTest.nextTest);
> +    }
> +
> +    function _expectMode(aExpectedMode, aCallback) {

Only lines 61-63 should be necessary.

@@ +68,5 @@
> +    // Listen for initial 'EventManager.start' and disable AccessFu.
> +    function prefStop() {
> +      ok(AccessFu._enabled, "AccessFu was started via preference.");
> +      AccessFuTest.once_log("EventManager.stop", AccessFuTest.finish);
> +      SpecialPowers.setIntPref("accessibility.accessfu.activate", 0);

Just like here ^

@@ +85,5 @@
> +      AccessFuTest.addFunc(prefStop);
> +      AccessFuTest.waitForExplicitFinish();
> +      AccessFuTest.runTests([
> +        ['accessibility.accessfu.quicknav_modes', 'Link,Heading,FormElement']
> +        ]);  // Will call SimpleTest.finish();

Nit: whitespace
Attachment #8504426 - Flags: review?(yzenevich) → review+
https://hg.mozilla.org/mozilla-central/rev/2fdd96ecd3e1
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.