358381 - [Mac a11y] Misc fixes and new features to mozAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Håkan Waara]

I'm sorry that I can't categorize this patch better. A lot of changes are needed in order to split off text-only functionality to the new mozTextAccessible (bug 357804) and to make buttons/checkboxes work correctly (bug 357805).

In addition, I fixed a number of things in my local tree just before leaving to the a11y hackfest. I'll give a list of all the changes along with the patch.

Comment 1

[Håkan Waara]

Attachment: Patch v1

Changes:

* Cache the role in an mRole instance var. This helps performance a lot, since the role is checked a lot in the code.

* Change name from |ourself| -> |representedView| and use |hasRepresentedView| to find out if an accessible has a parallell view. This looks more cocoaish, and is easier to understand than "ourself".

* Implement some new attributes: enabled/disable state, the help attribute, titleUIElement and custom description.

* Start listening to value-changed events (so far only textfields listen)

* Implement helper function NativeFromGeckoAccessible() to get the native accessible from an nsIAccessible

* Implement subrole method that can be overridden in mozAccessible subclasses (e.g., mozCheckboxAccessible)

* Simplify children/parent methods a lot to use new GetUnignoredChildren() and GetUnignoredParent() methods implemented in bug 357804

* Start using cocoa assertions (NSAssertn, where 'n' is the number of extra args) instead of ugly #ifdef DEBUG_hakan code. This also helps tracking down down regressions.

* Some role changes. For example the application role can't be use, since it is used in mozilla, in the middle of the hierarchy. The OS gets *very* confused about that!   Also, the heading elements are more like groups than text. (Text can't have children in UA.)

I think that should be every change.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 244075 [details] [diff] [review]
Patch v1


> // inits with the gecko owner.
>-- (id)initWithAccessible:(nsAccessible*)geckoParent;
>+- (id)initWithAccessible:(nsAccessibleWrap*)geckoParent;

Why the name of parameter is 'geckoParent', should be just geckoAccessible for example?

>+- (void)valueDidChange;

Why do you need valueDidChange in base control if half of controls isnt' editable?

> - (void)dealloc
> {
>-  // post a notification that we've gone away, if we're not ignored.
>-  // XXX: is this expensive perf-wise?
>-  if (![self accessibilityIsIgnored]) {
>-    NSAccessibilityPostNotification([self ourself],
>-                                    NSAccessibilityUIElementDestroyedNotification);
>-  }

Why do you don't need any more to post notification?

> 
> - (NSArray*)accessibilityAttributeNames
> {
>   // if we're expired, we don't support any attributes.
>   if (mIsExpired)
>     return [NSArray array];
>   
>-  static NSArray *supportedAttributes = nil;
>-  if (!supportedAttributes) {
>+  static NSArray *generalAttributes = nil;
>+  
>+  if (!generalAttributes) {
>     // standard attributes that are shared and supported by all generic elements.
>-    supportedAttributes = [[NSArray alloc] initWithObjects:NSAccessibilityChildrenAttribute, 
>+    generalAttributes = [[NSArray alloc] initWithObjects:  NSAccessibilityChildrenAttribute, 

I think it would be more nice if you static variables initializing will go in some initializing funciton. How about initWithAccessible or?

>                                                            NSAccessibilitySubroleAttribute,

Why do you expose here subrole attribute if not every control supports it?

>+  if ([attribute isEqualToString:kInstanceDescriptionAttribute])
>+    return [self customDescription];

Why method is called as customDescription. Where do you use 'description' name?

Comment 3

[alexander :surkov (:asurkov)]

Also, if it's possible then wrap calls of hasRepresentedView/getRepresentedView into certain method. I think code should be cleaner.

Comment 4

[Håkan Waara]

(In reply to comment #2)
> (From update of attachment 244075 [details] [diff] [review] [edit])
> 
> > // inits with the gecko owner.
> >-- (id)initWithAccessible:(nsAccessible*)geckoParent;
> >+- (id)initWithAccessible:(nsAccessibleWrap*)geckoParent;
> 
> Why the name of parameter is 'geckoParent', should be just geckoAccessible for
> example?

Good point, I'll change it.

> >+- (void)valueDidChange;
> 
> Why do you need valueDidChange in base control if half of controls isnt'
> editable?

I need some stub in the mozAccessible class, because the method is defined on mozAccessible (but only some subclasses actually use it).

> > - (void)dealloc
> > {
> >-  // post a notification that we've gone away, if we're not ignored.
> >-  // XXX: is this expensive perf-wise?
> >-  if (![self accessibilityIsIgnored]) {
> >-    NSAccessibilityPostNotification([self ourself],
> >-                                    NSAccessibilityUIElementDestroyedNotification);
> >-  }
> 
> Why do you don't need any more to post notification?

I've been experimenting with this. It seems to confuse VoiceOver when I post it. But there are also many bugs in the OS that makes this uncertain how it should work ... For example, an Apple employee recently admitted that notifications are very buggy for custom elements right now (http://lists.apple.com/archives/accessibility-dev/2006/Nov/index.html) :-(

Posting a notification is quite expensive, so I'll use it only when I know it's right and it works.

> 
> > 
> > - (NSArray*)accessibilityAttributeNames
> > {
> >   // if we're expired, we don't support any attributes.
> >   if (mIsExpired)
> >     return [NSArray array];
> >   
> >-  static NSArray *supportedAttributes = nil;
> >-  if (!supportedAttributes) {
> >+  static NSArray *generalAttributes = nil;
> >+  
> >+  if (!generalAttributes) {
> >     // standard attributes that are shared and supported by all generic elements.
> >-    supportedAttributes = [[NSArray alloc] initWithObjects:NSAccessibilityChildrenAttribute, 
> >+    generalAttributes = [[NSArray alloc] initWithObjects:  NSAccessibilityChildrenAttribute, 
> 
> I think it would be more nice if you static variables initializing will go in
> some initializing funciton. How about initWithAccessible or?

If I'd define the variable in initWithAccessible (or another method), then I could not use it in this method. Note that the variable is local, even though it's static.

> >                                                            NSAccessibilitySubroleAttribute,
> 
> Why do you expose here subrole attribute if not every control supports it?

Good question. It's required, and UA wants us to return nil (null) when there is no good subrole.

> 
> >+  if ([attribute isEqualToString:kInstanceDescriptionAttribute])
> >+    return [self customDescription];
> 
> Why method is called as customDescription. Where do you use 'description' name?

Oh, it's used just 8 lines below actually. :-)

Note that the 'description' method is special in objective-c. It's like toString() in Java. It returns the string value that will be printed if you print an object ( for example with NSLog(@"This object's debug info: %@", anObject); )

Comment 5

[Håkan Waara]

Attachment: widget/ changes needed

Here are some changes on the widget-a11y side that are needed.

* I've implemented two new mozAccessible protocol methods that tell if an accessible object is working together with a view (ChildView) or not. This is the representedView change.

* Fixed a smallish bug in how we get the document accessible. We were doing the same do_QueryReferent call twice in one case.

Comment 6

[Håkan Waara]

Finally checked in to trunk, thanks for the reviews!