398362 - First menuitem is not selected when opening a menu for the first time

Status: RESOLVED FIXED

(Core :: XUL, defect, P4)

Description

[Ginn Chen]

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.

Comment 1

[Ginn Chen]

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.

Comment 2

[Ria Klaassen (not reading all bugmail)]

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.

Comment 3

[Neil Deakin]

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.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Attachment: Screenshot

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.

Comment 5

[Ginn Chen]

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.

Comment 6

[Ginn Chen]

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.

Comment 7

[Ginn Chen]

Attachment: patch

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 8

[Neil Deakin]

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.

Comment 9

[Ginn Chen]

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

Comment 10

[Ginn Chen]

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

Comment 11

[Ginn Chen]

Attachment: patch v2

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

Comment 12

[Neil Deakin]

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

Comment 13

[Ginn Chen]

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 14

[Neil Deakin]

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?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Ginn Chen]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Ginn Chen]

Attachment: patch v2 with comments