Last Comment Bug 653607 - IAccessible::accChild fails on ARIA documents
: IAccessible::accChild fails on ARIA documents
Status: VERIFIED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla6
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
http://public.yahoo.com/kloots/aria/a...
Depends on:
Blocks: 2011ymaila11y
  Show dependency treegraph
 
Reported: 2011-04-28 16:56 PDT by James Teh [:Jamie]
Modified: 2011-06-01 11:39 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (11.19 KB, patch)
2011-05-19 02:54 PDT, alexander :surkov
dbolter: review+
Details | Diff | Splinter Review
Test file that causes a crash upon closing the tab. (333 bytes, text/html)
2011-05-19 06:03 PDT, Marco Zehe (:MarcoZ)
no flags Details
stack (2.37 KB, text/plain)
2011-05-19 06:22 PDT, David Bolter [:davidb]
no flags Details
patch to land (11.32 KB, patch)
2011-05-19 22:13 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description James Teh [:Jamie] 2011-04-28 16:56:10 PDT
Bug 592913 changed IAccessible::accChild so that it fails if it is called on anything other than a document accessible. In general, this is fine. Unfortunately, this also means that it fails if called on an ARIA document (@role="document"). This causes problems for ARIA documents inside ARIA applications. NVDA renders the document separately in this case, so it expects accChild to work on the ARIA document accessible. (In fact, NVDA can't distinguish between ARIA documents and normal documents.)

It'd be great if IAccessible::accChild could be made to work on ARIA documents just as it does for normal documents; i.e. only return an object if it is a descendant of the document.

Str:
1. Open the URL specified in this bug.
2. Retrieve the ARIA document accessible (div with @role="document"; second child of the top document).
3. Retrieve the unique ID of its first child.
4. Call IAccessible::accChild on the ARIA document accessible (step 2), passing it the unique ID of its first child (step 3).
Expected: The first child of the ARIA document accessible should be returned.
Actual: E_INVALIDARG is returned.

Related NVDA issue ticket: http://www.nvda-project.org/ticket/1452
Comment 1 James Teh [:Jamie] 2011-04-28 16:59:23 PDT
I'm not sure how feasible it is for Mozilla to do this. If it is an insane amount of work, it's probably easier for us to just crawl the hierarchy looking for the document. That isn't particularly elegant or performant (as noted in bug 592913), but I'm keenly aware that there are many larger issues that need addressing at present. :)
Comment 2 James Teh [:Jamie] 2011-05-13 00:47:31 PDT
Marco, what is the likelihood of getting a fix for this on the Mozilla side soon? If it's low, we'll try to just work around it in NVDA.
Comment 3 Marco Zehe (:MarcoZ) 2011-05-13 00:55:33 PDT
Alex, can you take a look at this? Do you have any idea what's going on?
Comment 4 alexander :surkov 2011-05-13 02:20:12 PDT
Jamie, should I care about performance or it could be unperformant solution?
Comment 5 James Teh [:Jamie] 2011-05-15 16:39:00 PDT
I guess that depends how unperformant. :) We could call accChild on the document for every object that gets focus within the document's HWND. However, even if you're crawling up the parents, it's still probably more performant on the Mozilla side than it is for us to do this out-of-process, so I still think it'd be better.
Comment 6 David Bolter [:davidb] 2011-05-17 12:52:23 PDT
(In reply to comment #5)
> I guess that depends how unperformant. :) We could call accChild on the
> document for every object that gets focus within the document's HWND.

Did you measure this? (Or get a feel for it?)
Comment 7 David Bolter [:davidb] 2011-05-17 12:53:16 PDT
Scratch that. We don't really want you relying on HWNDs anyway.
Comment 8 James Teh [:Jamie] 2011-05-17 16:54:12 PDT
(In reply to comment #6)
> > We could call accChild on the
> > document for every object that gets focus within the document's HWND.
Clarification on that statement: I mean that in the worst case, we could do this; we already do this. In other words, any solution needs to take into account that we'll call it at least on every focus change and possibly in more than one document.

> Did you measure this? (Or get a feel for it?)
We already call accChild a lot. However, if we implement our work around, we'll have to crawl the hierarchy more than we do now. It looks as if we are going to have to do this for Yahoo! anyway, so this bug would just make it faster when it is implemented.

(In reply to comment #7)
> Scratch that. We don't really want you relying on HWNDs anyway.
We just use the HWND as a "return early" optimisation. If a focus change happens in a completely different application, there's no point in us asking a Firefox document whether the focused object is a descendant.
Comment 10 David Bolter [:davidb] 2011-05-19 05:54:35 PDT
Comment on attachment 533580 [details] [diff] [review]
patch

r=me. This looks right to me.

>+    // Document.
>+    if (IsDoc())
>+      return AsDoc()->GetAccessibleByUniqueIDInSubtree(uniqueID);
>+
>+    // ARIA document.
>+    if (ARIARole() == nsIAccessibleRole::ROLE_DOCUMENT) {
>+      nsDocAccessible* document = GetDocAccessible();
>+      nsAccessible* child = document->GetAccessibleByUniqueIDInSubtree(uniqueID);
>+      for (nsAccessible* parent = child->GetParent();
>+          parent && parent != document; parent = parent->GetParent()) {
>+        if (parent == this)
>+          return child;
>+      }
>+    }

Optional:
I spent 5 minutes here (before coffee) since I didn't know what the purpose of the loop was. Maybe add a comment that we need to check that the child is a child of this aria document? ( Then I'll spend only 1 min next time :) )
Comment 11 Marco Zehe (:MarcoZ) 2011-05-19 06:03:55 PDT
Created attachment 533621 [details]
Test file that causes a crash upon closing the tab.

1. Open this file with the try-server build and NVDA running.
2. Tab from the textbox to the document.
3. Close the tab with CTRL+W.

Result: Crash.
Comment 12 David Bolter [:davidb] 2011-05-19 06:22:09 PDT
Created attachment 533625 [details]
stack
Comment 13 David Bolter [:davidb] 2011-05-19 06:23:37 PDT
Comment on attachment 533580 [details] [diff] [review]
patch

>+    // ARIA document.
>+    if (ARIARole() == nsIAccessibleRole::ROLE_DOCUMENT) {
>+      nsDocAccessible* document = GetDocAccessible();
>+      nsAccessible* child = document->GetAccessibleByUniqueIDInSubtree(uniqueID);

I think we need to null check child here. r=me can stay if that's fixed.

>+      for (nsAccessible* parent = child->GetParent();
Comment 14 alexander :surkov 2011-05-19 22:13:10 PDT
Created attachment 533889 [details] [diff] [review]
patch to land
Comment 15 alexander :surkov 2011-05-19 22:48:03 PDT
landed - http://hg.mozilla.org/mozilla-central/rev/03dc9705a551
Comment 16 James Teh [:Jamie] 2011-05-23 23:12:27 PDT
Verified fixed in: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110523 Firefox/6.0a1

Note You need to log in before you can comment on or make changes to this bug.