342146 - Implement basic support for windows/documents and hook up to widget/

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Håkan Waara]

This bug is for the work to hook up window/document support for the mac a11y, as well as the code that hooks up widget/src/cocoa up to the accessibility module.

Comment 1

[Håkan Waara]

Attachment: accessible/ WIP patch

This is my work in progress patch so far in accessible/. 

Still not working, I'm trying to resolve a hang I get when going up the accessible tree.

Comment 2

[Håkan Waara]

Attachment: WIP patch in widget/

Comment 3

[Håkan Waara]

Attachment: WIP patch v2

Making progress...

I fixed some issues:

* Infinite loop in the construction of native accessibles
* Incorrect usage of objective c's |super| caused exceptions and crashes

Now the window is almost properly hooked up. I need to get hit testing working, and add a mozRootAccessible native accessible.   Slowly getting there!

Comment 4

[Håkan Waara]

Attachment: WIP patch v3

* I'm hard at work on a new approach in the widget/ land.  I'm subclassing NSWindow so I can divert all accessibility calls right down into accessible/. Non-working yet.

* Lots of lazy loading

* Implemented more attributes that mozAccessible implements

Comment 5

[Håkan Waara]

Attachment: WIP patch v4

Still working on this. I've revamped the widget/ part yet again, after I finally got in touch with some mozilla-mac people.

The owner of the accessibility tree will be the ChildView, and I won't touch the window at all. This is more embedding-friendly too, because embeddors might have their own window subclass.

* Started implementing support for position/size in mozAccessible

Still trying to work out the quirks of why accessible testing tools more or less ignore my abstract class (doesn't even query it for the attributes it supports). Just posted a question to apple's a11y list too about this.

Comment 6

[Håkan Waara]

Attachment: WIP v5

More work in progress. Finally fixed so native accessibles show up in the accessibility hierarchy!  

Squashed a bunch of bugs, and trying to work out lots of quirks to get the OS to "see" the accessibles when I mouse over them. Still working on that.

Comment 7

[Håkan Waara]

Doing some serious progress here these days; I got hit-testing working 70% and *a lot* of accessibility objects that I didn't expect are showing up :-)

Due to internet-outage at my apt, I'm hacking offline.

Comment 8

[Håkan Waara]

Attachment: WIP v6

Posting what I have to make sure everyone knows I'm putting sweat and blood into this, as well as pulling out the few gray hairs I have left.

I think I never estimated this level of complexity. I've rewritten the widget <-> accessible glue maybe 10 times, and finally I think it's on the right track.

Unfortunately, now the accessible code is instead acting up on me. 

I'm doing hit testing where the mouse is, by using nsIAccessible's getChildAtPoint(x, y) API. This is giving me crashes, where it is trying to use a private pointer (|mFirstChild|) which seems to be invalid (already freed?).

Maybe it's out of sync with the cache?   In some parts of the code, the |mFirstChild| member is referred to as a weak reference, yet it is used in other parts of the code as were it always valid. This seems to be a very dangerous assumption indeed.

For the record, here are the things that make my task quite complex:

* NSAccessibility has been sparsely documented by Apple on how it really works under the hood. The major user has been VoiceOver, which is also developed by Apple.

* widget/cocoa hasn't been really stable yet - it's still under development. It's also the only widget toolkit that doesn't have one nsIWidget per window (it may have tens of them). 

This latter is a very significant detail for us, since we get our root accessible by sending an event down the nsIWidget. On Windows/Linux there is only ever one, so it is quite simple. Just getting platform parity here has been a major challenge.

* I'm developing in Objective-C, while the rest of the accessible/ module is in C/C++. The only other part of Gecko that is talking with OS X (and thus has to use Objective-C) is the widget/cocoa code.  The fact that I'm inside another module and have to do the same thing has also been a major challenge.  If I could simply do it in C++, like the other accessible/ kids, this would probably be much simpler.

* No one understands the accessibility code as well as the cocoa/widget code. Well, actually, there is only about 1-2 persons that actually understand what the cocoa/widget code does, and they are usually swamped, so I'm basically on my own. I don't complain though, it's my job :-) but it would help if I could get assistance sometimes.

I'm really stubborn with this, and really want to make it, but like I said, I'm running into major complexity issues, and just want to make sure it's written here for the record.

Comment 9

[Håkan Waara]

Attachment: WIP v7

I've fixed the crashing problems. The problem was that nsAccessNode::Init() was not called for my accessibles, and this method is crucial, because it does all the caching. I had overridden Init() to do my own initing, and since it's virtual, you have to call up to the superclass.

Many, many bugs have been fixed. The accessible tree looks OK when I print it in the console (I can even see the roles), but OS X's accessible tools won't show the tree after the first mozRootAccessible node. They don't give any information as to why. So basically I'm stabbing in the dark.

Some apple engineers on the accessibility list have told me that "the hierarchy is probably screwed up somehow". It's unfortunate that there is no information by Apple's software as to what goes wrong. It just fails silently after it has parsed our accessibility hierarchy.

Comment 10

[Håkan Waara]

Attachment: Screenshot of Firefox inspected with Accessibility Verifier

Major breakthrough, after another day of hard debugging I think I solved the major issue that prevented Apple's tools from showing anything!

Attached is a screenshot showing Firefox's accessibility hierarchy, run through the Accessibility Verifier. You can see the Firefox toolbar, the search menu in there.

Comment 11

[Håkan Waara]

Attachment: WIP v8

This is basically the code I'd like to check-in, and work from. In order to get here, I've had to fix a gecko/widget bug. It's reporting the wrong position/size of frames/nsIWidgets on mac only.

I need to see if I can fix bug 350018 which I consider to be the last blocker bug.

Comment 12

[Håkan Waara]

Attachment: accessible/ portion (checked in)

I would like to check-in what I have now (since the scope of this bug probably has been exceeded anyway), and work from that. Here are the changes to accessible/

* The main object is the mozAccessible, which is a objective-c class. Every nsAccessibleWrap has an associated mozAccessible.

* The rootaccessible and docaccessible have their own mozAccessible-inherited class (mozDocAccessible and mozRootAccessible)

* The C++ classes own their native obj-c instances, and when they die (via Shutdown() or destructor), they also release their ref to the native object.

Comment 13

[Håkan Waara]

Attachment: widget/ portion

Josh, This is the widget/ part of my accessibility work. It's quite straightforward. If possible, please review this as soon as possible, as I'd like to check this in so I can continue working on trunk.

Here's a review of it:

* First and foremost, none of the added code will be used unless ACCESSIBILITY is defined. It's off by default (on mac).

* I make ChildView implement the formal mozAccessible @protocol. This is so ChildView can talk to my new mozAccessible objects over module boundaries without compiler warnings.

* ChildView does almost no work at all, it forward all NSAccessibility calls down to the accessibility module. So, every ChildView has an associated mozAccessible slave. :)

Comment 14

[Håkan Waara]

Comment on attachment 236807 [details] [diff] [review]
widget/ portion

See previous comment for review aid.

Comment 15

[Aaron Leventhal]

I'm not sure how I'm going to review Objective C code very well.\
One first question, can the nsRoleMap code be done as an array instead of as a switch? Would save some codesize.

Comment 16

[Aaron Leventhal]

You can remove the nsAccessNodeWrap class if you're not going to do anything in it. Just use a typedef so that nsAccessNodeWrap becomes the same as nsAccessNode. We do the same thing for other classes, as you've probably seen.

+ NSAccessibilityTopLevelUIElementAttribute, // XXX: 10.4+ only!
+ NSAccessibilityDescriptionAttribute, // XXX: 10.4+ only!
Since all properties are just name, value pairs, how can something only be supported in 10.4+?

//NSAccessibilitySubroleAttribute,
This sounds very interesting. What is it?

Comment 17

[Håkan Waara]

(In reply to comment #15)
> I'm not sure how I'm going to review Objective C code very well.\
> One first question, can the nsRoleMap code be done as an array instead of as a
> switch? Would save some codesize.

I need to return strings (unlike atk/msaa where all roles are integer constats), so that's why I used a switch, but I suppose I could also have a static array of all these strings. Isn't that kind of memory inefficient though?

Comment 18

[Håkan Waara]

(In reply to comment #16)
> You can remove the nsAccessNodeWrap class if you're not going to do anything in
> it. Just use a typedef so that nsAccessNodeWrap becomes the same as
> nsAccessNode. We do the same thing for other classes, as you've probably seen.

OK

> 
> + NSAccessibilityTopLevelUIElementAttribute, // XXX: 10.4+ only!
> + NSAccessibilityDescriptionAttribute, // XXX: 10.4+ only!
> Since all properties are just name, value pairs, how can something only be
> supported in 10.4+?

Good question. I can still support these pre-10.4, but I won't be able to use the constants since they are first defined in the 10.4+ SDK. 

> 
> //NSAccessibilitySubroleAttribute,
> This sounds very interesting. What is it?
> 

It's the subrole attribute. For example, there's the Button role, then there's the IncrementPage subrole that is only used for the down scrollbutton arrow. All Apple's defined subroles can be seen here: http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Protocols/NSAccessibility_Protocol/index.html#//apple_ref/doc/uid/TP40004155

Comment 19

[Aaron Leventhal]

Comment on attachment 236806 [details] [diff] [review]
accessible/ portion (checked in)

One r= will be sufficient.

Comment 20

[Aaron Leventhal]

> + NSAccessibilityTopLevelUIElementAttribute, // XXX: 10.4+ only!
> + NSAccessibilityDescriptionAttribute, // XXX: 10.4+ only!

Can you define these yourself in your own header? We had the same problem with MSAA and ATK. In fact for ATK we just imported up-to-date headers. We had to look at the license, though.

Comment 21

[Håkan Waara]

(In reply to comment #20)
> > + NSAccessibilityTopLevelUIElementAttribute, // XXX: 10.4+ only!
> > + NSAccessibilityDescriptionAttribute, // XXX: 10.4+ only!
> 
> Can you define these yourself in your own header? We had the same problem with
> MSAA and ATK. In fact for ATK we just imported up-to-date headers. We had to
> look at the license, though.

Exactly, I will just define them myself, since they're only strings in the end.

Comment 22

[Håkan Waara]

Comment on attachment 236806 [details] [diff] [review]
accessible/ portion (checked in)

I sent an overview of this patch via email to Ginn and Alex.

Comment 23

[Håkan Waara]

I have lots of things I need to fix, I just wanted to check-in at some point so I can start filing bugs and dividing up tasks.

Since this is not part of the build, can I check-in the accessible/src/mac part without review? I have no problem with fixing review comments and discussing the architecture after this.

Comment 24

[Aaron Leventhal]

(In reply to comment #23)
> Since this is not part of the build, can I check-in the accessible/src/mac part
> without review? I have no problem with fixing review comments and discussing
> the architecture after this.

Okay. Makes sense in the interest of being able to write more non-conflicting patches. 

Comment 25

[Håkan Waara]

Comment on attachment 236807 [details] [diff] [review]
widget/ portion

Requesting from Josh, now that he's back.

Comment 26

[Simon Fraser [no longer active]]

Comment on attachment 236806 [details] [diff] [review]
accessible/ portion (checked in)


>Index: src/mac/mozAccessible.h
>===================================================================

>+@interface mozAccessible : NSObject <mozAccessible>
>+{
>+  nsAccessible    *geckoAccessible;  // weak reference; it owns us.
>+  
>+  NSMutableArray  *children;         // strong ref to array of children
>+  
>+  // we can be marked as 'expired' if Shutdown() is called on our geckoAccessible.
>+  // since we might still be retained by some third-party, we need to do cleanup
>+  // in |expire|, and prevent any potential harm that could come from someone using us
>+  // after this point.
>+  BOOL isExpired;
>+}

Using an "m" prefix on member variables would make the code easier to read.


>+// returns the size of this accessible.
>+- (NSValue*)size;
>+- (NSValue*)position;

Is there a reason not to return NSSize and NSPoint here? KVO works with NSSize, NSPoint, NSRect and NSRange.

>+// called by third-parties to determine the deepest child element under the mouse
>+- (id)accessibilityHitTest:(NSPoint)point;

A more descriptive name would be nice, like "findThinkAtPoint:". "hitTest" doesn't tell me what it returns. Can the return type be more specific?

>Index: src/mac/mozAccessible.mm
>===================================================================



>+  if ([attribute isEqualToString:NSAccessibilityDescriptionAttribute])
>+    return [self customDescription];
>+  else if ([attribute isEqualToString:NSAccessibilityPositionAttribute]) 
>+    return [self position];
>+  else if ([attribute isEqualToString:NSAccessibilityRoleAttribute])
>+    return [self role];
>+  //else if ([attribute isEqualToString:NSAccessibilitySubroleAttribute])
>+    //return @""; // default to no subrole
>+  else if ([attribute isEqualToString:NSAccessibilityValueAttribute])
>+    return [self value];
>+  else if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute])
>+    return @""; // default to no role description
>+  else if ([attribute isEqualToString:NSAccessibilityFocusedUIElementAttribute])
>+    return [self accessibilityFocusedUIElement];
>+  else if ([attribute isEqualToString:NSAccessibilityFocusedAttribute])
>+    return [NSNumber numberWithBool:[self isFocused]];
>+  else if ([attribute isEqualToString:NSAccessibilitySizeAttribute])
>+    return [self size];
>+  else if ([attribute isEqualToString:NSAccessibilityWindowAttribute])
>+    return [self window];
>+  else if ([attribute isEqualToString:NSAccessibilityTopLevelUIElementAttribute])
>+    // most apps seem to return the window as the top level ui element attr.
>+    return [self window];
>+  else if ([attribute isEqualToString:NSAccessibilityTitleAttribute])
>+    return [self title];

Lose all the "else"s, since there are returns.

>+// gets our native children lazily.
>+// returns nil when there are no children.
>+- (NSArray*)children
>+{
>+  // XXXhakan: is there some way we can be notified when a child is added/changed,
>+  // so we can expire then? then we could avoid this expensive check.
>+  PRInt32 count=0;
>+  geckoAccessible->GetChildCount (&count);
>+  
>+  // short-circuit here. if we already have a children
>+  // array, and the count hasn't changed, just return our
>+  // cached version.
>+  if (children && ([children count] == (unsigned)count))
>+    return children;

What if one child was removed, but another added?

When widgets are created and destroyed, is the widget code telling yoiu to invalidate your child arrays?

>+  if (!children)
>+    children = [[NSMutableArray alloc] initWithCapacity:count];
>+  
...
>+  
>+  return (NSArray*)children;

This is being leaked. Also, the cast is not necessary.

Comment 27

[Håkan Waara]

Simon, thanks for the review!

(In reply to comment #26)
> (From update of attachment 236806 [details] [diff] [review] [edit])
> 
> >Index: src/mac/mozAccessible.h
> >===================================================================
> 
> >+@interface mozAccessible : NSObject <mozAccessible>
> >+{
> >+  nsAccessible    *geckoAccessible;  // weak reference; it owns us.
> >+  
> >+  NSMutableArray  *children;         // strong ref to array of children
> >+  
> >+  // we can be marked as 'expired' if Shutdown() is called on our geckoAccessible.
> >+  // since we might still be retained by some third-party, we need to do cleanup
> >+  // in |expire|, and prevent any potential harm that could come from someone using us
> >+  // after this point.
> >+  BOOL isExpired;
> >+}
> 
> Using an "m" prefix on member variables would make the code easier to read.

Hmm, personally I dislike the "m"-prefix in objc-code. The code looks cleaner without it. But I suppose I can use it if you think it helps.

> 
> >+// returns the size of this accessible.
> >+- (NSValue*)size;
> >+- (NSValue*)position;
> 
> Is there a reason not to return NSSize and NSPoint here? KVO works with NSSize,
> NSPoint, NSRect and NSRange.

I *think* that the NSAccessibility APIs want NSValues back. Are you saying they're ok with getting raw NSSizes/NSPoints? I will try.

> 
> >+// called by third-parties to determine the deepest child element under the mouse
> >+- (id)accessibilityHitTest:(NSPoint)point;
> 
> A more descriptive name would be nice, like "findThinkAtPoint:". "hitTest"
> doesn't tell me what it returns. Can the return type be more specific?

This is one of those NSAccessibility APIs (see NSAccessibility.h and related docs). That's why I'm using that name.

Also, I've made an informal protocol, 'mozAccessible' (see mozAccessibleProtocol.h) with the NSAccessibility APIs + some others.

ChildView (in nsChildView.mm) also adheres to this protocol. Actually the point of the protocol is so that mozAccessibles and ChildView can talk to each other across the module boundaries. (See also mozView, which has a similar usage.)

> >Index: src/mac/mozAccessible.mm
> >===================================================================

<snip useful comment>

> 
> >+// gets our native children lazily.
> >+// returns nil when there are no children.
> >+- (NSArray*)children
> >+{
> >+  // XXXhakan: is there some way we can be notified when a child is added/changed,
> >+  // so we can expire then? then we could avoid this expensive check.
> >+  PRInt32 count=0;
> >+  geckoAccessible->GetChildCount (&count);
> >+  
> >+  // short-circuit here. if we already have a children
> >+  // array, and the count hasn't changed, just return our
> >+  // cached version.
> >+  if (children && ([children count] == (unsigned)count))
> >+    return children;
> 
> What if one child was removed, but another added?
> 
> When widgets are created and destroyed, is the widget code telling yoiu to
> invalidate your child arrays?

Good point; I'm not sure about this. I would like to not fetch the whole array of children every time, since this is an expensive operation, but I need to see how aggressively nsIAccessible invalidates children.

> 
> >+  if (!children)
> >+    children = [[NSMutableArray alloc] initWithCapacity:count];
> >+  
> ...
> >+  
> >+  return (NSArray*)children;
> 
> This is being leaked. Also, the cast is not necessary.

No leak; "children" is an instance variable, and it's only being instantiated if null. It's released in dealloc.

Comment 28

[Håkan Waara]

As a solution to the cached children array getting out of sync, I could do this:

* Lazily fetch children when asked for them (like today)
* For every child, setup the parent as a delegate
* If the child is expired (goes away) the parent is notified, so it can clear its cache.

Comment 29

[Håkan Waara]

(In reply to comment #28)
> As a solution to the cached children array getting out of sync, I could do
> this:
> 
> * Lazily fetch children when asked for them (like today)
> * For every child, setup the parent as a delegate
> * If the child is expired (goes away) the parent is notified, so it can clear
> its cache.

Actually, the C++ accessibility code already takes care of invalidating the nodes at the right time (when the DOM tree is changed), so I don't need to do anything.

Comment 30

[Simon Fraser [no longer active]]

(In reply to comment #27)
> Simon, thanks for the review!

> No leak; "children" is an instance variable, and it's only being instantiated
> if null. It's released in dealloc.

This demonstrates why you need a prefix for member vars. :)

Comment 31

[alexander :surkov (:asurkov)]

I have a feel that 'm' prefix using will be not bad, at least it allows to be compatible with other accessibility code. Does object-c++ assume any prefixes using?

Please update your patch with Simon's comments that makes sense.

Probably the mozAccessible protocol should be placed in public/mac directory?

Is it good that mozAccessible protocol has the same name that has mozAccessible class? Does object-c++ protocols have prefix like xpcom interfaces has 'nsI' one? The one and the same name for two different things confuses me.

Can you say me more how it works? I can't see start point from that mac accessible code beging the work. I can assume that should happen with the accessibilityAttributeValue method but I don't see how. If you can then please give me a link to read how to make accessible my mac's application for 3d party softwares.

Comment 32

[Håkan Waara]

(In reply to comment #31)
> Probably the mozAccessible protocol should be placed in public/mac directory?

Good idea.

> Is it good that mozAccessible protocol has the same name that has mozAccessible
> class? Does object-c++ protocols have prefix like xpcom interfaces has 'nsI'
> one? The one and the same name for two different things confuses me.

One of the reasons I use moz* instead of nsI is because nsI already very occupied, by the general interfaces.

> Can you say me more how it works? I can't see start point from that mac
> accessible code beging the work. I can assume that should happen with the
> accessibilityAttributeValue method but I don't see how. If you can then please
> give me a link to read how to make accessible my mac's application for 3d party
> softwares.
> 

First, the root accessible is created (when nsRootAccessibleWrap is created). After that all queries are done via the accessibility* methods.

Here's Apple's documentation on accessibility:
http://developer.apple.com/documentation/Accessibility/Conceptual/AccessibilityMacOSX/index.html

Comment 33

[Håkan Waara]

Comment on attachment 236807 [details] [diff] [review]
widget/ portion

This has been checked in with some stylistic changes requested by Josh.

Mento, would you be able to look it over too?

Comment 34

[Håkan Waara]

Comment on attachment 236806 [details] [diff] [review]
accessible/ portion (checked in)

Checked this in today. It's not part of the build (off by default). 

I will continue development of this in new targetted bugs instead of continuing on this one forever.

Comment 35

[Mark Mentovai]

Comment on attachment 236807 [details] [diff] [review]
widget/ portion

+ result = (nsEventStatus_eConsumeNoDefault == status) ? PR_TRUE : PR_FALSE;

Prevailing style is |var == konstant|.  But you should just say result = ConvertStatus(status);.  Or even just call straight through to DispatchWindowEvent.

Why's the addref in GetDocumentAccessible instead of DispatchAccessibleEvent?  Actually, you only call DispatchAccessibleEvent once, and it's not really a generic Accessible-event dispatcher, it just sends NS_GETACCESSIBILITY.  It seems like you should just have all of this integrated into GetDocumentAccessible and dispense with DispatchAccessibleEvent altogether.

+  id <mozAccessible> nativeAccessible = nil;

id<mozAccessible> - you've also got something similar later in your patch.

+  mGeckoChild->GetDocumentAccessible (getter_AddRefs (accessible));             
+  accessible->GetNativeInterface ((void**)&nativeAccessible);
Far too many spaces.

r+ on the rest of the patch, and to make those changes.

Comment 36

[Håkan Waara]

Attachment: Patch to address mento's review comments (checked in)

Fixed mento's review comments.

Comment 37

[Håkan Waara]

Development continues in other bugs. Thanks for all the reviews!