STR: 1. Load any page with NVDA running. 2. Focus any link. 3. Press either APPLICATIONS or Shift+F10. Expected: Context menu should come up and talk. Actual: Context menu comes up, but we don't notify NVDA any more that this has happened. Arrowing up and down does not select menu items, but moves the virtual cursor instead.

This broke some time between the 2013-06-27 and 2013-07-01 builds. Sorry I cannot be more specific than that yet, because the Mozilla FTP server is down and doesn't let me download builds to find the actual two builds between which it broke. But from two builds I have, I see that it worked on the 27th of June, but no longer on the 1st of July.

regression candidate is bug 670087 which was landed 27 June. Jamie, it'd be good to know what is NVDA expectations were broken.

(In reply to Marco Zehe (:MarcoZ) from comment #0) > Actual: Context menu comes up, but we don't notify NVDA any more that this > has happened. Arrowing up and down does not select menu items, but moves the > virtual cursor instead. Note that if you pass the arrow keys through, they do select a menu item and it does fire a focus event. So, it's just the initial appearance of the menu that NVDA isn't notified about.

I'm pretty sure this regression was indeed caused by bug 670087. As required for that bug, popup windows now return the popup as their client (OBJID_CLIENT) instead of the top level frame. What I neglected to realise/note in that bug is that this means events will also hit that accessible, which in turn means IAccessible::accChild will be called on that accessible for all events fired for that HWND. The problem is that IAccessible::accChild on a popup menu accessible is failing for the id of the popup menu accessible itself, even though it does work for the menu items. This is also affecting the location bar autocomplete popup, as well as doorhanger notifications. For the location bar autocomplete, accChild is failing even with ids of its menu items. For doorhangers, it's failing for all of the focusable children (and I suspect all other children as well, but i haven't tested that). In short, on any accessible returned as the root client of an HWND, IAccessible::accChild *must* work correctly for the ids for that accessible and any of its descendants.

Thank you, Jamie.

try sever build will be available in couple here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-7c1198759764

Comment on attachment 773485 [details] [diff] [review] patch Sorry, but this does not fix the bug. The symptoms are basically unchanged for the context menu.

Jamie, it'd be great if you check the try build and say what's still wrong.

(In reply to alexander :surkov from comment #9) > Jamie, it'd be great if you check the try build and say what's still wrong. Unfortunately, as far as I can see, there's no difference. accChild is still failing for the same ids as before.

Actually, there is a slight improvement. Doorhangers now work as expected. Menus and location bar autocomplete are still broken as before.

Jamie, here will be another build in couple hours: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-036f17eda8ad. Context menus seems working but addressbar doesn't. I don't see that accChild triggers for addressbar popup. Can you take a look where the problem is please?

(In reply to alexander :surkov from comment #12) > Context menus seems working Confirmed. > but addressbar > doesn't. I don't see that accChild triggers for addressbar popup. Can you > take a look where the problem is please? Further investigation reveals that WM_GETOBJECT with OBJID_CLIENT on the autocomplete HWND (class: MozillaDropShadowWindowClass) returns an oleacc client; i.e. Firefox isn't answering WM_GETOBJECT for this window at all. In order for accHitTest to work, I guess it should return the autocomplete list (the parent of the autocomplete items). Note that calling accChild on the autocomplete list with an id of one of its children fails currently, so this would need to be fixed as well.

try build will be here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-ad8e85cb9406

(In reply to alexander :surkov from comment #15) > try build will be here: > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov. > alexander@gmail.com-ad8e85cb9406 Menus, context menus, doorhangers and the location bar autocomplete are all working with this build. Thanks!

(In reply to James Teh [:Jamie] from comment #16) > (In reply to alexander :surkov from comment #15) > > try build will be here: > > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov. > > alexander@gmail.com-ad8e85cb9406 > Menus, context menus, doorhangers and the location bar autocomplete are all > working with this build. Thanks! thank you, Jamie, you can steal f? request from Marco changing it f+ ;)

Comment on attachment 776521 [details] [diff] [review] patch2 f=me. I can confirm Jamie's findings.

Comment on attachment 776521 [details] [diff] [review] patch2 I'm not sure why you need to make these changes off hand can you explain in the commit message or somewhere? >+Accessible* >+DocAccessible::GetAccessibleOrDescendant(nsINode* aNode) const >+{ >+ Accessible* acc = GetAccessible(aNode); >+ if (acc) >+ return acc; >+ >+ acc = GetContainerAccessible(aNode); >+ if (acc) { >+ for (uint32_t idx = 0; idx < acc->ChildCount(); idx++) { pull ChildCount() out of the loop >+ Accessible* child = acc->GetChildAt(idx); >+ for (nsIContent* elm = child->GetContent(); elm; >+ elm = elm->GetFlattenedTreeParent()) { >+ if (elm == aNode) >+ return child; no need to keep looking at nodes above the node for the container accessible right? >+++ b/accessible/src/windows/msaa/AccessibleWrap.cpp >@@ -32,16 +32,17 @@ > #include "nsINodeInfo.h" > #include "nsIServiceManager.h" > #include "nsTextFormatter.h" > #include "nsView.h" > #include "nsViewManager.h" > #include "nsEventMap.h" > #include "nsArrayUtils.h" > #include "mozilla/Preferences.h" >+#include "nsLayoutUtils.h" why do you need it? >+ Accessible* parent = child; >+ while (parent && parent != document) { >+ if (parent == this) >+ return child; maybe we should just add a function to tell if one accessible is the child of another? >+#ifdef DEBUG >+#include "mozilla/a11y/Logging.h" >+#endif we should really just get around to making configure deal with A11Y_LOG

(In reply to Trevor Saunders (:tbsaunde) from comment #19) > Comment on attachment 776521 [details] [diff] [review] > patch2 > > I'm not sure why you need to make these changes off hand can you explain in > the commit message or somewhere? popup elements sometimes aren't accessible itself but their children are, for example, in case of addressbar's autocomplete > >+ for (uint32_t idx = 0; idx < acc->ChildCount(); idx++) { > > pull ChildCount() out of the loop > > >+ Accessible* child = acc->GetChildAt(idx); > >+ for (nsIContent* elm = child->GetContent(); elm; > >+ elm = elm->GetFlattenedTreeParent()) { > >+ if (elm == aNode) > >+ return child; > > no need to keep looking at nodes above the node for the container accessible > right? agree > >+ Accessible* parent = child; > >+ while (parent && parent != document) { > >+ if (parent == this) > >+ return child; > > maybe we should just add a function to tell if one accessible is the child > of another? do we have other use cases? > > >+#ifdef DEBUG > >+#include "mozilla/a11y/Logging.h" > >+#endif > > we should really just get around to making configure deal with A11Y_LOG file a bug?

(In reply to alexander :surkov from comment #20) > (In reply to Trevor Saunders (:tbsaunde) from comment #19) > > Comment on attachment 776521 [details] [diff] [review] > > patch2 > > > > I'm not sure why you need to make these changes off hand can you explain in > > the commit message or somewhere? > > popup elements sometimes aren't accessible itself but their children are, > for example, in case of addressbar's autocomplete ok > > >+ Accessible* parent = child; > > >+ while (parent && parent != document) { > > >+ if (parent == this) > > >+ return child; > > > > maybe we should just add a function to tell if one accessible is the child > > of another? > > do we have other use cases? I'm pretty sure I've een it other places.

(In reply to Trevor Saunders (:tbsaunde) from comment #21) > > > >+ Accessible* parent = child; > > > >+ while (parent && parent != document) { > > > >+ if (parent == this) > > > >+ return child; > > > > > > maybe we should just add a function to tell if one accessible is the child > > > of another? > > > > do we have other use cases? > > I'm pretty sure I've een it other places. anyway it seems a small bit of code that maybe easier to recreate than remember we have a function somewhere.

Trev, I realized that I landed it having no r+ from you. Could you please finish review asap so I can land a follow up to fix your findings rather than backing out it? https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b9e91729d3

Comment on attachment 777757 [details] [diff] [review] patch > Accessible* >-DocAccessible::GetAccessibleOrContainer(nsINode* aNode) >+DocAccessible::GetAccessibleOrContainer(nsINode* aNode) const btw hm, maybe we should add over loads for nsIDocument* and Element or nsIContent* since atleast in the doc case we know the doc has an accessible but nobody will ever want its parent. >+Accessible* >+DocAccessible::GetAccessibleOrDescendant(nsINode* aNode) const >+{ >+ Accessible* acc = GetAccessible(aNode); >+ if (acc) >+ return acc; >+ >+ acc = GetContainerAccessible(aNode); >+ if (acc) { >+ uint32_t childCnt = acc->ChildCount(); >+ for (uint32_t idx = 0; idx < childCnt; idx++) { >+ Accessible* child = acc->GetChildAt(idx); >+ for (nsIContent* elm = child->GetContent(); >+ elm && elm != acc->GetContent(); when can it be null? if acc is document and child is not under body? >- Accessible* GetContainerAccessible(nsINode* aNode) >+ Accessible* GetContainerAccessible(nsINode* aNode) const btw can we change it to take nsIContent* or Element*?

(In reply to Trevor Saunders (:tbsaunde) from comment #25) > >-DocAccessible::GetAccessibleOrContainer(nsINode* aNode) > >+DocAccessible::GetAccessibleOrContainer(nsINode* aNode) const > > btw hm, maybe we should add over loads for nsIDocument* and Element or > nsIContent* since atleast in the doc case we know the doc has an accessible > but nobody will ever want its parent. right. single Element* should work > >+ Accessible* acc = GetAccessible(aNode); > >+ if (acc) > >+ return acc; > >+ > >+ acc = GetContainerAccessible(aNode); > >+ if (acc) { > >+ uint32_t childCnt = acc->ChildCount(); > >+ for (uint32_t idx = 0; idx < childCnt; idx++) { > >+ Accessible* child = acc->GetChildAt(idx); > >+ for (nsIContent* elm = child->GetContent(); > >+ elm && elm != acc->GetContent(); > > when can it be null? if acc is document and child is not under body? right, some crazy cases > >- Accessible* GetContainerAccessible(nsINode* aNode) > >+ Accessible* GetContainerAccessible(nsINode* aNode) const > > btw can we change it to take nsIContent* or Element*? I thought that will be nice but it required some work so one day it'd be good to do.