Closed
Bug 324963
Opened 19 years ago
Closed 18 years ago
Menu highlight is broken/doesn't show up/not painted on mouse drag
Categories
(Core :: Web Painting, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: stephend, Assigned: roc)
References
Details
(Keywords: regression, Whiteboard: [blocking1.9a1+])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 1•19 years ago
|
||
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.
Comment 2•19 years ago
|
||
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
Comment 3•19 years ago
|
||
*** Bug 325203 has been marked as a duplicate of this bug. ***
![]() |
||
Updated•19 years ago
|
Assignee: nobody → roc
Component: Layout → Layout: View Rendering
QA Contact: layout → ian
Comment 4•19 years ago
|
||
*** Bug 325757 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 5•19 years ago
|
||
Works for me on Linux trunk. Is this Windows only?
Reporter | ||
Comment 6•19 years ago
|
||
(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.
Comment 7•19 years ago
|
||
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.
Assignee | ||
Comment 8•19 years ago
|
||
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...
Comment 9•19 years ago
|
||
*** Bug 328703 has been marked as a duplicate of this bug. ***
Comment 10•19 years ago
|
||
*** Bug 330288 has been marked as a duplicate of this bug. ***
![]() |
||
Updated•19 years ago
|
Flags: blocking1.9a1?
Reporter | ||
Updated•19 years ago
|
Summary: Menu highlight is broken/doesn't show up/not painted → Menu highlight is broken/doesn't show up/not painted on mouse drag
Comment 11•19 years ago
|
||
*** Bug 333652 has been marked as a duplicate of this bug. ***
Comment 12•19 years ago
|
||
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.
Comment 13•19 years ago
|
||
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
Comment 14•19 years ago
|
||
(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.
Comment 15•19 years ago
|
||
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.
Comment 16•19 years ago
|
||
mmortal03: Please read Comment 14&15.
Comment 17•18 years ago
|
||
I can't open the "Tools" or "Help" menus in the new Firefox 2.0 Beta 2.
Comment 18•18 years ago
|
||
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.
Updated•18 years ago
|
Flags: blocking1.9a1? → blocking1.9+
Comment 19•18 years ago
|
||
*** Bug 355710 has been marked as a duplicate of this bug. ***
Comment 20•18 years ago
|
||
(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.
Comment 21•18 years ago
|
||
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. :/
Comment 22•18 years ago
|
||
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.
Assignee | ||
Comment 23•18 years ago
|
||
Huh. Set gDumpEventList=1 in the debugger (or manually in the source) and get a dump of the results...
Comment 24•18 years ago
|
||
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.
Comment 25•18 years ago
|
||
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)
Comment 26•18 years ago
|
||
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.
Comment 27•18 years ago
|
||
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.
Assignee | ||
Comment 28•18 years ago
|
||
In comment #25 the first dump looks incorrect and the next three dumps look correct.
Comment 29•18 years ago
|
||
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?
Assignee | ||
Comment 30•18 years ago
|
||
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?
Whiteboard: [blocking1.9a1+]
Comment 31•18 years ago
|
||
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.
Comment 32•18 years ago
|
||
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)
Comment 33•18 years ago
|
||
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.
Assignee | ||
Comment 34•18 years ago
|
||
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?
Comment 35•18 years ago
|
||
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?
Assignee | ||
Comment 36•18 years ago
|
||
Is widget/win32 sending that event to the window widget instead of the menu popup's widget? If so, why?
Comment 37•18 years ago
|
||
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.
Assignee | ||
Comment 38•18 years ago
|
||
My comment #36 was about the first event dump in comment #25.
Comment 39•18 years ago
|
||
Yes, and in that case the targeted widget was the menu bar but with coordinates outside its area.
Assignee | ||
Comment 40•18 years ago
|
||
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?
Comment 41•18 years ago
|
||
No, mouse capture was on because the button was down. There's no problem when a button isn't kept down.
Reporter | ||
Comment 42•18 years ago
|
||
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.
Assignee | ||
Comment 43•18 years ago
|
||
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.
Assignee | ||
Comment 44•18 years ago
|
||
Try this, see if it helps. It should make the menupopup show up in the event dump list.
Comment 45•18 years ago
|
||
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.
Comment 46•18 years ago
|
||
Also the dump is unaffected.
Assignee | ||
Comment 47•18 years ago
|
||
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.
Assignee | ||
Comment 48•18 years ago
|
||
That is not a good idea because it will affect other things like scrolling an element that contains <menu> elements.
Assignee | ||
Comment 49•18 years ago
|
||
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.
Assignee | ||
Comment 50•18 years ago
|
||
I'll get Ere to try this when he arrives at the Firefox Summit tomorrow :-)
Attachment #243525 -
Attachment is obsolete: true
Assignee | ||
Comment 51•18 years ago
|
||
Previous patch was the wrong patch
Attachment #245613 -
Attachment is obsolete: true
Comment 52•18 years ago
|
||
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.
Attachment #245613 -
Attachment is obsolete: false
Assignee | ||
Comment 53•18 years ago
|
||
Come over here so we can fix some bugs together, mate!
Comment 54•18 years ago
|
||
I'm coming, but got to wait for the bus at 6:30 :)
Comment 55•18 years ago
|
||
Even the real plan B seems to work.
Assignee | ||
Updated•18 years ago
|
Attachment #245620 -
Flags: superreview?(mats.palmgren)
Attachment #245620 -
Flags: review?(mats.palmgren)
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?
Assignee | ||
Comment 57•18 years ago
|
||
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 58•18 years ago
|
||
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 59•18 years ago
|
||
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
Attachment #245620 -
Flags: superreview?(mats.palmgren)
Attachment #245620 -
Flags: superreview+
Attachment #245620 -
Flags: review?(mats.palmgren)
Attachment #245620 -
Flags: review+
Assignee | ||
Comment 60•18 years ago
|
||
(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);
Assignee | ||
Comment 61•18 years ago
|
||
(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.
Assignee | ||
Comment 62•18 years ago
|
||
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. :-)
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 63•18 years ago
|
||
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.
Comment 64•18 years ago
|
||
(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.
Comment 65•18 years ago
|
||
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.
Assignee | ||
Comment 66•18 years ago
|
||
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?
Reporter | ||
Comment 67•18 years ago
|
||
Verified FIXED using build 2006-11-30-09 of SeaMonkey trunk under Windows XP; thanks!
Status: RESOLVED → VERIFIED
Comment 68•18 years ago
|
||
This checkin might have regressed Bug 362498.
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•