Closed Bug 350063 Opened 16 years ago Closed 16 years ago

Window index returned -1 always on calling Accessible_getIndexInParent


(Core :: Disability Access APIs, defect)

Not set





(Reporter: nagappan, Assigned: evan.yan)



(Keywords: access)


(1 file, 1 obsolete file)

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)
FF version: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060824 Minefield/3.0a1
On calling Accessible_getIndexInParent (frame)

What's frame? Any node?
Blocks: newatk
Severity: minor → normal
By frame here we're talking about SPI_ROLE_FRAME.
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.
Assignee: aaronleventhal → Evan.Yan
rootAcc has been added to the appRootAcc's children, but nsAppRootAccessible::GetFirstChild() returns null.

still debugging...
Attached patch patch draft (obsolete) — Splinter Review
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.
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

+        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.
Attached patch patchSplinter Review
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.
Attachment #237120 - Attachment is obsolete: true
Attachment #237593 - Flags: review?(nian.liu)
Attachment #237593 - Flags: review?(nian.liu) → review+
Comment on attachment 237593 [details] [diff] [review]

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.
Attachment #237593 - Flags: superreview?(neil)
Comment on attachment 237593 [details] [diff] [review]

Actually, if you check that one last question out, it's good enough.
Attachment #237593 - Flags: superreview?(neil)
Closed: 16 years ago
Resolution: --- → FIXED
(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.
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);
You need to log in before you can comment on or make changes to this bug.