Closed Bug 398362 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: XUL, defect, P4)

x86
Linux
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

Details

Attachments

(2 files, 2 obsolete files)

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

Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #287391 - Flags: review?(enndeakin)
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.
Attachment #287391 - Flags: review?(enndeakin) → review-
(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.
Flags: blocking1.9?
Summary: First menuitem is not selected if switched from another menu for the first time → First menuitem is not selected when opening a menu for the first time
Attached patch patch v2 (obsolete) — Splinter Review
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);
     }
Attachment #287391 - Attachment is obsolete: true
Attachment #289781 - Flags: review?(enndeakin)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
(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?
Attachment #289781 - Flags: review?(enndeakin) → review+
Attachment #289781 - Flags: superreview?(bzbarsky)
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.
Attachment #289781 - Attachment is obsolete: true
Attachment #294641 - Flags: superreview?(bzbarsky)
Attachment #289781 - Flags: superreview?(bzbarsky)
Attachment #294641 - Flags: superreview?(bzbarsky) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.