Steps to reproduce: 1. Launch Firefox 2. Launch Accerciser 3. Use Accerciser to examine the roles of the menus in the Application menu bar (i.e. File, Edit, View, History, etc., etc.) Expected results: Those items would have the role of menu and the items that contain would appear in the hierarchy beneath them as children. Actual results: Those items sometimes have the role of menu item and claim no children. I cannot reproduce this 100% of the time and haven't figured out the magical trigger. However, it seems to keep cropping up in the automated tests we're doing for Orca, so I'm hoping someone else can reproduce it. :-) Thanks!

Is this a regression?

Aaron I think this has been around at least a week. Joanmarie, thanks so much for reporting this! I thought it was just me. Note this is possibly related to bug 396941, or perhaps recent work that caused both?

Aaron, maybe we should reassign to a linux a11y person unless you think it is more core. (I'm bumping the severity since it implies some serious fragility in the code)

I've confirmed this bug with both Accerciser and GOK on today's FF3 nightly.

Ginn, sorry to do this to you, but you're the best person for this one. Seven us agreed! :)

Taking, but I've trouble to reproduce this with my debug build.

Well, I got it reproduced. (It's really hard!) The trick is try to slow down Firefox startup, and get Accerciser or GOK to query menu's atkobject during its startup. (I still could not get 100% reproducible.) We give ROLE_MENUITEM or ROLE_PARENTMENUITEM (mapping to atk role menu) depends on the its child count. That's the problem. http://lxr.mozilla.org/seamonkey/source/accessible/src/xul/nsXULMenuAccessible.cpp#480 And the role is cached in atkobject. Also the childcount is cached in nsAccessible object. I think it's not only a Linux issue.

Thanks Ginn! Does the object change its role from ROLE_MENUITEM to ROLE_PARENTMENUITEM when it detects children have been added to it? If so, does it issue a object:property-change:accessible-role event? If so, we probably could handle it in Orca.

Yes excellent work Ginn. Do you have a fix in mind? (As for other Linux issues please file!)

No, we don't change its role, and we don't change its childcount. (maybe sometimes we change its childcount) We need to reset the childcount and role at some point. I'm still trying to figure out how to fix it.

We do nsXULMenupopupAccessible::GenerateMenu(mDOMNode); in nsXULMenuitemAccessible::Init() and wish children menuitem get generated immediately. But they don't, AddLazyChildren() works in async way.

That probably changed recently with the menu rearchitecture.

Neil, is there a method to generate the menu immediately? If not, I think nsXULMenupopupAccessible::GenerateMenu is useless. Another option for us is, we can determine ROLE_MENUITEM or ROLE_PARENTMENUITEM by tag "menuitem" or "menu". And we should reset the childcount on AddLazyChildren() callback.

> We give ROLE_MENUITEM or ROLE_PARENTMENUITEM (mapping to atk role menu) > depends on the its child count. What is ROLE_PARENTMENUITEM? A menu? If so, using the child count to distinguish menus from menuitems is a bizarre and incorrect way to detect that. You should be using the tag name.

Neil, I agree. But I still wish there's a method to generate menuitems under menu synchronously. Probably by using this method we can also get bug 332663 fixed.

Ginn, I suggest that you just fix this more critical bug the easy way. We might no get to bug 332663 in time.

Aaron, probably it's worth to check whether it implements nsIDOMXULContainerElement interface? I don't really like to use often tagnames in code.

Looks good, but we still need to reset childcount when menu really generated.

Probably, we can just do InvalidateChildren in LazyGeneratePopupDone(). I've a WIP patch now. I'll post it tomorrow.

Comment on attachment 286701 [details] [diff] [review] Use Surkov's suggestion -- QI -- avoids dependency on tag and is smaller code Waiting for Ginn's new patch.

I couldn't just do accService->InvalidateSubtreeFor(), because it will re-create the parent menuitem accessible. Ideally, I should emit some events to notify AT client. But 1) I don't know which event is expected in this case. Perhaps fire children-changed:remove and then children-changed:add for the parent menuitem. 2) There's no way to fire AccEvent directly inside layout module. If we are going to remove this patch eventually, I think we don't need go that far.

What is this a child count of, and why is it being cached?

It's the child count of parent menuitem accessible. Accessible objects have a cache for their child accessible and chid count. Quote from http://www.mozilla.org/access/architecture The algorithm used to calculate the number of accessible children for an accessible node is expensive. We cannot assume that all of the accessible children will come from the direct children, grandchildren or even great-great-great-children of the current accessible's node. Therefore we have to iterate through the tree as if we were creating all of the accessible children, adding to the total as we go. To make this less expensive, once the child count or any child of an accessible is asked for, both the child count and the children are calculated at the same time and then cached, so that we can avoid doing these expensive operations more than once.

BTW: My patch should be used together with Aaron's. I'd like to hear more comments before asking for review.

I think, for Firefox 3, probably we should just do a workaround inside accessible module for this bug. We can just override CacheChildren for nsXULMenuitemAccessible. (or maybe add a nsXULParentMenuitemAccessible class). In there, we treat mAccChildCount == 0 as a loading/busy state. We will always try to get children again when mAccChildCount == 0. Since we do nsXULMenupopupAccessible::GenerateMenu(mDOMNode); in nsXULMenuitemAccessible::Init(), it won't cause performance issue. mAccChildCount == 0 should be a quite short period, except: the menu really doesn't have any child menuitem. Since we've no access to nsMenuPopupFrame::HasGeneratedChildren in accessible module, we'll never know if it is still being loaded or it has no child. But generally a valid parent menuitem always has children. So I think it's a reasonable workaround.

Comment on attachment 287531 [details] [diff] [review] workaround it in accessible module Please just make sure it doesn't break MSAA, which has a slightly different menu bar structure. In MSAA there is an additional accessible object for the submenu itself. In ATK those are considered redundant with the parent menu item and are not exposed.

Comment on attachment 287531 [details] [diff] [review] workaround it in accessible module it does have problems with msaa code, it doesn't compile and it couldn't fix this bug on Windows, because there's an extra menupopup accessible. I'll work out another patch.

Comment on attachment 286956 [details] [diff] [review] draft patch to reset childcount it doesn't fix the problem on Windows

Aaron, Since menuitems have different parent with MSAA and ATK, we need override CacheChildren for both nsXULMenuPopupAccessible, and nsXULParentMenuitemAccessible. Do you think it's a good idea? Also, I've a concern about the BUSY state, since we're not going to emit a state-changed event when the BUSY state is cleared. Maybe some AT program will be fooled, and never try to refresh the menu. But if we don't use this patch, the AT program may get 0 child for menu, and not try to refresh it anyway.

On Windows the same role is used for menu items and parent menu items; however, the HASPOPUP state used to indicate the a menu is a parent menu item. We base that state on the tag name "menu" (we could change it to use nsIDOMXULContainerElement): http://mxr.mozilla.org/seamonkey/source/accessible/src/xul/nsXULMenuAccessible.cpp#292 But, we haven't had any problems using the tag name, or with the accessible children being out of date, on Windows. > Since menuitems have different parent with MSAA and ATK, > we need override CacheChildren for both nsXULMenuPopupAccessible, and > nsXULParentMenuitemAccessible. > Do you think it's a good idea? I'd rather not, at this point. For one thing, I'm afraid of regressions. What actual end-user bug does the new CacheChildren() fix? Instead of this fix, can't we have CacheChildren() force the menu to be generated?

> it doesn't fix the problem on Windows I'm not able to see a problem on Windows. What do you see?

Could we listen for menugenerated in nsDocAccessible::AttributChanged() and InvalidateCacheSubtree() when that happens?

Nevermind, I see your comment now: (In reply to comment #11) > We do nsXULMenupopupAccessible::GenerateMenu(mDOMNode); in > nsXULMenuitemAccessible::Init() > and wish children menuitem get generated immediately. > But they don't, AddLazyChildren() works in async way. Neil, does AddLazyChildren() have to be asynch?

Ginn, is this a cleaner way to make sure that when the menu is generated the children are invalidated?

(In reply to comment #34) > > it doesn't fix the problem on Windows > I'm not able to see a problem on Windows. What do you see? > I didn't see any problem on Windows, but in theory it's possible. Imagine we get a getChildCount call during GenerateMenu dispatches LazyGenerateChildrenEvent to queue. Then the menu/menupopup will claim it has no child forever. Your patch doesn't fix the problem, because we set menugenerated attribute to notify nsMenuPopupFrame to call AddLazyChildren(), but the loading of menu may not be done yet. We need to do InvalidateCacheSubtree() after LazyGeneratePopupDone() is called. So your patch just make InvalidateCacheSubtree() being called after AddLazyChildren(), but LazyGeneratePopupDone() is still after them. (if the menu is not loaded before) If we change 6312 nsCOMPtr<nsIRunnable> event = 6313 new LazyGenerateChildrenEvent(aContent, mPresShell, aCallback, aArg); 6314 return NS_DispatchToCurrentThread(event); to 6314 return NS_DispatchToMainThread(event, NS_DISPATCH_SYNC); The menu will be loaded synchronously. And our problem will be gone. I don't know what's the impact to general menu behavior.

Ginn, maybe we can do #ifdef ACCESSIBILITY if (presShell->IsAccessibiltyActive) { return NS_DispatchToMainThread(event, NS_DISPATCH_SYNC); } #endif nsCOMPtr<nsIRunnable> event = new LazyGenerateChildrenEvent(aContent, mPresShell, aCallback, aArg); return NS_DispatchToCurrentThread(event); ?

It would be easier if we just add an isSynch parameter to AddLazyChildren().

Comment on attachment 288829 [details] [diff] [review] patch, do AddLazyChildren synchronously if requested by a11y module r+ on a11y parts

Comment on attachment 288829 [details] [diff] [review] patch, do AddLazyChildren synchronously if requested by a11y module > nsresult AddLazyChildren(nsIContent* aContent, > nsLazyFrameConstructionCallback* aCallback, >- void* aArg); >+ void* aArg, PRBool aIsAsynch = PR_TRUE); Please reverse this to use aIsSynch, so that that the default can be aIsSynch = PR_FALSE

Comment on attachment 288970 [details] [diff] [review] patch addressing Neil's comment >+ // If aIsSynch is false, this method constructs the frames asynchronously. Reverse this comment. (If aIsSynch is true...) aArg); >- return NS_DispatchToCurrentThread(event); >+ return aIsSynch ? NS_DispatchToMainThread(event, NS_DISPATCH_SYNC) : >+ NS_DispatchToCurrentThread(event); Is there a reason to do synchronous this through a thread dispatch? You could just call LazyGenerateChildrenEvent::Run directly.

This still needs to be checked in.

I'll check it in tomorrow. The tree is red now. And we need to address bug 398362. After checking in this, opening menu for the first time will not select the first menuitem on Linux with a11y on.