All users were logged out of Bugzilla on October 13th, 2018

Support on the fly system accessibility changes

RESOLVED FIXED in Firefox 15

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
Firefox 15
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

7 years ago
Blocks: 752905
(Assignee)

Updated

7 years ago
Assignee: nobody → eitan
OS: Linux → Android
Hardware: x86_64 → ARM
Target Milestone: --- → Firefox 15
(Assignee)

Comment 1

7 years ago
Created attachment 622222 [details] [diff] [review]
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.
Attachment #622222 - Flags: review?(dbolter)
Attachment #622222 - Flags: review?(bugmail.mozilla)
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 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?
(Assignee)

Comment 4

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

Comment 5

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

Comment 8

7 years ago
Created attachment 622461 [details] [diff] [review]
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
Attachment #622222 - Attachment is obsolete: true
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. :(
(Assignee)

Comment 11

7 years ago
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?
(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
(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.
https://hg.mozilla.org/mozilla-central/rev/ce53253d5717
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.