357805 - [Mac a11y] Implement special class for simple action elements (checkbox, button, etc)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Håkan Waara]

The simplest way to make XUL checkboxes, buttons, etc work is to create a new class, mozActionElement, that has the click action.

This is necessary so that only the right attributes (according to Apple's role spec at http://tinyurl.com/y5v5bb).

We can get lots of functionality from the superclass mozAccessible for free, and override some methods to make them cheaper (not look up unnecessary info).

These action elements are always flat (see bug 357804)

Comment 1

[Håkan Waara]

Attachment: New classes mozButtonAccessible and mozCheckboxAccessible

These are the new classes. They are very simple. To get a feel for how a simple accessibility object is implemented, this is a good case.

They support only the click action (AXPress), and report their state is unchecked/checked, etc to the AT.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 243329 [details] [diff] [review]
New classes mozButtonAccessible and mozCheckboxAccessible

>Index: mozActionElements.h
>===================================================================
>RCS file: mozActionElements.h
>diff -N mozActionElements.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ mozActionElements.h	24 Oct 2006 10:34:06 -0000
>@@ -0,0 +1,12 @@
>+#import <Cocoa/Cocoa.h>
>+#import "mozAccessible.h"
>+
>+/* Simple subclasses for things like checkboxes, buttons, etc. */
>+
>+@interface mozButtonAccessible : mozAccessible
>+- (void)click;
>+@end
>+

>+- (void)accessibilityPerformAction:(NSString*)action 
>+{
>+  if ([action isEqualToString:NSAccessibilityPressAction])
>+    [self click];
>+}
>+
>+- (void)click
>+{
>+  // both buttons and checkboxes have only one action. we should really stop using arbitrary
>+  // arrays with actions, and define constants for these actions.
>+  mGeckoAccessible->DoAction(0);
>+}

Why is click method needed? If you assume to use it only here then I'd suggest to get rid it. So I think you can just call DoAction() in accessibilityPerformAction.


>+@interface mozCheckboxAccessible : mozButtonAccessible
>+- (BOOL)isChecked;
>+@end

>+@implementation mozCheckboxAccessible
>+
>+- (NSString*)accessibilityActionDescription:(NSString*)action 
>+{
>+  if ([action isEqualToString:NSAccessibilityPressAction]) {
>+    if ([self isChecked])
>+      return @"uncheck checkbox";
>+    
>+    return @"check checkbox";
>+  }
>+  
>+  return nil;
>+}
>+
>+- (BOOL)isChecked
>+{
>+  PRUint32 state = 0;
>+  mGeckoAccessible->GetState(&state);
>+  return ((state & nsIAccessible::STATE_CHECKED) != 0);
>+}
>+
>+- (id)value
>+{
>+  // need to handle mixed state here too (return 2)
>+  return [NSNumber numberWithInt:([self isChecked] ? 1 : 0)];
>+}
>+
>+@end

I'm not sure I like a lot isChecked method. 1) can you use nsIAccessible::GetValue() to implement value method (probably you are able to just redirect call to GetValue() method)? 2) And accessibilityActionDescription method can call nsIAccessible::GetActionName(). But again I'm not sure what is more nice, it's up to you.

>+- (NSArray*)accessibilityAttributeNames
>+{
>+  static NSArray *attributes = nil;
>+  if (!attributes) {
>+    attributes = [[NSArray alloc] initWithObjects:NSAccessibilityParentAttribute, // required
>+                                                  NSAccessibilityRoleAttribute, // required
>+                                                  NSAccessibilityPositionAttribute, // required
>+                                                  NSAccessibilitySizeAttribute, // required
>+                                                  NSAccessibilityWindowAttribute, // required
>+                                                  NSAccessibilityPositionAttribute, // required
>+                                                  kTopLevelUIElementAttribute, // required
>+                                                  NSAccessibilityHelpAttribute,
>+                                                  NSAccessibilityEnabledAttribute, // required
>+                                                  NSAccessibilityFocusedAttribute, // required
>+                                                  NSAccessibilityTitleAttribute, // required
>+                                                  kInstanceDescriptionAttribute,
>+                                                  nil];
>+  }
>+  return attributes;
>+}

mozAccessible already defines accessibilityAttributeNames that returns some of these constants. Since mozAccessible is base class then it looks reasonable to resure it.

>+- (BOOL)accessibilityIsIgnored
>+{
>+  return NO;
>+}

Why is always 'no'? Should you test mozAccessible::mIsExpired?

>+- (NSString*)accessibilityActionDescription:(NSString*)action 
>+{
>+  if ([action isEqualToString:NSAccessibilityPressAction])
>+    return @"press button";
>+    
>+  return nil;
>+}
>+

Accessibility Cocoa documentation says that should be localized string. How are you going to do it?

Comment 3

[Håkan Waara]

(In reply to comment #2)
> Why is click method needed? If you assume to use it only here then I'd suggest
> to get rid it. So I think you can just call DoAction() in
> accessibilityPerformAction.

What if some other action element later wants to do something else than DoAction(0) on click? Then it would be very easy to just override click.

> 
> 
> >+@interface mozCheckboxAccessible : mozButtonAccessible
> >+- (BOOL)isChecked;
> >+@end
> 
> >+@implementation mozCheckboxAccessible
> >+
> >+- (NSString*)accessibilityActionDescription:(NSString*)action 
> >+{
> >+  if ([action isEqualToString:NSAccessibilityPressAction]) {
> >+    if ([self isChecked])
> >+      return @"uncheck checkbox";
> >+    
> >+    return @"check checkbox";
> >+  }
> >+  
> >+  return nil;
> >+}
> >+
> >+- (BOOL)isChecked
> >+{
> >+  PRUint32 state = 0;
> >+  mGeckoAccessible->GetState(&state);
> >+  return ((state & nsIAccessible::STATE_CHECKED) != 0);
> >+}
> >+
> >+- (id)value
> >+{
> >+  // need to handle mixed state here too (return 2)
> >+  return [NSNumber numberWithInt:([self isChecked] ? 1 : 0)];
> >+}
> >+
> >+@end
> 
> I'm not sure I like a lot isChecked method. 1) can you use
> nsIAccessible::GetValue() to implement value method (probably you are able to
> just redirect call to GetValue() method)? 2) And accessibilityActionDescription
> method can call nsIAccessible::GetActionName(). But again I'm not sure what is
> more nice, it's up to you.

Are you sure GetValue() even provides that info? It seems the way to find out a checkbox's value is by using GetState(). See http://lxr.mozilla.org/mozilla/source/accessible/src/html/nsHTMLFormControlAccessible.cpp#107

> >+- (NSArray*)accessibilityAttributeNames
> >+{
> >+  static NSArray *attributes = nil;
> >+  if (!attributes) {
> >+    attributes = [[NSArray alloc] initWithObjects:NSAccessibilityParentAttribute, // required
> >+                                                  NSAccessibilityRoleAttribute, // required
> >+                                                  NSAccessibilityPositionAttribute, // required
> >+                                                  NSAccessibilitySizeAttribute, // required
> >+                                                  NSAccessibilityWindowAttribute, // required
> >+                                                  NSAccessibilityPositionAttribute, // required
> >+                                                  kTopLevelUIElementAttribute, // required
> >+                                                  NSAccessibilityHelpAttribute,
> >+                                                  NSAccessibilityEnabledAttribute, // required
> >+                                                  NSAccessibilityFocusedAttribute, // required
> >+                                                  NSAccessibilityTitleAttribute, // required
> >+                                                  kInstanceDescriptionAttribute,
> >+                                                  nil];
> >+  }
> >+  return attributes;
> >+}
> 
> mozAccessible already defines accessibilityAttributeNames that returns some of
> these constants. Since mozAccessible is base class then it looks reasonable to
> resure it.
I would do that, if there was an NSArray/NSMutableArray API for creating an array based on another array. :-(

> 
> >+- (BOOL)accessibilityIsIgnored
> >+{
> >+  return NO;
> >+}
> 
> Why is always 'no'? Should you test mozAccessible::mIsExpired?

Ok.

> 
> >+- (NSString*)accessibilityActionDescription:(NSString*)action 
> >+{
> >+  if ([action isEqualToString:NSAccessibilityPressAction])
> >+    return @"press button";
> >+    
> >+  return nil;
> >+}
> >+
> 
> Accessibility Cocoa documentation says that should be localized string. How are
> you going to do it?
> 

You're right. It looks like I can use nsAccessible::GetTranslatedString() whick will use the accessible.properties in file, though!

Hopefully, I can customize that file, so I can use it both for the action name and action description.

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 243329 [details] [diff] [review]
New classes mozButtonAccessible and mozCheckboxAccessible

(In reply to comment #3)
> (In reply to comment #2)
> > Why is click method needed? If you assume to use it only here then I'd suggest
> > to get rid it. So I think you can just call DoAction() in
> > accessibilityPerformAction.
> 
> What if some other action element later wants to do something else than
> DoAction(0) on click? Then it would be very easy to just override click.

Perhaps, it's up to you. Just one thing confuse me is you define too many small functions. Probably they will become hard readable code.

> > I'm not sure I like a lot isChecked method. 1) can you use
> > nsIAccessible::GetValue() to implement value method (probably you are able to
> > just redirect call to GetValue() method)? 2) And accessibilityActionDescription
> > method can call nsIAccessible::GetActionName(). But again I'm not sure what is
> > more nice, it's up to you.
> 
> Are you sure GetValue() even provides that info? It seems the way to find out a
> checkbox's value is by using GetState(). See
> http://lxr.mozilla.org/mozilla/source/accessible/src/html/nsHTMLFormControlAccessible.cpp#107

I'm not sure. My point is code will more readable if you will redirect mac methods to similiar methods of mozilla. But again, it's up to you.


> > >+- (NSArray*)accessibilityAttributeNames
> > >+{
> > >+  static NSArray *attributes = nil;
> > >+  if (!attributes) {
> > >+    attributes = [[NSArray alloc] initWithObjects:NSAccessibilityParentAttribute, // required
> > >+                                                  NSAccessibilityRoleAttribute, // required
> > >+                                                  NSAccessibilityPositionAttribute, // required
> > >+                                                  NSAccessibilitySizeAttribute, // required
> > >+                                                  NSAccessibilityWindowAttribute, // required
> > >+                                                  NSAccessibilityPositionAttribute, // required
> > >+                                                  kTopLevelUIElementAttribute, // required
> > >+                                                  NSAccessibilityHelpAttribute,
> > >+                                                  NSAccessibilityEnabledAttribute, // required
> > >+                                                  NSAccessibilityFocusedAttribute, // required
> > >+                                                  NSAccessibilityTitleAttribute, // required
> > >+                                                  kInstanceDescriptionAttribute,
> > >+                                                  nil];
> > >+  }
> > >+  return attributes;
> > >+}
> > 
> > mozAccessible already defines accessibilityAttributeNames that returns some of
> > these constants. Since mozAccessible is base class then it looks reasonable to
> > resure it.
> I would do that, if there was an NSArray/NSMutableArray API for creating an
> array based on another array. :-(

Section "Overriding Methods to Support Attributes" of Accessibility Cocoa documentation contains the similiar example. Does it not work here?

> > 
> > >+- (BOOL)accessibilityIsIgnored
> > >+{
> > >+  return NO;
> > >+}
> > 
> > Why is always 'no'? Should you test mozAccessible::mIsExpired?
> 
> Ok.

I think, you are able just inherit accessibilityIsIgnored from mozAccessible. Right?

> > Accessibility Cocoa documentation says that should be localized string. How are
> > you going to do it?
> > 
> 
> You're right. It looks like I can use nsAccessible::GetTranslatedString() whick
> will use the accessible.properties in file, though!
> 
> Hopefully, I can customize that file, so I can use it both for the action name
> and action description.
> 

So, either fix it or file bug and add XXX section.

With answered questions r= me

Comment 5

[Håkan Waara]

(In reply to comment #4)
> Section "Overriding Methods to Support Attributes" of Accessibility Cocoa
> documentation contains the similiar example. Does it not work here?

No, because there's only an API for adding 1 object to an existing array like that, there's no simple way to append an array to another array.

> I think, you are able just inherit accessibilityIsIgnored from mozAccessible.
> Right?

I'll just check mIsExpired, to avoid doing a lookup for the role that is done in mozAccessible's isIgnored.

> 
> > > Accessibility Cocoa documentation says that should be localized string. How are
> > > you going to do it?
> > > 
> > 
> > You're right. It looks like I can use nsAccessible::GetTranslatedString() whick
> > will use the accessible.properties in file, though!
> > 
> > Hopefully, I can customize that file, so I can use it both for the action name
> > and action description.
> > 

Aaron said he was not sure I should localize these. It's easy to fix later if we want, so I will not do that for now unless we want to later.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> Aaron said he was not sure I should localize these. It's easy to fix later if
> we want, so I will not do that for now unless we want to later.
> 
I don't like to broke the spec but it's fine with me if you will not fix this right now, then just put XXX comment.

Comment 7

[Stuart Morgan]

(In reply to comment #3)
> I would do that, if there was an NSArray/NSMutableArray API for creating an
> array based on another array. :-(

(In reply to comment #5)
> No, because there's only an API for adding 1 object to an existing array like
> that, there's no simple way to append an array to another array.

arrayByAddingObjectsFromArray:? addObjectsFromArray:?

Comment 8

[Håkan Waara]

(In reply to comment #7)
> (In reply to comment #3)
> > I would do that, if there was an NSArray/NSMutableArray API for creating an
> > array based on another array. :-(
> 
> (In reply to comment #5)
> > No, because there's only an API for adding 1 object to an existing array like
> > that, there's no simple way to append an array to another array.
> 
> arrayByAddingObjectsFromArray:? addObjectsFromArray:?

Maybe I remembered wrong; it just hit me that another reason I can't do it is because I don't want to inherit attributes that these action elements will not support (that mozAccessible does).

Comment 9

[Håkan Waara]

Attachment: Final mozActionElements patch

Comment 10

[Håkan Waara]

Finally checked in to trunk, thanks for the reviews!