388359 - Menu items are highlighted as mouse pointer moves below menu

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Steve England [:stevee]]

1. New profile, start firefox
2. Left click on the Help menu
3. Move the mouse down the menu items and when you reach the bottom of the menu keep moving the mouse down

As you move the mouse pointer down below the menu list, you will see that some items in the menu are reselected as you move. This problem also applies to the right-click context menu.

Works fine:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007071606 Minefield/3.0a7pre

Broken:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007071608 Minefield/3.0a7pre

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1184590620&maxdate=1184597639

Due to bug 388064

Comment 1

[Eli Friedman]

The reason we're hitting this is because the root prescontext is not necessarily the prescontext for the root widget.  Essentially, we weren't checking in this case before.  Therefore, the fact that popup coordinates don't deal with subdocuments correctly is now an issue.

Comment 2

[Eli Friedman]

Ouch, this kinda sucks.  We really don't want to be making multiple requests for screen coordinates for every single mousemove, especially on Linux, but there's no other way to generically handle the coordinates.

BTW, why exactly do we have to search the list of popups from top to bottom using coordinates?  Is there some reason we can't trust the OS to dispatch events to the right popup?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mouse event dispatch goes through the presshell which uses display lists to find the correct frame. The always-on-top behaviour of popups cannot be handled by display lists --- I tried doing that but it was horrible because popups don't contribute to the overflow area of their parents, and a popup could be anywhere in the frame tree, so we have to scan the entire frame tree every time.

Comment 4

[Eli Friedman]

Couldn't we special-case the situation where the event widget is a popup/child of a popup?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There are problems with synthetic mouse-move events (e.g. the events generated after a reflow moves content). See bug 324963.

Comment 6

[Eli Friedman]

Then I guess we have no choice but to do these checks in screen coordinates.  (Hopefully it won't hurt Linux perf too much.)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about we put the screen coordinates in the mouse-move event, so we never have to get them more than once? (And I wonder if there's a way to get Linux to put the screen coordinates in the event to start with...)

Comment 9

[Eli Friedman]

We'd still have to get the screen coordinates for every popup; we don't store that, at least at the moment, and I don't know if gdk caches it.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

that's true. It's probably tough to cache since the window manager can move it anytime.

If you're worried about this, can we take an optimized path where we compare the widget of the popup to the widget in the event and use the widget coordindates in the event if they're the same?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, could you explain exactly what the situation is here, elaborating on comment #1?

Comment 12

[Eli Friedman]

Okay, a clearer description of comment 1:

The patch in bug 388064 gets rid of the per-root (viewmanager/prescontext) popup tracking.  This means that we're actually checking popups that don't have the same root.

The reason this is broken is that GetEventCoordinatesRelativeTo doesn't work correctly across roots.  The only way to fix that is to make it use screen coordinates to do the relevant translation.

Maybe I shouldn't worry about Linux perf, though, because we usually have few popups.

Comment 13

[Eli Friedman]

Attachment: Patch

If this ends up being a significant performance hit on Linux, we can optimize the case where we're not crossing roots.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This can actually get called from lots of places via nsLayoutUtils::GetEventCoordinatesRelativeTo... Identifying a regression on Linux could be complex. Would you mind doing that optimization now? We can do something pretty simple, e.g. define a helper function GetRootWidget(nsIWidget*) that walks the nsIWidget parent chain to the root, and then compare GetRootWidget(aWidget) with GetRootWidget(vieWidget).

Comment 15

[Eli Friedman]

Attachment: Patch v2

I'm not very confident that I got everything right, but it seems to work.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is more complex than necessary. Why not just have a helper that gets the root widget for a widget and the offset to that root, and call it twice, falling back to the screen-coordinates if the roots are different? The early exit when you find one of the widgets is an ancestor of the others seems like overkill to me. Saving X server round trips is a big deal but a few extra memory references and loop iterations isn't.

Comment 17

[Eli Friedman]

Attachment: Patch v3

Okay, this should be fine.  (If we actually cared about the performance of this function, we would move the offset calculation into widget; on Windows at least, it's actually faster to do the calculation in screen coordinates.)

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 273063 [details] [diff] [review]
Patch v3

looks good, but see Karl's comment

Comment 19

[Karl Tomlinson (:karlt)]

Linux will need a nsWindow::ScreenToWidget implementation.

I assume this is just an inverse of WidgetToScreen, but I don't understand why WidgetToScreen uses gdk_window_get_root_origin (including window manager frame) instead of gdk_window_get_origin for container widgets.

Comment 20

[Eli Friedman]

(In reply to comment #19)
> I assume this is just an inverse of WidgetToScreen, but I don't understand why
> WidgetToScreen uses gdk_window_get_root_origin (including window manager frame)
> instead of gdk_window_get_origin for container widgets.

Yes; I can switch to using WidgetToScreen if that would be easier.

I'm guessing that gdk_window_get_root_origin and gdk_window_get_origin return the same thing for child windows, so it doesn't actually make a difference.

Comment 22

[Karl Tomlinson (:karlt)]

Attachment: nsWindow::ScreenToWidget for gtk2

I think that, as nsIWidget has a ScreenToWidget, we should implement it
(and the TranslateWidgetToView looks better with ScreenToWidget).

I just inverted WidgetToScreen as is, as the important thing here is that the two are inverses.

Comment 23

[Karl Tomlinson (:karlt)]

Eli,
Please feel free to checkin my patch as well as yours (at an appropriate moment), if you are happy with this solution, as I don't have a cvs account yet.

Comment 24

[Eli Friedman]

Checked in.

Comment 26

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007072505 Minefield/3.0a7pre ID:2007072505
VERIFIED