752935 - Support on the fly system accessibility changes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Eitan Isaacson [:eeejay]]

This is for supporting ICS where AccessibilityManager.AccessibilityStateChangeListener could listen for system settings changes on the fly. And the settings are more than a binary, they should also show if touch exploration is enabled or not.

So I am thinking AccessFu could send an "Accessibility:Ready" message, and then on a separate thread for checking and sending "Accessibility:Settings". Either now or later I could extend that for ICS and register a listener for Android a11y changes.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Decouple Android a11y checking.

This is part 1. The second part will include some Java introspection magic that will allow us to take advantage of AccessibilityManager.AccessibilityStateChangeListener so we could send Accessibility:Settings messages every time system accessibility is toggled.

Putting two reviewers here, kats for the Java end to assure that this is performant.
davidb on the AccessFu js end.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 622222 [details] [diff] [review]
Decouple Android a11y checking.

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

::: accessible/src/jsat/AccessFu.jsm
@@ +47,5 @@
>        accessPref = this.prefsBranch.getIntPref('accessfu');
>      } catch (x) {
>      }
>  
> +    this.parsePreferences(accessPref);

parse usually doesn't mean process

@@ +97,5 @@
>      this.chromeWin.removeEventListener('scroll', this, true);
>      this.chromeWin.removeEventListener('TabOpen', this, true);
>    },
>  
> +  parsePreferences: function(aPref) {

I'm curious why you continue to introduce unnamed functions

@@ +168,5 @@
>    },
>  
>    observe: function observe(aSubject, aTopic, aData) {
>      switch (aTopic) {
> +      case 'Accessibility:Settings':

it's better to comment each case

@@ +169,5 @@
>  
>    observe: function observe(aSubject, aTopic, aData) {
>      switch (aTopic) {
> +      case 'Accessibility:Settings':
> +        dump(aTopic + ' ' + JSON.stringify(aData, null, 2) + '\n');

you tend to put more and more dumps what means that running a debug build with accessFu enabled produce much noise (not warnings).

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 622222 [details] [diff] [review]
Decouple Android a11y checking.

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

::: accessible/src/jsat/AccessFu.jsm
@@ +47,5 @@
>        accessPref = this.prefsBranch.getIntPref('accessfu');
>      } catch (x) {
>      }
>  
> +    this.parsePreferences(accessPref);

Yeah, processPreferences would WFM too.

@@ +119,5 @@
> +
> +    if (aPref == ACCESSFU_ENABLE)
> +      this.enable();
> +    else
> +      this.disable();

Fun fact: you could fold this into
this[(aPref == ACCESSFU_ENABLE) ? "enable" : "disable"]();

but don't :)

::: mobile/android/base/GeckoApp.java
@@ +1070,5 @@
> +                        AccessibilityManager accessibilityManager =
> +                            (AccessibilityManager) mAppContext.getSystemService(Context.ACCESSIBILITY_SERVICE);
> +                        try {
> +                            ret.put("enabled", accessibilityManager.isEnabled());
> +                            ret.put("exploreByTouch", false);

Probably want a // XXX need to read exploreByTouch pref comment right?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 622222 [details] [diff] [review]
> Decouple Android a11y checking.
> 
> Review of attachment 622222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +47,5 @@
> >        accessPref = this.prefsBranch.getIntPref('accessfu');
> >      } catch (x) {
> >      }
> >  
> > +    this.parsePreferences(accessPref);
> 
> parse usually doesn't mean process
> 
> @@ +97,5 @@
> >      this.chromeWin.removeEventListener('scroll', this, true);
> >      this.chromeWin.removeEventListener('TabOpen', this, true);
> >    },
> >  
> > +  parsePreferences: function(aPref) {
> 
> I'm curious why you continue to introduce unnamed functions
> 

Because I forgot?

> @@ +168,5 @@
> >    },
> >  
> >    observe: function observe(aSubject, aTopic, aData) {
> >      switch (aTopic) {
> > +      case 'Accessibility:Settings':
> 
> it's better to comment each case
> 
> @@ +169,5 @@
> >  
> >    observe: function observe(aSubject, aTopic, aData) {
> >      switch (aTopic) {
> > +      case 'Accessibility:Settings':
> > +        dump(aTopic + ' ' + JSON.stringify(aData, null, 2) + '\n');
> 
> you tend to put more and more dumps what means that running a debug build
> with accessFu enabled produce much noise (not warnings).

This dump is an artefact that should not be in the final patch.

Comment 5

[Eitan Isaacson [:eeejay]]

Not uploading a new patch until kats gives it a look.

(In reply to David Bolter [:davidb] from comment #3)
> Comment on attachment 622222 [details] [diff] [review]
> Decouple Android a11y checking.
> 
> Review of attachment 622222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +47,5 @@
> >        accessPref = this.prefsBranch.getIntPref('accessfu');
> >      } catch (x) {
> >      }
> >  
> > +    this.parsePreferences(accessPref);
> 
> Yeah, processPreferences would WFM too.
> 

Cool.

> @@ +119,5 @@
> > +
> > +    if (aPref == ACCESSFU_ENABLE)
> > +      this.enable();
> > +    else
> > +      this.disable();
> 
> Fun fact: you could fold this into
> this[(aPref == ACCESSFU_ENABLE) ? "enable" : "disable"]();
> 
> but don't :)
> 

You will be sorry you put that in my head :)

> ::: mobile/android/base/GeckoApp.java
> @@ +1070,5 @@
> > +                        AccessibilityManager accessibilityManager =
> > +                            (AccessibilityManager) mAppContext.getSystemService(Context.ACCESSIBILITY_SERVICE);
> > +                        try {
> > +                            ret.put("enabled", accessibilityManager.isEnabled());
> > +                            ret.put("exploreByTouch", false);
> 
> Probably want a // XXX need to read exploreByTouch pref comment right?

Sure, I'll add that.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 622222 [details] [diff] [review]
Decouple Android a11y checking.

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

Looks fine to me

::: mobile/android/base/GeckoApp.java
@@ +1071,5 @@
> +                            (AccessibilityManager) mAppContext.getSystemService(Context.ACCESSIBILITY_SERVICE);
> +                        try {
> +                            ret.put("enabled", accessibilityManager.isEnabled());
> +                            ret.put("exploreByTouch", false);
> +                        } catch (Exception ex) { }

Either log the exception using Log.e or add a comment as to why it can be ignored

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 622222 [details] [diff] [review]
Decouple Android a11y checking.

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

r=me, OK sounds like all the requested changes are not controversial.

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Decouple Android a11y checking, prepare for ICS on-the-fly a11y toggle events. r=davidb, r=kats

Updated patch with nits. I pushed it to try just to see how well it does on that talos test.
https://tbpl.mozilla.org/?tree=Try&rev=c0ce5cd2a1a9

Comment 9

[Eitan Isaacson [:eeejay]]

If I am reading this right, this patch might make things better :P
http://graphs.mozilla.org/graph.html#tests=[[84,23,20]]&sel=1336596435627.5403,1336618601308&displayrange=7&datatype=running

Comment 10

[Matt Brubeck (:mbrubeck)]

The average Tp4 NoChrome score before bug 749719 was 595.9 (stddev 12.0).
The average Tp4 NoChrome score  after bug 749719 was 658.2 (stddev 9.9).

The score for c0ce5cd2a1a9 on Try was 650.95, so while it may or may not have improved things a bit, it hasn't fully fixed the regression. :(

Comment 11

[Eitan Isaacson [:eeejay]]

The delta after the commit from bug 749719 was +44.
The delta from this patch is -38.
So the overall impact is +6. Am I reading that wrong? Is that still a regression?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #4)

> > I'm curious why you continue to introduce unnamed functions
> > 
> 
> Because I forgot?

I don't know ;) But if yes then ok, I needed to make sure you don't do that on demand.

> This dump is an artefact that should not be in the final patch.

ok

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Eitan Isaacson [:eeejay] from comment #11)
> The delta after the commit from bug 749719 was +44.
> The delta from this patch is -38.
> So the overall impact is +6. Am I reading that wrong? Is that still a
> regression?

You are reading it wrong. You can't look at the deltas. You need to look at the running average. As Matt said, the average Tp4 NoChrome before bug 749719 was still lower than 650.

Comment 14

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ce53253d5717

Comment 15

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/ce53253d5717