350063 - Window index returned -1 always on calling Accessible_getIndexInParent

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Nagappan]

Window index returned -1 always on calling Accessible_getIndexInParent

More info:
On calling Accessible_getIndexInParent (frame), always returns -1.

* GTK_MODULES environment variable is set
* firefox-3.0a1.en-US.linux-i686.tar.bz2
* Ubuntu Dapper
* at-spi (1.7.7-0), atk (1.11.4-0)

Comment 1

[Nagappan]

FF version: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060824 Minefield/3.0a1

Comment 2

[Aaron Leventhal]

On calling Accessible_getIndexInParent (frame)

What's frame? Any node?

Comment 3

[Aaron Leventhal]

By frame here we're talking about SPI_ROLE_FRAME.

Comment 4

[Aaron Leventhal]

Evan, can you help out with this bug? I believe it relates to how nsAppRootAccessible is made to parent the root accessibles. It's blocking further testing with LDTP.

Comment 5

[Evan Yan]

rootAcc has been added to the appRootAcc's children, but nsAppRootAccessible::GetFirstChild() returns null.

still debugging...

Comment 6

[Evan Yan]

Attachment: patch draft

the cause is mFirstChild and mNextSibling of rootAccWrap dont't work.
so the patch is to make rootAccWrap support mFirstChild and mNextSibling:

+ add nsAppRootAccessible::CacheChildren(), to set mFirstChild/mNextSibling/parent
+ remove GetFirstChild() and GetChildCount, just use nsAccessible's impl;
+ when add/removeRootAccessible, invalidate children.

I hope I won't break anything.

Comment 7

[Aaron Leventhal]

Looks good so far.

Only one problem I see:
1. GetParent() on a root accessible will fail unless CacheChildren() happens to be called on the app root accessible first. This normally isn't a problem because nsAccessible::GetParent() has a fallback of using the tree walker. The tree walker can't work in this case though. You can fix this by overriding GetParent() for this nsRootAccessibleWrap so that if it doesn't have a parent it uses the app root accessible (and then does a SetParent on itself). Or you could possibly override InvalidateChildren() for nsAppRootAccessible so it always does a CacheChildren() at the end, so the child root accessibles are always cached.
Can you take a look at InvalidateCacheSubtree's use of GetCachedParent()? I wrote it assuming that root accessibles don't have a parent. In fact we have to be careful because that assumption may appear elsewhere in mozilla/accessible
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsDocAccessible.cpp#1090

Nit: 
+        mAccChildCount = -1;
Use eChildCountUninitialized. I realize we have that mistake elsewhere, but lets do it right here.
+        PRUint32 count = 0;
+        mChildren->GetLength(&count);
+        mAccChildCount = NS_STATIC_CAST(PRInt32, count);

Have Nian Liu review it in detail when you have a final patch.

Comment 8

[Evan Yan]

Attachment: patch

modification to the draft patch:
+ fix bug of enumerating array element;
+ fix the use of eChildCountUninitialized in other files also.

nsRootAccessibleWrap has already overrided GetParent() to get nsAppRootAcc as the parent.
And I don't see a problem at InvalidateCacheSubtree's use of GetCachedParent(), nsRootAccessibleWrap::GetParent() will deal with it when GetCachedParent() failed.

Comment 9

[Aaron Leventhal]

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

One thing,
            childWeakRef = do_QueryInterface(childSupports);

	

            accessible = do_QueryReferent(childWeakRef);

I think you can directly do_QueryReferent(childSupports);
which would cut this down by 1 line.

Comment 10

[Aaron Leventhal]

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

Actually, if you check that one last question out, it's good enough.

Comment 11

[[e-mail]]

(In reply to comment #9)
>            childWeakRef = do_QueryInterface(childSupports);
>
>
>
>            accessible = do_QueryReferent(childWeakRef);
>
>I think you can directly do_QueryReferent(childSupports);
Not true, do_QueryReferent takes an nsIWeakReference* parameter.

Comment 12

[Evan Yan]

I removed childSupports in the patch checked in as follows:

+        nsCOMPtr<nsIWeakReference> childWeakRef;
+        nsCOMPtr<nsIAccessible> accessible;
+        nsCOMPtr<nsPIAccessible> previousAccessible;
+        PRBool hasMoreElements;
+        while(NS_SUCCEEDED(enumerator->HasMoreElements(&hasMoreElements))
+              && hasMoreElements) {
+            enumerator->GetNext(getter_AddRefs(childWeakRef));
+            accessible = do_QueryReferent(childWeakRef);

Comment 13

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381