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.

isn't that part of bug 568916?

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.

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.

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.

(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 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.

(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?

yes

it makes sense to have an option to disable a11y entirely but it might be nice if we can disable platform a11y only.

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.

> 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.

(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 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.

(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 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)

(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 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.

Bug left open for ATK support to be done.

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 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 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 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 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;