Closed
Bug 759736
Opened 13 years ago
Closed 13 years ago
Add VO-only mode for FF a11y on mac.
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: davidb, Assigned: hub)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
tbsaunde
:
review+
smichaud
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
(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.
Reporter | ||
Comment 2•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → hub
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
Assignee | ||
Comment 5•13 years ago
|
||
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.
Attachment #628866 -
Flags: feedback?(dbolter)
Reporter | ||
Comment 6•13 years ago
|
||
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? :)
Attachment #628866 -
Flags: feedback?(dbolter)
Assignee | ||
Comment 7•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #628866 -
Attachment is obsolete: true
Assignee | ||
Comment 8•13 years ago
|
||
Comment on attachment 628890 [details] [diff] [review]
Only instanciate a11y on Mac if VO is running. r=
Updated patch: was missing a file.
Attachment #628890 -
Flags: feedback?(dbolter)
Assignee | ||
Comment 9•13 years ago
|
||
(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.
Reporter | ||
Comment 10•13 years ago
|
||
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?
Reporter | ||
Comment 11•13 years ago
|
||
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 :)
Attachment #628890 -
Flags: feedback?(dbolter) → feedback+
Assignee | ||
Comment 12•13 years ago
|
||
> ::: 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.
Assignee | ||
Comment 13•13 years ago
|
||
(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.
Assignee | ||
Comment 14•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #628890 -
Attachment is obsolete: true
Assignee | ||
Comment 15•13 years ago
|
||
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.
Attachment #629020 -
Flags: review?(trev.saunders)
Comment 16•13 years ago
|
||
(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•13 years ago
|
||
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.
Attachment #629020 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 18•13 years ago
|
||
(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.
Assignee | ||
Comment 19•13 years ago
|
||
(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•13 years ago
|
||
> > 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.
Assignee | ||
Comment 21•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #629020 -
Attachment is obsolete: true
Assignee | ||
Comment 22•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #630030 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #630034 -
Flags: review?(trev.saunders)
Attachment #630034 -
Flags: review?(smichaud)
Comment 23•13 years ago
|
||
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.
Attachment #630034 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 24•13 years ago
|
||
@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•13 years ago
|
||
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.
Attachment #630034 -
Flags: review?(smichaud) → review+
Assignee | ||
Comment 26•13 years ago
|
||
Target Milestone: --- → mozilla16
Comment 27•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•