Make accessibility.force_disabled cross platform

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: davidb, Assigned: hub)

Tracking

unspecified
mozilla16
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Assignee: nobody → hub
(Assignee)

Updated

5 years ago
Blocks: 761763
(Assignee)

Comment 1

5 years ago
isn't that part of bug 568916?
(Assignee)

Comment 2

5 years ago
Created attachment 630390 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too. r=
(Assignee)

Comment 3

5 years ago
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.
Attachment #630390 - Flags: feedback?(dbolter)
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.
(Assignee)

Comment 5

5 years ago
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

5 years ago
(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

5 years ago
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.
Attachment #630390 - Flags: review?(trev.saunders)
(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

5 years ago
yes

Comment 10

5 years ago
it makes sense to have an option to disable a11y entirely but it might be nice if we can disable platform a11y only.
(Assignee)

Comment 11

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

Comment 12

5 years ago
Created attachment 631561 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.
(Assignee)

Updated

5 years ago
Attachment #630390 - Attachment is obsolete: true
Attachment #630390 - Flags: review?(trev.saunders)
Attachment #630390 - Flags: feedback?(dbolter)
(Assignee)

Updated

5 years ago
Attachment #631561 - Flags: review?(trev.saunders)
> 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.
Attachment #631561 - Flags: review?(trev.saunders)
(Assignee)

Comment 16

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

Comment 17

5 years ago
Created attachment 633321 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.
(Assignee)

Updated

5 years ago
Attachment #631561 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #633321 - Flags: review?(trev.saunders)
(Assignee)

Comment 18

5 years ago
Created attachment 633738 [details] [diff] [review]
Refactor accessibility.force_disabled to work on Mac too and make it tri-state.
(Assignee)

Updated

5 years ago
Attachment #633321 - Attachment is obsolete: true
Attachment #633321 - Flags: review?(trev.saunders)
(Assignee)

Comment 19

5 years ago
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)
Attachment #633738 - Flags: review?(trev.saunders)
(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.
Attachment #633738 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/821e67964e43
Whiteboard: [leave open]
(Assignee)

Updated

5 years ago
Attachment #633738 - Flags: checkin+
(Assignee)

Comment 23

5 years ago
Bug left open for ATK support to be done.
https://hg.mozilla.org/mozilla-central/rev/821e67964e43
(Assignee)

Comment 25

5 years ago
Created attachment 637256 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK. r=
(Assignee)

Updated

5 years ago
Attachment #633738 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #633738 - Attachment is obsolete: false
(Assignee)

Comment 26

5 years ago
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?
Attachment #637256 - Flags: review?(trev.saunders)

Comment 27

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

Updated

5 years ago
Blocks: 769304
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.
Attachment #637256 - Flags: review?(trev.saunders)
(Assignee)

Comment 29

5 years ago
Created attachment 638157 [details] [diff] [review]
Part 2: implement accessibility.force_disabled for ATK.
(Assignee)

Updated

5 years ago
Attachment #633738 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #637256 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #633738 - Attachment is obsolete: false
(Assignee)

Comment 30

5 years ago
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.
Attachment #638157 - Flags: review?(trev.saunders)
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;
Attachment #638157 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/77aefa0444d5
Whiteboard: [leave open]
Target Milestone: --- → mozilla16

Comment 33

5 years ago
http://hg.mozilla.org/mozilla-central/rev/77aefa0444d5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 781090
You need to log in before you can comment on or make changes to this bug.