On GNOME, first selectable menuitem in the menu should be selected when the menu is open. Steps for Firefox, 1) Press Alt+F, File menu pops, and "New Window" is selected. 2) Press right arrow, Edit menu pops, but no menuitem is selected. 3) Press right arrow again, "Select All" in Edit menu is selected. or Press left arrow and then press right arrow, "Select All" in Edit menu is selected. If the menu has popup once, it can't be reproduce until restart Firefox.

Another issue: Start Firefox For the first time, you press Alt+E, (or Alt+s, Alt+B), the first selectable menuitem in menu Edit (or History, Bookmarks) is NOT selected. But, if you press Alt+F, (or Alt+V, Alt+T), the first selectable menuitem in menu File (or View, Tools) is selected. FWIW: If you're testing with Firefox earlier than Oct.2 nightly, move your mouse cursor away from Firefox window, otherwise it will affect your results.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100223 Minefield/3.0a9pre Testing Comment 0, and using a new profile, this is WFM. I could however reproduce the issue partially (first item not being selected) with my default profile, but not always. Tested this about 4 times, so it still could be coincidence.

Don't see this issue myself on Windows. Haven't tested on Linux, but can't think of any reason offhand why this would be a platform specific difference.

Hm, there is something weird going on. I saw two menus selected at the same time. This suddenly showed up using a new profile without extensions.

Don't go that far. This bug is reported for Linux. I guess something like Bug 396869 can cause this. And also that's a timing issue, e.g. I saw Bug 387664 time by time. I'll do more tests when I'm back in office next week.

Neil, the platform different is on Linux, we don't treat disabled menuitem as a valid menuitem. see eMetric_SkipNavigatingDisabledMenuItem. This bug was caused by 2 reason: 1) The menu is lazy generated. In LazyGeneratePopupDone, we will select the first item. 492 PRBool selectFirstItem = (PRBool)NS_PTR_TO_INT32(aArg); 493 if (selectFirstItem) { 494 nsMenuFrame* next = pm->GetNextMenuItem(popupFrame, nsnull, PR_TRUE); 495 popupFrame->SetCurrentMenuItem(next); 496 } 497 498 pm->UpdateMenuItems(popup); But the disabled attr is not set until pm->UpdateMenuItems(popup); Thus, we select a disabled menuitem. The solution is simple, just move line 498 ahead. 2) When a11y is on, we generate menus when when create nsXULMenupopupAccessible. It's quite possible that we push 2 LazyGenerateChildrenEvent for same menu in queue. One is from nsXULMenupopupAccessible, doesn't have aSelectFirstItem. The other is from nsMenuPopupFrame::ShowPopup, have aSelectFirstItem == PR_TRUE. But we only do LazyGeneratePopup once. The first event wins.

1) call callback func, even it's already generated. 2) change the sequence of UpdateMenuItems and GetNextMenuItem I'm not sure whether UpdateMenuItems is needed after SetCurrentMenuItem. The patch works fine for me. Another thing is we do SetGeneratedChildren() in both nsCSSFrameConstructor::LazyGenerateChild and LazyGeneratePopupDone, I think we only need one.

Comment on attachment 287391 [details] [diff] [review] patch >Index: base/nsCSSFrameConstructor.cpp >- if (menuPopupFrame->HasGeneratedChildren()) >+ if (menuPopupFrame->HasGeneratedChildren()) { >+ if (mCallback) { >+ mCallback(mContent, frame, mArg); >+ } > return NS_OK; >+ } I don't understand this change. If the children have already been generated, then we shouldn't be doing this again. Does the patch to change menugenerated to use synchronous building make this unneccesary? > if (pm && popupFrame->IsMenu()) { > nsCOMPtr<nsIContent> popup = aPopup; >+ pm->UpdateMenuItems(popup); >+ > PRBool selectFirstItem = (PRBool)NS_PTR_TO_INT32(aArg); > if (selectFirstItem) { > nsMenuFrame* next = pm->GetNextMenuItem(popupFrame, nsnull, PR_TRUE); > popupFrame->SetCurrentMenuItem(next); > } >- >- pm->UpdateMenuItems(popup); Calling UpdateMenuItems can destroy popupFrame. You'll need to check if it is still alive using the weakFrame defined earlier.

(In reply to comment #8) > (From update of attachment 287391 [details] [diff] [review]) > I don't understand this change. If the children have already been generated, > then we shouldn't be doing this again. If a11y is active, by clicking the menu in menubar will create menuitem accessible, which will push a LazyGenerateChildrenEvent in queue with null mArg. Right after that, menupopup will push another LazyGenerateChildrenEvent in queue with mArg is TRUE. It will not select the first menuitem because we do nothing for the second LazyGenerateChildrenEvent. > Does the patch to change menugenerated > to use synchronous building > make this unneccesary? Exactly.

(In reply to comment #9) > > Does the patch to change menugenerated > > to use synchronous building > > make this unneccesary? > > Exactly. > I was wrong about this. Actually the fix of bug 396799 makes this bug more obvious with a11y on on Linux. The sequence of 2 LazyGenerateChildrenEvent are different when you are doing, 1# Press Alt+F 2# Select File->New Window item, press right arrow. The event sequence of 1# is, menupopup first, a11y menugenerate second, the sequence of 2# is, a11y menugenerate first, menupopup second. If we've already fixed pm->UpdateMenuItems(popup); 1# will success, 2# will fail. If we have made a11y menugenerate a sync method, 2# will success, but 1# will fail, because although a11y menugenerate comes later, but it will finish earlier and doesn't select the first menuitem. Is calling mCallback twice bad? We won't call it for the 3rd time.

same patch with checking weakFrame.IsAlive(). removing the extra popupFrame->SetGeneratedChildren(); we do it in nsCSSFrameConstructor.cpp. I'm not sure whether the checking of weakFrame.IsAlive() is still necessary below. if (weakFrame.IsAlive()) { popupFrame->PresContext()->PresShell()-> FrameNeedsReflow(popupFrame, nsIPresShell::eTreeChange, NS_FRAME_HAS_DIRTY_CHILDREN); }

(In reply to comment #10) > 1# will success, 2# will fail. > If we have made a11y menugenerate a sync method, 2# will success, but 1# will > fail, because although a11y menugenerate comes later, but it will finish > earlier and doesn't select the first menuitem. > > Is calling mCallback twice bad? > We won't call it for the 3rd time. > The only way the generated children flag can be set without the callback being called is if mCallback is null (which is never the case), or if ProcessChildren fails. I want to know what is going on that makes calling the callback necessary.

The problem is, we may have 2 LazyGenerateChildrenEvent for the same menu. A is synch and not select the first item, 1560 PresContext()->PresShell()->FrameConstructor()-> 1561 AddLazyChildren(mContent, LazyGeneratePopupDone, nsnull, PR_TRUE); B is asynch and select the first item, 532 PresContext()->PresShell()->FrameConstructor()-> 533 AddLazyChildren(mContent, LazyGeneratePopupDone, NS_INT32_TO_PTR(aSelectFirstItem)); If A gets in when B is in queue, we will not do mCallback for B, thus the first item will not be selected. We need to do mCallback again for selecting the first menuitem.

Comment on attachment 289781 [details] [diff] [review] patch v2 OK, let's just go with this then. This bug only applies when accessibility is in use right?

I would like to see some clear documentation (in the relevant header) of the API change happening here before I can review...

Neil, for a11y in use, the bug here is first menu item will not be selected for first time opening of a menu. (the change of nsCSSFrameConstructor.cpp fixed it) for a11y not in use, the bug here is first menu item will be selected no matter it is disabled or not for first time opening of a menu. On Linux, disabled menuitem should not be selected. (the change of nsMenuPopupFrame.cpp fixed it) bz, Besides the bug fixing in nsMenuPopupFrame.cpp, the change of nsCSSFrameConstructor.cpp is callback of LazyGenerateChildrenEvent will always be called even the children had been generated before. Do you think we should mention it in nsCSSFrameConstructor.h? mCallback and even LazyGenerateChildrenEvent are not documented at all.

I think the behavior of all API methods should be clearly documented. If there is no documentation now, such that consumers don't know what behavior to expect and we get bugs like this one, then we need to fix that.