592913 - Provide a way to quickly determine whether an accessible object is a descendant of a tab document

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.10pre) Gecko/20100829 Namoroka/3.6.10pre
Build Identifier: 

Prior to bug 130078, accessibility clients such as screen readers could use the HWND to efficiently determine whether an accessible object was part of a given document. That is, if the HWND of a given object was not equal to or a descendant of the document HWND, the client could safely conclude that the object was not part of the document. Bug 130078 removed separate HWNDs for documents such that the entire browser now exists within one HWND, thus breaking this method. As far as I am aware, the only method now to determine whether one object is a descendant of another is to crawl up the ancestors of the former object and compare against the latter, which is extremely inefficient especially out-of-process.

One possible solution is to change IAccessible::accChild() so that it fails if it is called for an ID which does not correspond to a descendant object. (Currently, IAccessible::accChild() returns an object as long as the ID is valid, even if it is not a descendant.)

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

If all you need to know if this accessible belongs to that document then perhaps it's worth to add document property to IAccessible2. For example,

[propget] HRESULT document([out, retval] IUnknown **document) what returns IAccessibleDocument instance.

It'll be very quick.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #0)
> One possible solution is to change IAccessible::accChild() so that it fails if
> it is called for an ID which does not correspond to a descendant object.
> (Currently, IAccessible::accChild() returns an object as long as the ID is
> valid, even if it is not a descendant.)

It won't be quick until we do something special. But of course it's much quicker than you could do. Though if you talk about IAccessible::accChild() changes on document accessible only then it can be done. The question would be whether should it working the same way for nested documents.

Comment 3

[James Teh [:Jamie]]

(In reply to comment #1)
> If all you need to know if this accessible belongs to that document then
> perhaps it's worth to add document property to IAccessible2. For example,
> [propget] HRESULT document([out, retval] IUnknown **document) what returns
> IAccessibleDocument instance.
I feel this is a very limited approach which only solves one particular instance of a larger question. If we're going to implement a new method, I think it'd be better to have an isDescendant() method. Gecko can optimise by first checking whether the document for the two objects is the same.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (In reply to comment #1)
> > If all you need to know if this accessible belongs to that document then
> > perhaps it's worth to add document property to IAccessible2. For example,
> > [propget] HRESULT document([out, retval] IUnknown **document) what returns
> > IAccessibleDocument instance.
> I feel this is a very limited approach which only solves one particular
> instance of a larger question.

Could you give an idea of all problems this bug should solve?

> If we're going to implement a new method, I
> think it'd be better to have an isDescendant() method. Gecko can optimise by
> first checking whether the document for the two objects is the same.

The problem of IsDescedant() approach is it's not performant by default and especially if it's expected to be used often then we should come with something else.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

I have the exact same comments/concerns/questions as Alex. However, I think we could solve this problem performantly but we need to know how it is biting first.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Alex, I'm thinking of a data member that essentially stores a unique path, and has properties that make it trivial to make comparisons that describe the ancestor/descendant relationship. The way I have in mind, to set these members, relies on the work you did to always construct accessibles in parent to child order.

Comment 7

[James Teh [:Jamie]]

(In reply to comment #4)
> > I feel [that an IAccessible2::document property] is a very limited approach which only solves one particular
> > instance of a larger question.
> Could you give an idea of all problems this bug should solve?
Here, I'm more talking about the method added to IAccessible2. If we're going to add methods to IAccessible2, they shouldn't be specific to one use case, but rather,  they should cover as many as possible. isDescendant() is a more general case.

> The problem of IsDescedant() approach is it's not performant by default and
> especially if it's expected to be used often then we should come with something
> else.
It's not performant if it's used in cases where it can't be optimised, but in this case, I'm only talking about using it to test whether an object is a descendant of a document. In that case, you can use exactly the same code that you would have used to implement the document property. In pseudo-code:
bool isDescendant(IAccessible2* obj) {
  if (obj->document == this)
    return true;
  // Do normal slow isDescendant check here.
}

Comment 8

[James Teh [:Jamie]]

Maybe it could even throw an error if it is used in an unoptimised case. In other words, it's only a helper method; if it isn't implemented/supported, you should do it the slow way.

Comment 9

[alexander :surkov (:asurkov)]

Ok, I see your point. If we follow isDescedant way then it should be something more general I think like compare(aObj1, aObj2) what returns tri-state value pointing to parent-child relation of these objects.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Alex, I'm thinking of a data member that essentially stores a unique path, and
> has properties that make it trivial to make comparisons that describe the
> ancestor/descendant relationship. The way I have in mind, to set these members,
> relies on the work you did to always construct accessibles in parent to child
> order.

Yes, if we follow Jamie's way then we should do something like this. But we need to cache things, keep them updated just because we introduced new method, if somebody wants to use it then it shouldn't hang the firefox for a while.

Comment 11

[James Teh [:Jamie]]

(In reply to comment #10)
> > Alex, I'm thinking of a data member that essentially stores a unique path, and
> > has properties that make it trivial to make comparisons that describe the
> > ancestor/descendant relationship.
> Yes, if we follow Jamie's way then we should do something like this.
Why? At least for now, the only case that needs to be optimised is the descendant of document case, which can just be done by comparing the documents for two given objects. The only reason I'm proposing isDescendant is that I think it is a more general case for addition to IA2.

Note that we may have a problem here with timing. If we don't find another solution to bug 591874, we'll need to get this into IA2 before FF4beta6. I'm not sure if this is really possible. Therefore, perhaps the accChild solution is better for now.

(In reply to comment #2)
> Though if you talk about IAccessible::accChild()
> changes on document accessible only then it can be done. The question would be
> whether should it working the same way for nested documents.
accChild should return objects only if the node is somewhere beneath the content document, including nodes in nested documents.

Comment 12

[alexander :surkov (:asurkov)]

bool isDescedant (or int compare(obj1, obj2) as I suggested) is util method and perhaps it's not correct to put it between object properties. And if I get right we still only one use case for this, get the object document. If we keep in mind this usecase and nothing else then I think I would prefer to
1) add document property on accessible object, this property is similar to DOM and it's ok that is not universal, it has usecase and therefore it's useful as is.
2) make IServiceProvider::QueryService() to return IAccessibleDocument document for accessible object (as it was suggested to us by AT vendor).

Jamie, side question, are you interesting in closest document? For example,

document for firefox
  document for tab
    document for iframe
      button

If you check what document the button has then what document would you expect? That concerns to isDescedant and accChild as well, should docuemntForTab.isDescedant(button) return false?

Comment 13

[James Teh [:Jamie]]

(In reply to comment #12)
> bool isDescedant (or int compare(obj1, obj2) as I suggested) is util method and
> perhaps it's not correct to put it between object properties.
I don't understand what you mean by "put it between object properties". Can you explain further?

> 1) add document property on accessible object, this property is similar to DOM
> and it's ok that is not universal, it has usecase and therefore it's useful as
> is.
I think adding methods to support very limited and specific use cases is a bad idea. If we keep doing this, the interface is going to get huge and all servers have to provide stubs for a whole load of methods they won't ever use. Also, isDescendant could well be useful elsewhere. Even unoptimised, it is still better for out-of-process clients than walking the tree.

> 2) make IServiceProvider::QueryService() to return IAccessibleDocument document
> for accessible object (as it was suggested to us by AT vendor).
I assume you'll need to create a new IID for this fake service?

> Jamie, side question, are you interesting in closest document? For example,
> document for firefox
>   document for tab
>     document for iframe
>       button
> If you check what document the button has then what document would you expect?
This is one problem with the document property. For my purposes, I want the document for tab, but logic would suggest that it should be the document for iframe.

> That concerns to isDescedant and accChild as well, should
> docuemntForTab.isDescedant(button) return false?
Imo, it should return true.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (In reply to comment #12)
> > bool isDescedant (or int compare(obj1, obj2) as I suggested) is util method and
> > perhaps it's not correct to put it between object properties.
> I don't understand what you mean by "put it between object properties". Can you
> explain further?

I meant IA2 interfaces are bunch of properties of accessible objects. IsDescedant is rather util method, not a property of the object. Perhaps this is ok but unusual.

> > 1) add document property on accessible object, this property is similar to DOM
> > and it's ok that is not universal, it has usecase and therefore it's useful as
> > is.
> I think adding methods to support very limited and specific use cases is a bad
> idea. If we keep doing this, the interface is going to get huge and all servers
> have to provide stubs for a whole load of methods they won't ever use. Also,
> isDescendant could well be useful elsewhere. Even unoptimised, it is still
> better for out-of-process clients than walking the tree.

I don't think accessible document property should fall into category of methods of limited use cases, since it should be universal and allow to traverse the tree more easily. But you're an AT client developer, so I should trust your judgement :)

Ok, fair enough. As I said above I see your point but I'm not sure I totally share it. At least I don't see anything real bad with isDescedant approach.

> > 2) make IServiceProvider::QueryService() to return IAccessibleDocument document
> > for accessible object (as it was suggested to us by AT vendor).
> I assume you'll need to create a new IID for this fake service?

The suggestion is the following. If you pass two uids of IAccessibleDocument then we return direct document accessible for the accessible object. If you pass uid of IAccessibleDocument and new uid for "root document" then we return document for a tab content.

Btw, new uid for this is more doable in terms of IA2 spec changes because it requires to add it to IAccessibleDocument idl file I think, and doesn't require to change any IA2 interface. In the case of IAccessible2 changes we have lot of pending discussions to change this interface by some way, so it's not good to add just one new isDescedant method.

> > Jamie, side question, are you interesting in closest document? For example,
> > document for firefox
> >   document for tab
> >     document for iframe
> >       button
> > If you check what document the button has then what document would you expect?
> This is one problem with the document property. For my purposes, I want the
> document for tab, but logic would suggest that it should be the document for
> iframe.

Ok, it sounds it's the same for other parties.

> > That concerns to isDescedant and accChild as well, should
> > docuemntForTab.isDescedant(button) return false?
> Imo, it should return true.

I think we could change accChild implementation on document accessible, at least it shouldn't broke AT if it returns true for any descendant element in hierarchy.

Comment 15

[James Teh [:Jamie]]

(In reply to comment #14)
> I meant IA2 interfaces are bunch of properties of accessible objects.
> IsDescedant is rather util method, not a property of the object. Perhaps this
> is ok but unusual.
It'd certainly need to be a method, not a property method.

> I don't think accessible document property should fall into category of methods
> of limited use cases, since it should be universal and allow to traverse the
> tree more easily.
I guess i question why documents are a specific case. However, it's certainly true that they do seem to be a specific case for all current ATs, so perhaps this is okay. I still think the more general method is better though.

> The suggestion is the following. If you pass two uids of IAccessibleDocument
> then we return direct document accessible for the accessible object.
There's no IAccessibleDocument interface at present, so I guess you're proposing a new interface?
> If you
> pass uid of IAccessibleDocument and new uid for "root document" then we return
> document for a tab content.
The second parameter to QueryService is supposed to be a valid interface of the object, so it needs to be IAccessible2, IUnknown, etc.

> Btw, new uid for this is more doable in terms of IA2 spec changes because it
> requires to add it to IAccessibleDocument idl file I think, and doesn't require
> to change any IA2 interface. In the case of IAccessible2 changes we have lot of
> pending discussions to change this interface by some way, so it's not good to
> add just one new isDescedant method.
I agree. I still don't follow how you're going to handle the QueryService idea though; see above.

> I think we could change accChild implementation on document accessible, at
> least it shouldn't broke AT if it returns true for any descendant element in
> hierarchy.
That's certainly what happened before bug 130078.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)

> > The suggestion is the following. If you pass two uids of IAccessibleDocument
> > then we return direct document accessible for the accessible object.
> There's no IAccessibleDocument interface at present, so I guess you're
> proposing a new interface?

You're right. I get usual to think Gecko API matches IA2.

> > If you
> > pass uid of IAccessibleDocument and new uid for "root document" then we return
> > document for a tab content.
> The second parameter to QueryService is supposed to be a valid interface of the
> object, so it needs to be IAccessible2, IUnknown, etc.

Ok, can we introduce new service guids for direct document and for tab document?
And use IAccessible as second argument?

> > I think we could change accChild implementation on document accessible, at
> > least it shouldn't broke AT if it returns true for any descendant element in
> > hierarchy.
> That's certainly what happened before bug 130078.

I meant if we hadn't bug 130078 or if we'll get old windows hierarchy emulation.

Comment 17

[James Teh [:Jamie]]

(In reply to comment #16)
> Ok, can we introduce new service guids for direct document and for tab
> document?
> And use IAccessible as second argument?
Yes. This is fairly hacky, but it'll work.

> > > I think we could change accChild implementation on document accessible, at
> > > least it shouldn't broke AT if it returns true for any descendant element in
> > > hierarchy.
> > That's certainly what happened before bug 130078.
> I meant if we hadn't bug 130078 or if we'll get old windows hierarchy
> emulation.
Can this not be done without these? That is, restore the behaviour of accChild for documents so that it as it was before bug 130078, even without the HWND hierarchy emulation.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)

> Can this not be done without these? That is, restore the behaviour of accChild
> for documents so that it as it was before bug 130078, even without the HWND
> hierarchy emulation.

Right, I see. This can be done I believe.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (In reply to comment #17)
> 
> > Can this not be done without these? That is, restore the behaviour of accChild
> > for documents so that it as it was before bug 130078, even without the HWND
> > hierarchy emulation.
> 
> Right, I see. This can be done I believe.

Jamie, if accChild fix is all you need to get NVDA working then I'll fix this.

Comment 20

[James Teh [:Jamie]]

(In reply to comment #19)
> > > restore the behaviour of accChild
> > > for documents so that it as it was before bug 130078, even without the HWND
> > > hierarchy emulation.
> Jamie, if accChild fix is all you need to get NVDA working then I'll fix this.
Yes, we can get things working on the NVDA side with this fix. If you can do it without introducing a perf hit on your side, then yes, this would be great.

Comment 21

[alexander :surkov (:asurkov)]

It shouldn't be slower than it is. Can we consider this bug fixed if we follow accChild approach?

Comment 22

[alexander :surkov (:asurkov)]

Jamie, how are you going to use accChild to know if this object belongs to tab document accessible? If I get right that's a root problem when you should decide whether you should turn on virtual buffer.

Comment 23

[James Teh [:Jamie]]

(In reply to comment #21)
> Can we consider this bug fixed if we follow
> accChild approach?
Yes. I filed this bug thinking about a more general approach, but really, all we need is the more specific fix. In fact, please change the summary to "Provide a way to quickly determine whether an accessible object is a descendant of a tab document". (I don't have permission to change the summary.)

(In reply to comment #22)
> Jamie, how are you going to use accChild to know if this object belongs to tab
> document accessible? If I get right that's a root problem when you should
> decide whether you should turn on virtual buffer.
We detect when to use a virtual buffer by scanning the focus ancestors, which we fetch whenever the focus changes. We don't need accChild to make this determination. We just need accChild once we already have a buffer and want to know whether an object is beneath the document associated with that buffer.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> (In reply to comment #21)
> > Can we consider this bug fixed if we follow
> > accChild approach?
> Yes. I filed this bug thinking about a more general approach, but really, all
> we need is the more specific fix. In fact, please change the summary to
> "Provide a way to quickly determine whether an accessible object is a
> descendant of a tab document". 

perhaps more detailed summary will fit better like "make IAccessible::accChild on document accessible return an accessible iif it's a descendant"?

> (I don't have permission to change the summary.)

as I said early you should apply for extended permissions

> (In reply to comment #22)
> > Jamie, how are you going to use accChild to know if this object belongs to tab
> > document accessible? If I get right that's a root problem when you should
> > decide whether you should turn on virtual buffer.
> We detect when to use a virtual buffer by scanning the focus ancestors, which
> we fetch whenever the focus changes. We don't need accChild to make this
> determination. We just need accChild once we already have a buffer and want to
> know whether an object is beneath the document associated with that buffer.

Ok, but how would you know this document is the document that virtual buffer should be attached to?

Comment 25

[alexander :surkov (:asurkov)]

request blocking status based on comment #20

Comment 26

[Marco Zehe (:MarcoZ)]

Changing summary as requested by Jamie, for now.

Comment 27

[James Teh [:Jamie]]

(In reply to comment #24)
> > In fact, please change the summary to
> > "Provide a way to quickly determine whether an accessible object is a
> > descendant of a tab document". 
> perhaps more detailed summary will fit better like "make IAccessible::accChild
> on document accessible return an accessible iif it's a descendant"?
That's fine. However, I'd take any solution which solves this problem without excessive work for any party, hence the solution-independent summary.

> > We detect when to use a virtual buffer by scanning the focus ancestors, which
> > we fetch whenever the focus changes.
> Ok, but how would you know this document is the document that virtual buffer
> should be attached to?
Two clarifications on my description above. First, we use the focus ancestors to determine when to create a new buffer. Second, we scan the ancestors from top down, so the first document to match is the one we create a buffer for. Does this answer the question?

Comment 28

[alexander :surkov (:asurkov)]

Ok, fair enough. Thank you.

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 30

[Marco Zehe (:MarcoZ)]

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

A few comments I found while looking over the patch:

>+   * Return the child document accesisble at the given index.

Typo in the word "accessible".

>+  PRUint32 i, length = tmp->mChildDocuments.Length();
>+  for (i = 0; i < length; ++i) {

Pre-increment operator? Won't that leave out the 0th element?

>+   * Return parent document that this document belongs contained to.

"belongs to" or "is contained in".

>+    // tabDoc and testDoc are differnent documents depending on wtherer test

"different" and "whether"

The tests look great. I'll now give this one a spin in a local build with the build Jamie sent me a while back.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)

> >+  PRUint32 i, length = tmp->mChildDocuments.Length();
> >+  for (i = 0; i < length; ++i) {
> 
> Pre-increment operator? Won't that leave out the 0th element?

Just copy/paste. No, increment operation is called after the cycle. So there's no difference between pre or post operator in this case.

 > The tests look great. I'll now give this one a spin in a local build with the
> build Jamie sent me a while back.

Thank you!

Comment 32

[alexander :surkov (:asurkov)]

I think we should detect presence of certain screen reader of certain versions and emulate window hierarchy it expects. New versions of screen readers and AT we don't work with will deal with with existing windows hierarchy. It allows us to remove this deprecated approach in future more easy and makes us sure new AT won't depend on this deprecated way. Since we work with most of screen reader vendors then it shouldn't affect on AT users. Does this sound reasonable?

Btw, I've got working NVDA. So this approach works. Based on information from other AT vendors I think it this approach works for them as well. I need to get your opinions for the question above and I can polish the patch.

Comment 33

[James Teh [:Jamie]]

(In reply to comment #32)
> I think we should detect presence of certain screen reader of certain versions
> and emulate window hierarchy it expects.
Was this comment more meant for bug 591874?
It's actually impossible to detect odl versions of NVDA, since we don't do any dll versioning for our in-proc dlls. However, I think we can get the "right fix" into our upcoming release, so this isn't a big problem.

Comment 34

[Marco Zehe (:MarcoZ)]

This patch has a couple of problems with the version of NVDA Jamie sent me over on bug 591874.
First, it does fix the problem of the awesome bar and others causing NVDA to go into focus mode unexpectedly.

However, several pages come up blank: http://www.facebook.com, http://www.amazon.de are two examples. The virtual buffer is blank, and every press of "tab" yields NVDA to speak "unknown". Other pages like YouTube seem to work OK on first glance.

Comment 35

[James Teh [:Jamie]]

(In reply to comment #34)
> This patch has a couple of problems with the version of NVDA Jamie sent me over
> on bug 591874.
> several pages come up blank: http://www.facebook.com,
> http://www.amazon.de are two examples. The virtual buffer is blank, and every
> press of "tab" yields NVDA to speak "unknown".
I suspect this would be the same even without this patch. It sounds similar to the strange issue I mentioned in bug 591874. I'll file a bug on that soon.

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

Approving blocking. This bug is NVDA's preferred solution to the fallout from bug 130078.

Comment 37

[alexander :surkov (:asurkov)]

Ok, if we have likely another problem but we definitely want to take this accChild changes then it's worth to pass this patch through review process and land it.

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

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

>-  if (!nsAccUtils::MustPrune(this)) {

Why can you remove this? Is it because we already prune internally? Why was this here?

Comment 39

[alexander :surkov (:asurkov)]

(In reply to comment #38)
> Comment on attachment 473430 [details] [diff] [review]
> patch
> 
> >-  if (!nsAccUtils::MustPrune(this)) {
> 
> Why can you remove this? Is it because we already prune internally? Why was
> this here?

becuase it's inside of GetXPAccessibleFor

Comment 40

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Ah I see the MustPrune in GetXPAccessibleFor.

>   /**
>+   * Return parent document that this document belongs contained to.

I would just say "Return the parent document"

>   /**
>+   * Append the child document accessible to this document accessible.

I would say "Append the given document accessible to this document's child document accessibles"

and
"Remove the given document accessible from this document's child document accessibles."

> STDMETHODIMP nsAccessibleWrap::get_accChild(
>       /* [in] */ VARIANT varChild,
>       /* [retval][out] */ IDispatch __RPC_FAR *__RPC_FAR *ppdispChild)
> {
> __try {
>   *ppdispChild = NULL;
>-  if (!mWeakShell || varChild.vt != VT_I4)
>+  if (IsDefunct())
>     return E_FAIL;

According to spec we should also:
if (varChild.vt == VT_EMPTY) return E_INVALIDARG;

Comment 41

[David Bolter [:davidb] (NeedInfo me for attention)]

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

r=me if Marco gives the thumps up.

Comment 42

[alexander :surkov (:asurkov)]

(In reply to comment #40)
 
> According to spec we should also:
> if (varChild.vt == VT_EMPTY) return E_INVALIDARG;

It does, GetXPAccessibleFor() returns nsnull in this case and get_accChild will end up with E_INVALIDARG.

Comment 43

[Marco Zehe (:MarcoZ)]

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

OK, r=me for this patch since it makes things work in principal with NVDA again. let's deal with the problems I saw on facebook and amazon.de separately.

Comment 44

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #42)
> (In reply to comment #40)
> 
> > According to spec we should also:
> > if (varChild.vt == VT_EMPTY) return E_INVALIDARG;
> 
> It does, GetXPAccessibleFor() returns nsnull in this case and get_accChild will
> end up with E_INVALIDARG.

OK great, thanks for the explanation.

Comment 45

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/b76cfd9e1028