344896 - refactor accessibility events handling

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Previously AaronL describes how accessibility handles events:
http://groups.google.com/group/mozilla.dev.accessibility/browse_thread/thread/c2734a0f354bae6c

nsRootAccessible::GetTargetNode contains lines:

if (content && content->IsContentOfType(nsIContent::eHTML) &&
    (bindingParent = content->GetBindingParent()) != nsnull) { 

It allows to handle both widgets like xul:textbox and xul:dialog. But xforms widgets are don't fit in it. For example xf:input for xul has the next sturcture:

<xf:input>
  <!-- anonymous nodes -->
  <xul:textbox>
    <!-- anonymous nodes -->
    <html:input type="text"/>
    <!-- anonymous nodes -->
  </xul:textbox>
  <!-- anonymous nodes -->
</xf:input>

Will the following scenario work? We handle event, get original target and go up in tree until we get accessible node. At least it should work for xul:textbox, xul:dialog, xf:input. What about the rest of controls?

Comment 1

[Aaron Leventhal]

There is a problem with that, I think:

If you ask the accessibilityservice for an accessible for the anonymous HTML input, it will create one for you.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Aaron Leventhal]

Comment on attachment 229858 [details] [diff] [review]
patch

  /**
+   * True if there are not accessible children in anonymous content.
+   */
+  readonly attribute boolean hasAnonymousChildren;
+
It looks like this comment should remove the word "not".

Do you think it should be a public method or is it only used by our implementation. If it's just an implementation method you can put it in nsPIAccessible.

Sr='s have told me never to use an nsCOMPtr as a static, it can cause crashes. You have to addref and release it manually. We do that for the other nsIFoo's in nsAccessNode. See Init/ShutdownXPAccessibility.

Also, gAccessibilityService is long. How about gAccService or gA11yService?

+  PRBool mHasAnonymousAccChildren;
This adds 4 byts per accessible where we don't need to. Since it's a virtual method you can just override it for each class that doesn't want to walk anon content. Then you can also remove  the aWalkAnonContent from CacheChildren() and just use the results of the method instead, in order to line them up.

Comment 4

[Aaron Leventhal]

I need to look at your GetTargetNode() code after lunch :)

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

The CallGetService() function is defined as:
CallGetService(const nsCID &aClass, const nsIID &aIID, void **aResult);

But it is used as:
nsISomeService *sService;
CallGetService(CONTRACTID, &sService);

Can you clarify this magic?

Comment 6

[Aaron Leventhal]

Comment on attachment 230273 [details] [diff] [review]
patch2

Personally I think we can be safe just asserting sAccService is not null, because how would we even get into the accessibility code without an accessibility service.

Comment 7

[Aaron Leventhal]

No QI to nsIAccessibleRetrieval necessary because of:
interface nsIAccessibilityService : nsIAccessibleRetrieval

Also, I think the comments section of GetTargetNode is very important. I would like to see an explanation of what originalTarget is. I would also like to see some examples. It's nice to know when this is used and how. This is going to be confusing code for future developers unless we comment it very well.

Still reading.

Comment 8

[Aaron Leventhal]

If we have a different original target and target, will the target always be in the bindingParent chain of the originalTarget?

If so, I think what we want is to use the closest node to originalTarget where no ancestor disallows anonymous child accessibles. Correct?

Comment 9

[alexander :surkov (:asurkov)]

Attachment: allowsAnonChildAccessibles patch

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 230445 [details] [diff] [review]
allowsAnonChildAccessibles patch

Sorry, wrong patch

Comment 11

[alexander :surkov (:asurkov)]

Attachment: allowsAnonChildAccessibles patch2

Comment 12

[alexander :surkov (:asurkov)]

Attachment: allowsAnonChildAccessibles patch3

Comment 13

[Aaron Leventhal]

Comment on attachment 230456 [details] [diff] [review]
allowsAnonChildAccessibles patch3

This is making me crash when I load up MSAA Inspect. The InitXPAccessibility() can't set sAccessibilityService because it doesn't exist yet.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: allowsAnonChildAccessibles patch4

Comment 15

[Aaron Leventhal]

allowsAnonChildAccessibles part checked in. Leaving open for GetTargetNode() changes.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: GetTargetNode

Comment 17

[Aaron Leventhal]

Yikes! This hangs an infinite loop.
+      while ((bindingParent = content->GetBindingParent()) != nsnull) {
+        bindingsStack.AppendObject(bindingParent);
+      }

Make sure you test the next patch before you send it to me :b
You can test it with Inspect or a demo version of Window-Eyes from www.gwmicro.com.

I like the new comments. The code could still benefit from more comments inline, explaining what the bindingsStack is. For example:
// Build stack of binding parents so we can walk it in reverse
Also, I think if you Prepend the bindingParents into the array it will be a little clearer, because to me it makes sense if the array goes in top-down order. Not sure if you need to delete items as you use them.

+    if (bindingsCount) {
Either comment why this condition is used, or have a temporary variable PRBool  useBindingParent that you set in the loop to make it totally clear.

Comment 18

[alexander :surkov (:asurkov)]

Attachment: GetTargetNode2

> Yikes! This hangs an infinite loop.

I'm so sorry. There was something wrong in my tests. I fixed that what your comments is about.

Comment 19

[Aaron Leventhal]

Comment on attachment 233457 [details] [diff] [review]
GetTargetNode2

This works. There is one possible small improvement that I should have thought of earlier.
Instead of using a temporary variable like useBindingParent here:
if (!allowsAnonChildren) {
  useBindingParent = PR_TRUE;
  break;
}
You could just do:
if (!allowsAnonChildren) {
  CallQueryInterface(bindingParent, aTargetNode);
  NS_ASSERTION(*aTargetNode, "No target node for binding parent of anonymous event target");
  return;
}

An then you don't need the first part of the if() or the else after the loop.

I'll leave that up to you.

Comment 20

[alexander :surkov (:asurkov)]

Just an example:

interface nsIInterface{
  attribute boolean Bool;
  void Method();
};

class Class1: public nsIInterface{
};
class Class2: public Class1{
};

NS_IMETHODIMP Class1::GetBool(PRBool *aVal)
{
  *aVal = PR_FALSE;
  return NS_OK;
}
NS_IMETHODIMP Class1::Method()
{
  PRBool val;
  GetBool(&val);
  return NS_OK;
}
NS_IMETHODIMP Class2::GetBool(PRBool *aVal)
{
  *aVal = PR_FALSE;
  retunr NS_OK;
}

then I get nsIInterface for Class2 object and call Method() method. So what will GetBool() be called? Does interface method implementations acts like virtual functions?

It looks like allowsAnonChildAccessibles is called for nsAccessible object all the time even if successors object is override the method.

Comment 21

[Aaron Leventhal]

(In reply to comment #20)
> Just an example:
> 
> interface nsIInterface{
>   attribute boolean Bool;
>   void Method();
> };
> 
> class Class1: public nsIInterface{
> };
> class Class2: public Class1{
> };
> 
> NS_IMETHODIMP Class1::GetBool(PRBool *aVal)
> {
>   *aVal = PR_FALSE;
>   return NS_OK;
> }
> NS_IMETHODIMP Class1::Method()
> {
>   PRBool val;
>   GetBool(&val);
>   return NS_OK;
> }
> NS_IMETHODIMP Class2::GetBool(PRBool *aVal)
> {
>   *aVal = PR_FALSE;
>   retunr NS_OK;
> }
> 
> then I get nsIInterface for Class2 object and call Method() method. So what
> will GetBool() be called? 
Class2 will be called.

> Does interface method implementations acts like
> virtual functions?
Exactly. See the xpidl compiler output for the idl. It creates pure virtual functions and puts them in the nsIFoo.h output file.

> It looks like allowsAnonChildAccessibles is called for nsAccessible object all
> the time even if successors object is override the method.
No that's not right.
 

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 233457 [details] [diff] [review]
GetTargetNode2

I should fix aaronlev's issues. And it looks like some accessible objects for xul elements become broken. I'll put new patch later.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 233457 [details] [diff] [review]
GetTargetNode2

+      nsCOMArray<nsIContent> bindingsStack;
+      for (bindingParent = content->GetBindingParent(); bindingParent != nsnull;
+           bindingParent = bindingParent->GetBindingParent()) {
+        bindingsStack.InsertObjectAt(bindingParent, 0);
+      }
+
+      PRInt32 bindingsCount = bindingsStack.Count();
+      for (PRInt32 index = 0; index < bindingsCount; index++) {
+        bindingParent = bindingsStack[index];

Since you're putting up a new patch, I would also recommend reversing how you build/iterate over the bindingsStack array. Rather than always inserting at the beginning of the array (which gets more and more expensive as the array grows) and iterating from the beginning to the end you'd get faster code if you'd always append to the end of the array and iterate from the end to beginning. Lookup is constant time, and appending is cheaper than inserting at the beginning.

Comment 24

[Aaron Leventhal]

My bad Surkov -- I told you to reverse the array. You had it the way Johnny suggests. However, is that really true that inserting is O(n) for the size of the array?

Comment 25

[Aaron Leventhal]

Johnny, I'm the one who told him to use InsertAt(). I didn't realize it was O(N) for the size of the array, that seems odd.

In any case, the array should never be larger than 3, I don't believe. *Maybe* 4. I don't think it's an issue and it made the code a little simpler.

Is it necessary to change it? (Looks like a recommendation not an sr+ requirement)

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> Johnny, I'm the one who told him to use InsertAt(). I didn't realize it was
> O(N) for the size of the array, that seems odd.
> 
> In any case, the array should never be larger than 3, I don't believe. *Maybe*
> 4. I don't think it's an issue and it made the code a little simpler.
> 
> Is it necessary to change it? (Looks like a recommendation not an sr+
> requirement)
> 

If Append() is more faster then I'd prefer to use it. I guess code doesn't lose obviousness a much.

Comment 27

[Johnny Stenback (:jst)]

Since the numbers here are really low I don't think it's *necessary* to reverse the array again, but I'm not going to hold anyone back who wants to do it anyways :)

Comment 28

[alexander :surkov (:asurkov)]

Attachment: AnonChildAlly patch1

After patch of bug 349885 will be checked, events handling for xul:textbox, for example, will be broken.