[AccessFu] Add preference for explore by touch

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla16
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
This adds the preference, and a minor refactor. I tried to make it as readable and simple as possible.
(Assignee)

Comment 1

5 years ago
Created attachment 632374 [details] [diff] [review]
Explore by touch preference
Attachment #632374 - Flags: review?(dbolter)
Comment on attachment 632374 [details] [diff] [review]
Explore by touch preference

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

::: accessible/src/jsat/AccessFu.jsm
@@ +101,5 @@
> +  _processPreferences: function _processPreferences(aEnabled, aTouchEnabled) {
> +    let accessPref = ACCESSFU_DISABLE;
> +    try {
> +      accessPref = (aEnabled == undefined) ?
> +        this.prefsBranch.getIntPref('activate') : aEnabled;

You could write:
accessPref = aEnabled || this.prefsBranch.getIntPref('activate');

(same for the ebt case)

@@ -113,5 @@
>          return;
>        }
> -
> -      if (this._observingSystemSettings) {
> -        Services.obs.removeObserver(this, 'Accessibility:Settings');

Where do you remove this observer?
(Assignee)

Comment 3

5 years ago
(In reply to David Bolter [:davidb] from comment #2)
> Comment on attachment 632374 [details] [diff] [review]
> Explore by touch preference
> 
> Review of attachment 632374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +101,5 @@
> > +  _processPreferences: function _processPreferences(aEnabled, aTouchEnabled) {
> > +    let accessPref = ACCESSFU_DISABLE;
> > +    try {
> > +      accessPref = (aEnabled == undefined) ?
> > +        this.prefsBranch.getIntPref('activate') : aEnabled;
> 
> You could write:
> accessPref = aEnabled || this.prefsBranch.getIntPref('activate');
> 
> (same for the ebt case)
> 

I don't think I can because I want it to check out as true when aEnabled is 0. So I need to explicitly check if the argument was passed.

> @@ -113,5 @@
> >          return;
> >        }
> > -
> > -      if (this._observingSystemSettings) {
> > -        Services.obs.removeObserver(this, 'Accessibility:Settings');
> 
> Where do you remove this observer?

I never do. Gave up on that. I don't think it is that bad. attach() could only be run once, so there is no concern of adding the observer more than once.
(In reply to Eitan Isaacson [:eeejay] from comment #3)
> (In reply to David Bolter [:davidb] from comment #2)
> > Comment on attachment 632374 [details] [diff] [review]
> > Explore by touch preference
> > 
> > Review of attachment 632374 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/jsat/AccessFu.jsm
> > @@ +101,5 @@
> > > +  _processPreferences: function _processPreferences(aEnabled, aTouchEnabled) {
> > > +    let accessPref = ACCESSFU_DISABLE;
> > > +    try {
> > > +      accessPref = (aEnabled == undefined) ?
> > > +        this.prefsBranch.getIntPref('activate') : aEnabled;
> > 
> > You could write:
> > accessPref = aEnabled || this.prefsBranch.getIntPref('activate');
> > 
> > (same for the ebt case)
> > 
> 
> I don't think I can because I want it to check out as true when aEnabled is
> 0. So I need to explicitly check if the argument was passed.

Oh! I see.

> > @@ -113,5 @@
> > >          return;
> > >        }
> > > -
> > > -      if (this._observingSystemSettings) {
> > > -        Services.obs.removeObserver(this, 'Accessibility:Settings');
> > 
> > Where do you remove this observer?
> 
> I never do. Gave up on that. I don't think it is that bad. attach() could
> only be run once, so there is no concern of adding the observer more than
> once.

I guess that's ok?
(Assignee)

Comment 5

5 years ago
(In reply to David Bolter [:davidb] from comment #4)
> (In reply to Eitan Isaacson [:eeejay] from comment #3)
> > (In reply to David Bolter [:davidb] from comment #2)
> > > Comment on attachment 632374 [details] [diff] [review]
> > > Explore by touch preference
> > > 
> > > Review of attachment 632374 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: accessible/src/jsat/AccessFu.jsm
> > > @@ -113,5 @@
> > > >          return;
> > > >        }
> > > > -
> > > > -      if (this._observingSystemSettings) {
> > > > -        Services.obs.removeObserver(this, 'Accessibility:Settings');
> > > 
> > > Where do you remove this observer?
> > 
> > I never do. Gave up on that. I don't think it is that bad. attach() could
> > only be run once, so there is no concern of adding the observer more than
> > once.
> 
> I guess that's ok?

The patch in bug 764057 allows Accessibility:Settings to be sent in an unsolicited manner and we need to catch it.

Also, I think I was trying to be fancy earlier and it was silly. This is hardly readable already, don't need to do that dance. No real gain.
Comment on attachment 632374 [details] [diff] [review]
Explore by touch preference

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

r=me since I think this only improves the current situation. I do worry about not cleaning up observers and I think we probably need a cleanup function to be called during shutdown but we'll need to check with front-end folks on the blessed way to do that (and if it is indeed necessary).
Attachment #632374 - Flags: review?(dbolter) → review+
(Assignee)

Comment 7

5 years ago
(In reply to David Bolter [:davidb] from comment #6)
> Comment on attachment 632374 [details] [diff] [review]
> Explore by touch preference
> 
> Review of attachment 632374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me since I think this only improves the current situation. I do worry
> about not cleaning up observers and I think we probably need a cleanup
> function to be called during shutdown but we'll need to check with front-end
> folks on the blessed way to do that (and if it is indeed necessary).

We don't yet have an AccessFu.detach, but if we did, that is where we would remove the observer. Connecting an observer for one event and then detaching it seems a bit like an antipattern to me.
(Assignee)

Comment 8

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/ad0775733110
Assignee: nobody → eitan
Target Milestone: --- → mozilla16
(In reply to Eitan Isaacson [:eeejay] from comment #7)
> (In reply to David Bolter [:davidb] from comment #6)
> > Comment on attachment 632374 [details] [diff] [review]
> > Explore by touch preference
> > 
> > Review of attachment 632374 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me since I think this only improves the current situation. I do worry
> > about not cleaning up observers and I think we probably need a cleanup
> > function to be called during shutdown but we'll need to check with front-end
> > folks on the blessed way to do that (and if it is indeed necessary).
> 
> We don't yet have an AccessFu.detach, but if we did, that is where we would
> remove the observer.

Yes. Exactly. Extensions for example have something like this where they are expected to do cleanup (including removing observers). I'm not 100% sure how cleanup should happen in the accessfu case but if we have a detach then of course there. JS objects don't have destructor calls so we can't do the obvious.
https://hg.mozilla.org/mozilla-central/rev/ad0775733110
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.