Last Comment Bug 761589 - Make accessibility.force_disabled cross platform
: Make accessibility.force_disabled cross platform
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on: 538530
Blocks: 761763 769304 781090
  Show dependency treegraph
 
Reported: 2012-06-05 07:12 PDT by David Bolter [:davidb]
Modified: 2012-08-07 22:24 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Refactor accessibility.force_disabled to work on Mac too. r= (5.89 KB, patch)
2012-06-05 17:56 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Review
Refactor accessibility.force_disabled to work on Mac too and make it tri-state. (6.20 KB, patch)
2012-06-08 16:04 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Review
Refactor accessibility.force_disabled to work on Mac too and make it tri-state. (6.08 KB, patch)
2012-06-14 16:57 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Review
Refactor accessibility.force_disabled to work on Mac too and make it tri-state. (6.58 KB, patch)
2012-06-15 17:47 PDT, Hubert Figuiere [:hub]
tbsaunde+mozbugs: review+
hub: checkin+
Details | Diff | Review
Part 2: implement accessibility.force_disabled for ATK. r= (2.98 KB, patch)
2012-06-27 14:17 PDT, Hubert Figuiere [:hub]
no flags Details | Diff | Review
Part 2: implement accessibility.force_disabled for ATK. (1.07 KB, patch)
2012-06-30 15:54 PDT, Hubert Figuiere [:hub]
tbsaunde+mozbugs: review+
Details | Diff | Review

Description David Bolter [:davidb] 2012-06-05 07:12:02 PDT
This would be for users who don't want the a11y performance tax, but somehow can't avoid our engine being instantiated.

Already implemented for Windows over on bug 538530.
Comment 1 Hubert Figuiere [:hub] 2012-06-05 13:53:48 PDT
isn't that part of bug 568916?
Comment 2 Hubert Figuiere [:hub] 2012-06-05 17:56:09 PDT
Created attachment 630390 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too. r=
Comment 3 Hubert Figuiere [:hub] 2012-06-05 17:57:15 PDT
Comment on attachment 630390 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too. r=

I believe this is the correct approach.

I need to do it for ATK as well. Refactored but not tested Windows.
Comment 4 Trevor Saunders (:tbsaunde) 2012-06-05 20:11:48 PDT
So, is this pref supposed to disable a11y no matter what tries to turn it on, or just if the platform tries to turn it on?  If the former this is not the right approach, we should instead just add a check somewhere in NS_GetAccessibilityService() or nsAccessibilityService::Init().  In the later case I don't really like that each platform has to deal with it seperately, but perhaps its more reasonable.
Comment 5 Hubert Figuiere [:hub] 2012-06-05 21:10:30 PDT
On Windows it was initially just the plateform. I made it more generic.

Yes I'm thinking we should try to move that to the cross platform code.

As for the approach of really disabling the nsAccessibilityService, this was also my approach for the VoiceOver white listing. So I didn't reconsider it this time given that you didn't really like it.
Comment 6 alexander :surkov 2012-06-05 21:44:46 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> So, is this pref supposed to disable a11y no matter what tries to turn it
> on, or just if the platform tries to turn it on?

I think it should be former since 3d parties softwares tend to use accessibility service too.
Comment 7 alexander :surkov 2012-06-05 21:45:55 PDT
Comment on attachment 630390 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too. r=

asking Trevor for review since he deals with this code at regular basis.
Comment 8 Trevor Saunders (:tbsaunde) 2012-06-07 15:39:30 PDT
(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > So, is this pref supposed to disable a11y no matter what tries to turn it
> > on, or just if the platform tries to turn it on?
> 
> I think it should be former since 3d parties softwares tend to use
> accessibility service too.

To be clear you think this pref should effect addons too right?
Comment 9 alexander :surkov 2012-06-07 19:05:23 PDT
yes
Comment 10 alexander :surkov 2012-06-07 19:49:36 PDT
it makes sense to have an option to disable a11y entirely but it might be nice if we can disable platform a11y only.
Comment 11 Hubert Figuiere [:hub] 2012-06-08 10:57:16 PDT
Let's keep that one on disabling platform.

Also in that case, I'd like to turn it to a tri-state. -1 = force enable 0 = auto (default) and 1 = disabled.
The "force enabled" being for case like Mac were we have a whitelisting.
Comment 12 Hubert Figuiere [:hub] 2012-06-08 16:04:48 PDT
Created attachment 631561 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.
Comment 13 Trevor Saunders (:tbsaunde) 2012-06-13 06:33:57 PDT
> As for the approach of really disabling the nsAccessibilityService, this was
> also my approach for the VoiceOver white listing. So I didn't reconsider it
> this time given that you didn't really like it.

Well, I mostly didn't like adding stuff that was only for one platform to cross platform code.
Comment 14 Trevor Saunders (:tbsaunde) 2012-06-13 06:36:28 PDT
(In reply to Hub Figuiere [:hub] from comment #11)
> Let's keep that one on disabling platform.

I assume you mean this one, ok :)

> Also in that case, I'd like to turn it to a tri-state. -1 = force enable 0 =
> auto (default) and 1 = disabled.
> The "force enabled" being for case like Mac were we have a whitelisting.


that sounds good.  Ideally I think we'd have a way to force enabling without someone calling NS_GetAccessibilityService(), but I guess I'm fine to leave that for another day.
Comment 15 Trevor Saunders (:tbsaunde) 2012-06-13 06:55:30 PDT
Comment on attachment 631561 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.

>+int
>+A11yDisabledState()

I'd call it PlatformDisabledState() I think unless osmebody has a better idea.

>+{
>+  static int accDisabledState = 0xff;
>+
>+  if (accDisabledState == 0xff) {

I don't see a good reason to cache this, and allowing people to change it at runtime sounds nice. Obviously force disabling won't work, but other changes will.

>+    const char* kPrefName = "accessibility.force_disabled";
>+    accDisabledState = Preferences::GetInt(kPrefName, 0);

nit, you could just pass the string directly.

>+/**
>+ * Check the global disable flag. Don't call directly in the platform code.
>+ * Possible values are -1 all enabled, 0 default, 1 all disabled
>+ */

this comment doesn't really make sense, you do call it in the platform code.  I think I'd say "check if platform is allowed to activate accessibility"

>+int A11yDisabledState();

while you touch this stuff please introduce enum for these states.

> #include "ApplicationAccessibleWrap.h"
> 
>+#include "mozilla/Preferences.h"

you don't actually need that do you?

>+ * Mac a11y whitelisting
>+ */

nit, usual style is // comments in cpp files, though I'm not sure if that comment is really useful.

note, I'd say you should fix atk too.
Comment 16 Hubert Figuiere [:hub] 2012-06-13 14:29:11 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #15)

> >+{
> >+  static int accDisabledState = 0xff;
> >+
> >+  if (accDisabledState == 0xff) {
> 
> I don't see a good reason to cache this, and allowing people to change it at
> runtime sounds nice. Obviously force disabling won't work, but other changes
> will.

It is actually called quite often. And that's the behaviour we had before for the Windows only version.

> note, I'd say you should fix atk too.

I was wondering if I should touch that ; maybe I should leave it for part 2 of this bug - that I will do. I really need the Mac side.
Comment 17 Hubert Figuiere [:hub] 2012-06-14 16:57:56 PDT
Created attachment 633321 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.
Comment 18 Hubert Figuiere [:hub] 2012-06-15 17:47:08 PDT
Created attachment 633738 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.
Comment 19 Hubert Figuiere [:hub] 2012-06-15 17:47:58 PDT
Comment on attachment 633738 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.

updated to fix Windows build (missing include)
Comment 20 Trevor Saunders (:tbsaunde) 2012-06-16 01:32:52 PDT
(In reply to Hub Figuiere [:hub] from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > >+{
> > >+  static int accDisabledState = 0xff;
> > >+
> > >+  if (accDisabledState == 0xff) {
> > 
> > I don't see a good reason to cache this, and allowing people to change it at
> > runtime sounds nice. Obviously force disabling won't work, but other changes
> > will.
> 
> It is actually called quite often. And that's the behaviour we had before
> for the Windows only version.

I'd really like to see some evidence its an issue, getting the pref should just be a hash table look up or two iirc.  However I guess I'll live with it since we did it before and there's no critical reason to change it now.

> > note, I'd say you should fix atk too.
> 
> I was wondering if I should touch that ; maybe I should leave it for part 2
> of this bug - that I will do. I really need the Mac side.

uit really shouldn't take very long at all, but ok file a follow up.
Comment 21 Trevor Saunders (:tbsaunde) 2012-06-16 01:47:41 PDT
Comment on attachment 633738 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.

>+int
>+PlatformDisabledState()

I'd say the enum should be named, and it should return that type.

>+{
>+  static int accDisabledState = 0xff;

nit, kill the acc part obviously it has to do with accessibility

>+//
>+// Mac a11y whitelisting
>+//

nit, blank comment lines aren't needed for this sort of comment

> bool
> ShouldA11yBeEnabled()
> {
>-  return sA11yShouldBeEnabled;
>+  int disabledState = PlatformDisabledState();
>+  return (disabledState < ePlatformIsEnabled) || ((disabledState == ePlatformIsEnabled) && sA11yShouldBeEnabled);

I'd say you should check disabledState ==  ePlatformForceEnabled  incase we change what other values mean in the future.  Infact I'm dubious of the whole pick the nearest real value thing, but I guess its good to be compatable with the old way the pref worked.

>@@ -7315,30 +7316,20 @@ nsWindow::GetRootAccessible()
> {
>   // We want the ability to forcibly disable a11y on windows, because
>   // some non-a11y-related components attempt to bring it up.  See bug
>   // 538530 for details; we have a pref here that allows it to be disabled
>   // for performance and testing resons.
>   //
>   // This pref is checked only once, and the browser needs a restart to
>   // pick up any changes.

I think most of this comment belongs in prefs.js since it documents the pref not what we do here now.

>   // If the pref was true, return null here, disabling a11y.
>-  if (accForceDisable)

update this comment too, since its basically bogus now.

>-      return nsnull;
>+  if (mozilla::a11y::PlatformDisabledState() == mozilla::a11y::ePlatformIsDisabled)
>+    return nsnull;

no need for mozilla:: and that's probably over 80 chars.

r=me with those.
Comment 23 Hubert Figuiere [:hub] 2012-06-19 18:40:39 PDT
Bug left open for ATK support to be done.
Comment 24 Ed Morley [:emorley] 2012-06-20 02:19:10 PDT
https://hg.mozilla.org/mozilla-central/rev/821e67964e43
Comment 25 Hubert Figuiere [:hub] 2012-06-27 14:17:01 PDT
Created attachment 637256 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK. r=
Comment 26 Hubert Figuiere [:hub] 2012-06-27 14:51:35 PDT
Comment on attachment 637256 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK. r=

Here is the ATK implementation. Trevor, shall we get somedody else to review that as well?
Comment 27 alexander :surkov 2012-06-27 21:55:31 PDT
Comment on attachment 637256 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK. r=

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

::: accessible/src/atk/ApplicationAccessibleWrap.cpp
@@ +891,5 @@
> +{
> +  EPlatformDisabledState disabledState = PlatformDisabledState();
> +  return (disabledState == ePlatformIsForceEnabled)
> +    || ((disabledState == ePlatformIsEnabled)
> +        && platformEnable);

nit: don't keep operators on new line
Comment 28 Trevor Saunders (:tbsaunde) 2012-06-28 22:47:39 PDT
Comment on attachment 637256 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK. r=

>+ShouldEnable(bool platformEnable)
>+{

it doesn't really seem like a great idea for every return path from the next function to have to call this function for things to work correctly.  It also seems like it would just be simpler to add the following before we check GNOME_ACCESSIBILITY env var in ShouldA11yBeEnabled()

eDisabledState state = PlatformEnabledState()
if (platformEnabledState == eForceDisabled) {
  dbus_cancel_pendingrequest(sPendingCall);
  dbus_pending_request_unref(sBendingCall);
  sPendingcall = nsnull;
  return sShouldEnable = false;
} else if (state == eForceEnabled) {
  return sShouldEnable = true;
}

note this will end up doing slightly  different from what this does on windows since  eForceEnabled will mean accessibility is always on period.
Comment 29 Hubert Figuiere [:hub] 2012-06-30 15:54:25 PDT
Created attachment 638157 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK.
Comment 30 Hubert Figuiere [:hub] 2012-06-30 15:56:14 PDT
Comment on attachment 638157 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK.

I don't know why I wanted to make it so complicated.

Note that, like on Windows, it doesn't implement the "force enabled" value.
Comment 31 Trevor Saunders (:tbsaunde) 2012-07-01 11:55:37 PDT
Comment on attachment 638157 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK.

>+  EPlatformDisabledState disabledState = PlatformDisabledState();
>+  if (disabledState == ePlatformIsDisabled) {
>+    sShouldEnable = false;
>+    return sShouldEnable;
>+  }

nit, just do

if (PlatformDisabledState() == ePlatformIsDisabled)
  return sShouldEnable = false;

Note You need to log in before you can comment on or make changes to this bug.