396799 - Menus on the Application menu bar sometimes claim to be menu items

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

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!

Comment 1

[Aaron Leventhal]

Is this a regression?

Comment 2

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

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?

Comment 3

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

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)

Comment 4

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

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

Comment 5

[Aaron Leventhal]

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

Comment 6

[Ginn Chen]

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

Comment 7

[Ginn Chen]

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.

Comment 8

[Willie Walker]

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.

Comment 9

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

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

Comment 10

[Ginn Chen]

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.

Comment 11

[Ginn Chen]

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

Comment 12

[Aaron Leventhal]

That probably changed recently with the menu rearchitecture.

Comment 13

[Ginn Chen]

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.

Comment 14

[Neil Deakin]

> 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.

Comment 15

[Ginn Chen]

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.

Comment 16

[Aaron Leventhal]

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

Comment 17

[Aaron Leventhal]

Attachment: Use Neil's suggestion -- works great

Comment 18

[alexander :surkov (:asurkov)]

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

Comment 19

[Aaron Leventhal]

Attachment: Use Surkov's suggestion -- QI -- avoids dependency on tag and is smaller code

Comment 20

[Ginn Chen]

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

Comment 21

[Ginn Chen]

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

Comment 22

[Aaron Leventhal]

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.

Comment 23

[Ginn Chen]

Attachment: draft patch to reset childcount

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.

Comment 24

[Neil Deakin]

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

Comment 25

[Ginn Chen]

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.
 

Comment 26

[Ginn Chen]

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

Comment 27

[Ginn Chen]

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 28

[Ginn Chen]

Attachment: workaround it in accessible module

Comment 29

[Aaron Leventhal]

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 30

[Ginn Chen]

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 31

[Ginn Chen]

Comment on attachment 286956 [details] [diff] [review]
draft patch to reset childcount

it doesn't fix the problem on Windows

Comment 32

[Ginn Chen]

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.

Comment 33

[Aaron Leventhal]

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?

Comment 34

[Aaron Leventhal]

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

Comment 35

[Aaron Leventhal]

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

Comment 36

[Aaron Leventhal]

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? 

Comment 37

[Aaron Leventhal]

Attachment: Idea: InvalidateCacheSubtree() will invalidate the children of the menu on a delay -- AddLazyChildren() should be finished by then

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

Comment 38

[Ginn Chen]

(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.

Comment 39

[Aaron Leventhal]

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);

?

Comment 40

[Ginn Chen]

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

Comment 41

[Ginn Chen]

Attachment: patch, do AddLazyChildren synchronously if requested by a11y module

Comment 42

[Aaron Leventhal]

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

r+ on a11y parts

Comment 43

[Neil Deakin]

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 44

[Ginn Chen]

Attachment: patch addressing Neil's comment

Comment 45

[Neil Deakin]

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.

Comment 46

[Ginn Chen]

Attachment: patch to commit in

Comment 47

[Aaron Leventhal]

This still needs to be checked in.

Comment 48

[Ginn Chen]

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.