759736 - Add VO-only mode for FF a11y on mac.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Due to the perf issues that are cropping up, when we move up-channel with our accessibility on Mac I think we should make it VoiceOver only (rather than turn it off completely).

So this bug would be about checking the voiceOverOnOffKey flag from com.apple.universalaccess (essentially having a whitelist with only VO for now).

Thoughts?

Comment 1

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #0)

> So this bug would be about checking the voiceOverOnOffKey flag from
> com.apple.universalaccess (essentially having a whitelist with only VO for
> now).
> 
> Thoughts?

if that works then definitely. At least until we find a case where we miss something important.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

The key is probably easiest but also note there is an undocumented attribute "AXEnhancedUserInterface" we can check:
http://lists.apple.com/archives/accessibility-dev/2006/Feb/msg00009.html

Comment 3

[Hubert Figuiere [:hub]]

I think we need a combination of both. The first to check for voiceover, the second to use as an event to check again, as to allow instanciating when VoiceOver is enabled while Firefox is running. Something like that.

Also bonus point if we can do that with a preference in Gecko to bypass that check and always instanciate.

Comment 4

[Hubert Figuiere [:hub]]

Attachment: Only instanciate a11y on Mac if VO is running. r=

Comment 5

[Hubert Figuiere [:hub]]

Comment on attachment 628866 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

looking for feedback on the approach. I'll review the potential nits.

Still missing the part where we add a Gecko preference.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 628866 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

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

This seems roughly right but I don't see an ApplicationAccessibleWrap.mm file here :)


(One of the follow ups I think I'd like to see to this bug is a telemetry bug on mac a11y consumers. For now we could just say unknown when it isn't voiceover.)

::: accessible/src/base/nsAccessibilityService.h
@@ +48,5 @@
> +
> +bool IsVoiceOverChecked();
> +  
> +/** we need to check VoiceOver */
> +void NeedCheckVoiceOver(bool aNeed);

What is this for? ( And where is it? :) )

::: accessible/src/mac/Makefile.in
@@ +17,5 @@
>    
>    
>  CMMSRCS = \
>            AccessibleWrap.mm \
> +          ApplicationAccessibleWrap.mm \

Promise? :)

Comment 7

[Hubert Figuiere [:hub]]

Attachment: Only instanciate a11y on Mac if VO is running. r=

Comment 8

[Hubert Figuiere [:hub]]

Comment on attachment 628890 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

Updated patch: was missing a file.

Comment 9

[Hubert Figuiere [:hub]]

(In reply to David Bolter [:davidb] from comment #6)
 
> ::: accessible/src/base/nsAccessibilityService.h
> @@ +48,5 @@
> > +
> > +bool IsVoiceOverChecked();
> > +  
> > +/** we need to check VoiceOver */
> > +void NeedCheckVoiceOver(bool aNeed);
> 
> What is this for? ( And where is it? :) )


In the missing file.

Checking the CFPreference is time consuming, so we do only once and get signaled if the state as likely changed.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 628890 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

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

::: accessible/src/mac/ApplicationAccessibleWrap.mm
@@ +19,5 @@
> +  
> +void NeedCheckVoiceOver(bool aNeed)
> +{
> +  gVoiceOverChecked = !aNeed;
> +}

Could you just remove IsVoiceOverChecked and NeedCheckVoiceOver and use static bool sVoiceOverChecked like you do with sShouldBeEnabled?

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 628890 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

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

f+ (on the right track)

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1775,5 @@
>  
> +#ifdef XP_MACOSX
> +  if (!ShouldA11yBeEnabled())
> +    return NS_ERROR_FAILURE;
> +#endif

I haven't thought deeply if this is the right place to check. I'll trust you and the reviewers :)

Comment 12

[Hubert Figuiere [:hub]]

> ::: accessible/src/mac/ApplicationAccessibleWrap.mm
> @@ +19,5 @@
> > +  
> > +void NeedCheckVoiceOver(bool aNeed)
> > +{
> > +  gVoiceOverChecked = !aNeed;
> > +}
> 
> Could you just remove IsVoiceOverChecked and NeedCheckVoiceOver and use
> static bool sVoiceOverChecked like you do with sShouldBeEnabled?

Good point. I think it is what is left of a much more spread out code as the override of the app Obj-C method was implemented elswhere.

Comment 13

[Hubert Figuiere [:hub]]

(In reply to David Bolter [:davidb] from comment #11)
> Comment on attachment 628890 [details] [diff] [review]
> Only instanciate a11y on Mac if VO is running. r=
> 
> Review of attachment 628890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f+ (on the right track)
> 
> ::: accessible/src/base/nsAccessibilityService.cpp
> @@ +1775,5 @@
> >  
> > +#ifdef XP_MACOSX
> > +  if (!ShouldA11yBeEnabled())
> > +    return NS_ERROR_FAILURE;
> > +#endif
> 
> I haven't thought deeply if this is the right place to check. I'll trust you
> and the reviewers :)

If I put it in the same place as in Atk, in the ApplicationAccessibleWrap::Init() method, then it might be too late. I'd rather not instanciate the service.

I'm open to other ideas.

Comment 14

[Hubert Figuiere [:hub]]

Attachment: Only instanciate a11y on Mac if VO is running. r=

Comment 15

[Hubert Figuiere [:hub]]

Comment on attachment 629020 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

The difference with the previous patch is that I no longer check the preferences, just the change of AXEnhancedUserInterface. Preferences were unreliable. This seems to be the approach taken by Chromium.

I wonder if there is a cleaner way to no create the Accessibility Service.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #15)
> Comment on attachment 629020 [details] [diff] [review]
> Only instanciate a11y on Mac if VO is running. r=
> 
> The difference with the previous patch is that I no longer check the
> preferences, just the change of AXEnhancedUserInterface. Preferences were
> unreliable. This seems to be the approach taken by Chromium.
> 
> I wonder if there is a cleaner way to no create the Accessibility Service.

what we do on other platforms is check if we want accessibility on before fireing a nsAccessibleEvent which will create it look at widget/gtk2/nsWindow.cpp for an example.  In this case you'd check a11y::IsAccessibilityEnabled() or whatever it is in nsChildView::GetDocumentAccessible()

Comment 17

[Trevor Saunders (:tbsaunde)]

Comment on attachment 629020 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

>+@interface GeckoNSApplication(a11y)
>+-(void)accessibilitySetValue:(id)value forAttribute:(NSString*)attribute;

does this always get called during startup?

I'd also llike to see this reworked per my previous comment.

Comment 18

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> Comment on attachment 629020 [details] [diff] [review]
> Only instanciate a11y on Mac if VO is running. r=
> 
> >+@interface GeckoNSApplication(a11y)
> >+-(void)accessibilitySetValue:(id)value forAttribute:(NSString*)attribute;
> 
> does this always get called during startup?

No. This gets called by the AXClient when setting an attribute on the AXApplication object. In our specific check our code is only called when that attribute is set (by VoiceOver).

> I'd also llike to see this reworked per my previous comment.

I doubt that whatever is done will have an influence on this specific bit.

Comment 19

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #16)
> (In reply to Hub Figuiere [:hub] from comment #15)
> > Comment on attachment 629020 [details] [diff] [review]
> > Only instanciate a11y on Mac if VO is running. r=
> > 
> > The difference with the previous patch is that I no longer check the
> > preferences, just the change of AXEnhancedUserInterface. Preferences were
> > unreliable. This seems to be the approach taken by Chromium.
> > 
> > I wonder if there is a cleaner way to no create the Accessibility Service.
> 
> what we do on other platforms is check if we want accessibility on before
> fireing a nsAccessibleEvent which will create it look at
> widget/gtk2/nsWindow.cpp for an example.  In this case you'd check
> a11y::IsAccessibilityEnabled() or whatever it is in
> nsChildView::GetDocumentAccessible()

What we do on Linux mean that to enable accessibility we have to restart the browser. In my case I instantiate accessibility on demand. When VoiceOver starts, a11y starts. (we never stop it).

and yes. to supplement the answer in comment 18, if VoiceOver is runing, the attribute is set relatively early in the startup phase when VoiceOver notice the application.

Comment 20

[Trevor Saunders (:tbsaunde)]

> > I'd also llike to see this reworked per my previous comment.
> 
> I doubt that whatever is done will have an influence on this specific bit.

I didn't mean to attach that part to the specific bit.

(In reply to Hub Figuiere [:hub] from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> > (In reply to Hub Figuiere [:hub] from comment #15)
> > > Comment on attachment 629020 [details] [diff] [review]
> > > Only instanciate a11y on Mac if VO is running. r=
> > > 
> > > The difference with the previous patch is that I no longer check the
> > > preferences, just the change of AXEnhancedUserInterface. Preferences were
> > > unreliable. This seems to be the approach taken by Chromium.
> > > 
> > > I wonder if there is a cleaner way to no create the Accessibility Service.
> > 
> > what we do on other platforms is check if we want accessibility on before
> > fireing a nsAccessibleEvent which will create it look at
> > widget/gtk2/nsWindow.cpp for an example.  In this case you'd check
> > a11y::IsAccessibilityEnabled() or whatever it is in
> > nsChildView::GetDocumentAccessible()
> 
> What we do on Linux mean that to enable accessibility we have to restart the
> browser. In my case I instantiate accessibility on demand. When VoiceOver
> starts, a11y starts. (we never stop it).

I don't understand why you think this.  If a nsAccessibleEventI don't understand why you think this, but I'm pretty sure what I suggest works.  the only way accessibility is started is by calling Do_GetService() for it, which platforms cause to happen by firing a nsAccessibleEvent.  So, if nsChildView::GetDocumentAccessible()  doesn't fire a nsAccessibleEvent to get the docAccessible if VO is off then accessibility will not be on, and if it fires that event a11y will be started.

> and yes. to supplement the answer in comment 18, if VoiceOver is runing, the
> attribute is set relatively early in the startup phase when VoiceOver notice
> the application.

ok, so observing is enough, and we don't need to see if it was true to start with then.

Comment 21

[Hubert Figuiere [:hub]]

Attachment: Only instanciate a11y on Mac if VO is running. r=

Comment 22

[Hubert Figuiere [:hub]]

Attachment: Only instanciate a11y on Mac if VO is running. r=

Comment 23

[Trevor Saunders (:tbsaunde)]

Comment on attachment 630034 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

>--- /dev/null
>+++ b/accessible/src/mac/ApplicationAccessibleWrap.mm
>@@ -0,0 +1,39 @@
>+/* -*- Mode: Objective-C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

vim one too :)

>+bool ShouldA11yBeEnabled()

nit, type on own line

>+  if ([attribute isEqualToString:@"AXEnhancedUserInterface"])
>+    mozilla::a11y::sA11yShouldBeEnabled = ([value intValue] == 1);

I'd probably use using here in case we add more stuff one day.

Comment 24

[Hubert Figuiere [:hub]]

@smichaud: I only request review on the parts in nsAppShell that are not a11y related, but needed for me to implement a category for a11y.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 630034 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=

> @smichaud: I only request review on the parts in nsAppShell that are not
> a11y related, but needed for me to implement a category for a11y.

OK then.  I'm just r+ing the changes to nsAppShell.h and nsAppShell.mm, which are trivial and harmless.

Comment 26

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fafddce7a330

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fafddce7a330