Build ID: 20060126 of either Firefox or SeaMonkey on trunk, Windows XP. I think this is fallout from bug 317375 Steps to Reproduce: 1. Click and hold down the left mouse button on any top-level menu item (Help, for instance). 2. Now, still holding the left mouse button, scroll through the submenus. Expected Results: We should still paint the highlight (which here is blue) Actual Results: No highlight appears until you depress the left mouse button and click again.

With left click & hold on Menu, and move down, menu items are highlighted with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060124 Firefox/1.6a1 Not highlighted with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060127 Firefox/1.6a1 So bug 317375 is certainly a likely candidate.

works in 20060125 1745pst build fails in 20060126 0944pst build http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20060125+1745&maxdate=20060126+0944&cvsroot=%2Fcvsroot

*** Bug 325203 has been marked as a duplicate of this bug. ***

*** Bug 325757 has been marked as a duplicate of this bug. ***

Works for me on Linux trunk. Is this Windows only?

(In reply to comment #5) > Works for me on Linux trunk. Is this Windows only? Someone with a Mac could and should answer, but it seems to be, yeah.

Note that if you have a toolbar or bookmark toolbar button which is partly underneath the menu, you can see it get highlighted as you move the pointer over where the covered portion of it would be in the menu. So it seems the mouseover signal is getting sent to the button below the menu instead of to the menu item.

Hmm, sounds like nsDisplayXULEventRedirector isn't working, but I'm not sure why that would only be a problem on Windows. It probably isn't hard to fix though. Set gDumpEventList=1 and see what you get...

*** Bug 328703 has been marked as a duplicate of this bug. ***

*** Bug 330288 has been marked as a duplicate of this bug. ***

*** Bug 333652 has been marked as a duplicate of this bug. ***

I debugged this a little bit a while ago and found that the events were getting sent to the wrong native widget, and therefore to the wrong frame, so that the nsXULEventRedirector never actually came into play. I don't know if I'll be able to get back to debugging it, so I thought I'd just mention it here.

Maybe a separate bug, but seems related, and this is in the Branch now: If you click and hold down the left mouse button on any top level menu, and then go down to a point past the tab bar and into where the page is displayed, the menu highlight will stop displaying. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060610 BonEcho/2.0a3 ID:2006061009

(In reply to comment #13) > Maybe a separate bug, but seems related, and this is in the Branch now: That's definitely another bug. This bug is about a regression from a patch that landed only on the trunk. If there isn't already another bug describing your problem, you should file a new bug.

Sounds like Bug 324251, i had a hard time reproducing that bug so (and while this Bug here is not fixed it's hard to see that bug in action). If you have any idea when this regressed on the branch, add that info to Bug 324251, maybe with that info we could then find out what patch is responsible for this.

mmortal03: Please read Comment 14&15.

I can't open the "Tools" or "Help" menus in the new Firefox 2.0 Beta 2.

Oh nevermind they work now. This problem is actually really annoying because it also affects right-click context menu sub-menus (not on default, but extensions that add them) on Windows, which is for "quick access". You have to click twice and it really slows down.

*** Bug 355710 has been marked as a duplicate of this bug. ***

(In reply to comment #6) > (In reply to comment #5) > > Works for me on Linux trunk. Is this Windows only? > > Someone with a Mac could and should answer, but it seems to be, yeah. This now happens on Mac now that Cocoa widgets have been enabled on the trunk, but the bookmark toolbar menus are the only ones affected AFAICT.

Actually it also happens on the back/forward history menus as well, I almost never use that function so didn't notice it until just now. :/

I debugged this a bit. nsLayoutUtils::GetFrameForPoint(frame, eventPoint); in PresShell::HandleEvent() returns wrong frame. For example with bookmarks menu open hovering over it where toolbar is underneath, toolbar frame is returned instead of the bookmars menu frame.

Huh. Set gDumpEventList=1 in the debugger (or manually in the source) and get a dump of the results...

I did that once and it only showed toolbox and associated stuff, nothing about the menu popup. I can try to get the complete output tonight.

This is the dump when hovering above "Bookmark This page..." mouse button pressed: Event handling --- (2910,660): Background 023FD76C(RootBox(window)(-1)) (0,0,8865,10140) opaque uniform Background 023FDAF8(DocElementBox(window)(-1)) (0,0,8865,10140) uniform Clip 00000000() (0,0,8865,10140) Background 03715E38(Box(toolbox)(26)) (0,0,8865,1275) Border 03715E38(Box(toolbox)(26)) (0,0,8865,1275) Background 0376AC98(Box(toolbar)(1)) (0,330,8865,555) uniform Border 0376AC98(Box(toolbar)(1)) (0,330,8865,555) Background 0392DB44(Box(toolbaritem)(5)) (2880,345,3747,525) uniform Background 0392DC4C(Box(hbox)(0)) (2880,412,3747,390) uniform After that, if there is a button below the cursor under the menu popup, the tooltip flashes quickly and the following is added to the dump: Event handling --- (435,300): Background 04A935E0(MenuPopup(menupopup)(0)) (0,0,4380,12000) opaque uniform Border 04A935E0(MenuPopup(menupopup)(0)) (0,0,4380,12000) Background 04A936BC(Box(arrowscrollbox)(-1)) (45,45,4290,11910) uniform Background 04A9385C(XULScroll(scrollbox)(-1)) (45,195,4290,11610) uniform Clip 00000000() (45,195,4290,11610) Background 04A937F4(Box(scrollbox)(-1)) (45,195,4290,21810) uniform Clip 00000000() (45,195,4290,11610) Background 04A9391C(Box(box)(-1)) (45,195,4290,21810) uniform Background 04A939D8(Menu(menuitem)(0)) (45,195,4290,255) uniform Border 04A939D8(Menu(menuitem)(0)) (45,195,4290,255) XULEventRedirector 04A939D8(Menu(menuitem)(0)) (331,210,2714,225) Background 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225) uniform Clip 00000000() (45,195,4290,11610) XULEventRedirector 04A939D8(Menu(menuitem)(0)) (331,210,2714,225) EventReceiver 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225) XULTextBox 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225) Event handling --- (435,300): Background 04A935E0(MenuPopup(menupopup)(0)) (0,0,4380,12000) opaque uniform Border 04A935E0(MenuPopup(menupopup)(0)) (0,0,4380,12000) Background 04A936BC(Box(arrowscrollbox)(-1)) (45,45,4290,11910) uniform Background 04A9385C(XULScroll(scrollbox)(-1)) (45,195,4290,11610) uniform Clip 00000000() (45,195,4290,11610) Background 04A937F4(Box(scrollbox)(-1)) (45,195,4290,21810) uniform Clip 00000000() (45,195,4290,11610) Background 04A9391C(Box(box)(-1)) (45,195,4290,21810) uniform Background 04A939D8(Menu(menuitem)(0)) (45,195,4290,255) uniform Border 04A939D8(Menu(menuitem)(0)) (45,195,4290,255) XULEventRedirector 04A939D8(Menu(menuitem)(0)) (331,210,2714,225) Background 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225) uniform Clip 00000000() (45,195,4290,11610) XULEventRedirector 04A939D8(Menu(menuitem)(0)) (331,210,2714,225) EventReceiver 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225) XULTextBox 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225) Perhaps this happens because of MouseTrailer. Anyway, it all went wrong already in the very beginning. This is the dump when hovering above "Bookmark This page..." mouse button pressed (how it should be, I guess): Event handling --- (540,315): Background 04A935E0(MenuPopup(menupopup)(0)) (0,0,4380,12000) opaque uniform Border 04A935E0(MenuPopup(menupopup)(0)) (0,0,4380,12000) Background 04A936BC(Box(arrowscrollbox)(-1)) (45,45,4290,11910) uniform Background 04A9385C(XULScroll(scrollbox)(-1)) (45,195,4290,11610) uniform Clip 00000000() (45,195,4290,11610) Background 04A937F4(Box(scrollbox)(-1)) (45,195,4290,21810) uniform Clip 00000000() (45,195,4290,11610) Background 04A9391C(Box(box)(-1)) (45,195,4290,21810) uniform Background 04A939D8(Menu(menuitem)(0)) (45,195,4290,255) opaque uniform Border 04A939D8(Menu(menuitem)(0)) (45,195,4290,255) XULEventRedirector 04A939D8(Menu(menuitem)(0)) (331,210,2714,225) Background 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225) uniform Clip 00000000() (45,195,4290,11610) XULEventRedirector 04A939D8(Menu(menuitem)(0)) (331,210,2714,225) EventReceiver 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225) XULTextBox 04A93ADC(TextBox(label)(-1)[value=Bookmark This Page...]) (331,210,2714,225)

When I looked into this a while ago, I seem to recall that the wrong native widget was getting the mouseover event, so we were calling GetFrameForPoint on the wrong frame (the root content frame if the menu hovered over the page), meaning that the we weren't finding the menu frame and adding it to the event display list.

That's not happening in my tests. GetFrameForPoint is correctly called on the menu frame but result is wrong if the cursor is not over it. Also native widget layer seems to work properly or at least I failed to spot any error.

In comment #25 the first dump looks incorrect and the next three dumps look correct.

Now I don't claim to know nsLayoutUtils::GetFrameForPoint or its inner workings too well, but do we have a fundamental problem here? It's being called with aFrame=menubar but coordinates outside it (mouse capture until button is released). Still, BuildDisplayListForStackingContext is called for this frame we're not over. Will it ever even consider the other frame (=menu) that's on top of its siblings or parent?

I'm really confused. Here's what I get on GTK2 where things seem to work fine: Event handling --- (870,180): Background 0x2aaab67f0f98(MenuPopup(menupopup)(0)) (0,0,5085,7470) Background 0x2aaab67f1098(Box(arrowscrollbox)(-1)) (30,15,5025,7440) uniform Background 0x2aaab5c02b18(XULScroll(scrollbox)(-1)) (30,15,5025,7440) uniform Clip (nil)() (30,15,5025,7440) Background 0x2aaab5c02a80(Box(scrollbox)(-1)) (30,15,5025,7440) uniform Clip (nil)() (30,15,5025,7440) Background 0x2aaab5c02c40(Box(box)(-1)) (30,15,5025,7440) uniform Background 0x2aaab5c02cd8(Menu(menuitem)(0)) (30,15,5025,330) XULEventRedirector 0x2aaab5c02cd8(Menu(menuitem)(0)) (390,60,3690,240) Background 0x2aaab5c02dd8(TextBox(label)(-1)[value=Bookmark This Page...]) (390,60,3690,240) uniform Clip (nil)() (30,15,5025,7440) XULEventRedirector 0x2aaab5c02cd8(Menu(menuitem)(0)) (390,60,3690,240) EventReceiver 0x2aaab5c02dd8(TextBox(label)(-1)[value=Bookmark This Page...]) (390,60,3690,240) XULTextBox 0x2aaab5c02dd8(TextBox(label)(-1)[value=Bookmark This Page...]) (390,60,3690,240) This is what I expect AND it's exactly the same as all but the first dump in comment #25. Are you actually getting different dumps on Windows or not?

Yes, all three dumps come every time after moving mouse when there is a button underneath. The first (incorrect) one comes with the actual mouse move using the menu bar frame and coordinates outside of it. The latter (correct-lookinh) two come slightly after that when the button tooltip flashes quickly and then disappears, but they refer to the menu frame using its coordinates. They must be synthesized because the mouse capture directs all real events to the menu bar.

The correct dumps don't show up at all if there's nothing that would show a tooltip at the cursor position under the menu frame. In this case all dumps show content underneath the menu frame, for example the following if the url bar is underneath: Event handling --- (3585,540): Background 02E61ED4(RootBox(window)(-1)) (0,0,11430,10275) opaque uniform Background 02E62260(DocElementBox(window)(-1)) (0,0,11430,10275) uniform Clip 00000000() (0,0,11430,10275) Background 036EECD0(Box(toolbox)(26)) (0,0,11430,1275) Border 036EECD0(Box(toolbox)(26)) (0,0,11430,1275) Background 037437A0(Box(toolbar)(1)) (0,330,11430,555) uniform Border 037437A0(Box(toolbar)(1)) (0,330,11430,555) Background 03904CAC(Box(toolbaritem)(5)) (2880,345,5799,525) uniform Background 03904DB4(Box(hbox)(0)) (2880,412,5799,390) uniform Background 039130AC(Box(textbox)(0)) (2925,442,5379,330) opaque uniform Border 039130AC(Box(textbox)(0)) (2925,442,5379,330) Background 0391320C(Box(hbox)(-1)) (2940,457,5094,300) opaque uniform Background 0391384C(Box(hbox)(0)) (3315,510,4389,195) uniform Background 039138C0(nsTextControlFrame) (3315,510,4389,195) uniform Clip 00000000() (0,0,11430,10275) WrapList 03913B78(HTMLScroll(div)(-1)) (3315,510,4389,195) Background 03913B78(HTMLScroll(div)(-1)) (3315,510,4389,195) uniform Clip 00000000() (3315,510,4389,195) Background 03913CBC(Area(div)(-1)) (3315,510,3840,195) uniform Clip 00000000() (3315,510,4389,195) Text 044E5B10(Text(0)) (3330,510,3810,195)

Should we actually try to get rid of mouse capture in Win32 widget code? I've been now running a short while without it and haven't had any problems.

So Windows automatically continues to send mouse events as long as the left button is down, even if the mouse moves outside the window? What about other mouse buttons?

Yes, but without capture they are directed to the window under the cursor, not the one where the button was pressed. I believe this applies to any button, but this could be checked. Anyway, the big question is: is it seems some expectations have changed, what does the higher level code now expect the widget layer to do while a mouse button is pressed?

Is widget/win32 sending that event to the window widget instead of the menu popup's widget? If so, why?

Currently with capture the window at cursor when the mouse button was pressed gets all events until the button is released. Perhaps because then the window where button is pressed will also know when it is released. And this used to work, so maybe that was how the correct behavior was specified. But I can't be sure as I wasn't around then. According to CVS blame it was implemented in nsWindow.cpp revision 3.157 by kmcclusk at 1999-06-21 so it's been a while.

My comment #36 was about the first event dump in comment #25.

Yes, and in that case the targeted widget was the menu bar but with coordinates outside its area.

So that was because we had mouse capture on? But we shouldn't have had mouse capture on because the mouse wasn't down, right?

No, mouse capture was on because the button was down. There's no problem when a button isn't kept down.

I don't know if this helps or not, but if you do the following, it paints: 1. Click on the menu item, holding down the mouse. 2. Scroll (notice it doesn't paint). 3. Release the mouse click 4. Try scrolling with the mouse button down: this usually works for me then.

aaah okay. The right thing to do here, then, is to make event display list construction walk into popups so we can find the popup under the cursor even when the event isn't for the popup widget.

Try this, see if it helps. It should make the menupopup show up in the event dump list.

Comment on attachment 243525 [details] [diff] [review] possible fix I didn't see the patch make any difference. Either I screwed up or it doesn't work.

Also the dump is unaffected.

I think I know what the problem is. The menuFrame is being excluded from the display list because its overflow area doesn't contain the menupopup's area, and therefore doesn't contain the event point. After the call to SyncLayout here http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsMenuFrame.cpp#1097 we need to GetOverflowRect(), union it with the area of the menupopup, and do FinishAndStoreOverflow.

That is not a good idea because it will affect other things like scrolling an element that contains <menu> elements.

I'm trying an approach where we track the active popup list in the root pres context and dispatch positioned events to those popups as necessary from PresShell::HandleEvent. That could fix this bug and will also be useful when we need to process synthetic mouse events after views have gone away.

I'll get Ere to try this when he arrives at the Firefox Summit tomorrow :-)

Previous patch was the wrong patch

Comment on attachment 245613 [details] [diff] [review] Plan B I've already arrived and just had to test this right away, and it actually works :) I'm probably not qualified to review this, but it seems to do the trick.

Come over here so we can fix some bugs together, mate!

I'm coming, but got to wait for the bus at 6:30 :)

Even the real plan B seems to work.

If Mats doesn't have time to do look at this in the next day or so, is there someone else that can do the review?

dbaron, bz or bryner could also review this ... I just try to keep load off them. It's fairly simple, just what I said in comment #49. We need to ensure that popups are always treated as "on top" of everything, over and above any normal CSS z-index processing, and are always targeted by events even though their frames do not contribute to the overflow areas of their ancestors (unlike all other frames). The view manager used to do this but it doesn't anymore because frame display lists have taken over those functions. So this patch gives a way for the root prescontext to track all active popups (although right now only menus are tracked) and take them into account when dispatching positioned events.

Comment on attachment 245620 [details] [diff] [review] REAL Plan B >Index: layout/base/nsPresContext.cpp >+// We may want to replace this with something faster... >+nsPresContext* >+nsPresContext::RootPresContext() That doesn't seem necessary to me, for what you use it for. >Index: layout/base/nsPresContext.h >+ // Find the prescontext for the root of the view manager hierarchy that contains >+ // this >+ nsPresContext* RootPresContext(); Please add a period at the end of comments that starts with a capital letter. In this case maybe even " prescontext." I traced the calls to NotifyAddedActivePopupToTop/NotifyRemovedActivePopup, and for context menus we do call "Removed" (from Destroy) but not "Added", is this intentional? >Index: layout/base/nsPresShell.cpp >+ if (popup->GetOverflowRect().Contains( >+ nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, popup))) { >+ // The event should target the popup >+ frame = popup; Ok, do we need to change the event coordinates accordingly?

Comment on attachment 245620 [details] [diff] [review] REAL Plan B >Index: layout/base/nsPresContext.h >+ NS_ASSERTION(this == RootPresContext(), "Only on root prescontext"); Add that assert to the other three methods too? >Index: layout/base/nsPresShell.cpp >@@ -13,25 +13,25 @@ >- * Håkan Waara <hwaara@chello.se> >+ * H�kan Waara <hwaara@chello.se> Don't make that change, tempting though it is? ;) >@@ -5854,25 +5854,47 @@ PresShell::HandleEvent(nsIView * >+ // We're starting our search from the root so it should include any >+ // popups (if something is capturing the mouse, this would not be true) I'm not sure I follow that parenthetical. With those nits, r+sr=bzbarsky

(In reply to comment #58) > (From update of attachment 245620 [details] [diff] [review] [edit]) > >Index: layout/base/nsPresContext.cpp > >+// We may want to replace this with something faster... > >+nsPresContext* > >+nsPresContext::RootPresContext() > > That doesn't seem necessary to me, for what you use it > for. Yeah. However, I anticipate that later we will start using it for more things (when nsViewManager goes away, all the RootViewManager stuff will become RootPresContext()). > I traced the calls to NotifyAddedActivePopupToTop/NotifyRemovedActivePopup, > and for context menus we do call "Removed" (from Destroy) but not "Added", > is this intentional? No, where should the call to NotifyAddedActivePopupToTop be added? > >Index: layout/base/nsPresShell.cpp > >+ if (popup->GetOverflowRect().Contains( > >+ nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, popup))) { > >+ // The event should target the popup > >+ frame = popup; > > Ok, do we need to change the event coordinates accordingly? No, because just below that we do nsPoint eventPoint = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, frame);

(In reply to comment #59) > Add that assert to the other three methods too? Sure > >Index: layout/base/nsPresShell.cpp > >@@ -13,25 +13,25 @@ > >- * H�kan Waara <hwaara@chello.se> > >+ * H�kan Waara <hwaara@chello.se> > > Don't make that change, tempting though it is? ;) Yeah > >@@ -5854,25 +5854,47 @@ PresShell::HandleEvent(nsIView * > >+ // We're starting our search from the root so it should include any > >+ // popups (if something is capturing the mouse, this would not be true) > > I'm not sure I follow that parenthetical. I'll reword it. The idea is that if something is capturing the mouse then we would not be here, and we wouldn't want to be here because we shouldn't be directing mouse events to popups in that case.

checked in. According to Mats, there may be issues with context menus. If that's so, file a followup bug, preferably with a hint for where would be a good place to insert the Add/Remove code. :-)

Actually, this bug won't manifest with context menus because I think they always capture the mouse. Still, it would be nice to have context menus (and combobox popups) in the popup list for completeness, if we ever start using it for something else.

(In reply to comment #63) I filed bug 362293 to follow up on the context menus. Note that submenus of context menus *are* added/removed with the patch.

Note that most of this patch will be obsolete with the patch bug 279703, which stores all popups in a list, so I don't think it's worth spending a lot of extra time catching edge cases.

Cool! Hmm, when that lands we can probably get rid of the prescontext's list. Can we fit nsComboboxControlFrame popups into the new popup manager too?

Verified FIXED using build 2006-11-30-09 of SeaMonkey trunk under Windows XP; thanks!

This checkin might have regressed Bug 362498.