Bug 13274 - REGRESSION: Mouse events are not always sent to the "captured" subframe when dragging
Summary: REGRESSION: Mouse events are not always sent to the "captured" subframe when ...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC OS X 10.4
: P1 Normal
Assignee: Dave Hyatt
Depends on:
Reported: 2007-04-03 16:48 PDT by Dex Deacon
Modified: 2007-04-06 14:55 PDT (History)
1 user (show)

See Also:

test page (106 bytes, text/html)
2007-04-03 16:48 PDT, Dex Deacon
no flags Details
subframe for test page (1.86 KB, text/html)
2007-04-03 16:52 PDT, Dex Deacon
no flags Details
possible patch (2.75 KB, patch)
2007-04-04 14:43 PDT, Dex Deacon
hyatt: review-
Details | Formatted Diff | Diff
Build upon previous patch (4.51 KB, patch)
2007-04-05 23:06 PDT, Dave Hyatt
hyatt: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dex Deacon 2007-04-03 16:48:05 PDT
If you click and hold the mouse in a subframe, then drag the mouse out of that subframe, it will stop receiving mousemove events.  Interestingly, if you drag the mouse over a *different* subframe, the original subframe will receive the events.  This is because of some weird logic in EventHandler; passMouseMoveEventToSubframe is called only if the mouse is over a subframe, and the target is then changed to the "captured" subframe's view (m_mouseDownView).

I think the fix is to remember the captured subframe, and call passMouseMoveEventToSubframe if either 1) we have a capture subframe, or 2) the mouse is over a subframe.  m_capturingMouseEventsNode looks like a convenient place to store the captured subframe.

Test case coming shortly.
Comment 1 Dex Deacon 2007-04-03 16:48:54 PDT
Created attachment 13940 [details]
test page
Comment 2 Dex Deacon 2007-04-03 16:52:59 PDT
Created attachment 13941 [details]
subframe for test page

Save this as "subframe.html" in the same place as the above file.
Comment 3 Dex Deacon 2007-04-04 14:43:09 PDT
Created attachment 13950 [details]
possible patch

Here's my attempt at a patch.  It uses the existing m_capturingMouseEvents variable on EventHandler to remember that future events should be routed to a subframe.  The layout tests pass, but I'm not quite sure if it would interfere with other uses of m_capturingMouseEvents (or if it does, maybe it should).
Comment 4 Dave Hyatt 2007-04-04 15:00:54 PDT
Comment on attachment 13950 [details]
possible patch

I don't think this is the right approach.  There are really two kinds of capture: there is capture for a node within a document, and then there is widget-level capture (which can apply to plugins or to frames).

I would argue that it is a platform's responsibility to do widget-level capture (and this capture automatically takes care of the subframe problem, since events just go straight to the correct widget).
Comment 5 Dave Hyatt 2007-04-04 15:08:01 PDT
This problem basically needs to be addressed in such a way that WebKit plugins work too.  I think it's really just a Mac-specific WebKit event routing issue and that the fix will end up being in the Mac WebKit code.

Comment 6 Dex Deacon 2007-04-05 16:47:04 PDT
I think if you do widget-level capture, you'll end up with a weird inconsistency.  Right now, mousemove events are sent to every frame that the mouse is over (for example, in my test page, mouse over the subframe, and both the subframe and toplevel receive mousemove events).  With widget-level capture, only the captured widget will receive events.  So you'll have this situation where the toplevel frame always receives mousemove events, unless a subframe has capture.

Not sure if that really matters, though.
Comment 7 Dave Hyatt 2007-04-05 17:06:36 PDT
I agree that it would be inconsistent, but I think it's a bug that the mousemove goes to the parent at all.

The parent frame should not be getting mousemoves while you are in the child frame in my opinion (and you'll see that it does not in Firefox).
Comment 8 Dave Hyatt 2007-04-05 17:07:15 PDT
We're effectively firing two unique mousemove events and sending them to each frame.  That's not correct and is worthy of a completely separate bug.
Comment 9 Dave Hyatt 2007-04-05 17:40:25 PDT
Looking into this more, this code is a total mess.

The lastMouseMovedFrame (whose purpose is really just to properly send a mouseout) is improperly interacting with the Mac-specific m_mouseDownView.

The m_mouseDownView code is also broken and doesn't even work.

I'm going to look into patching this.
Comment 10 Dave Hyatt 2007-04-05 21:22:03 PDT
Safari 2 behaves correctly regarding capture behavior,  so this is a P1 regression.
Comment 11 Dave Hyatt 2007-04-05 21:23:35 PDT
Note that in Safari 2, the capture mousemoves also go to all frames in the chain (just like non-capturing mousemoves), so Safari 2 is consistent.
Comment 12 Dave Hyatt 2007-04-05 22:49:56 PDT
So I've basically done a complete 180 here and now think your patch is the way to go.  It has the advantage of still working even if a widget toolkit sends things directly to a specific widget.
Comment 13 Dave Hyatt 2007-04-05 23:06:56 PDT
Created attachment 13975 [details]
Build upon previous patch

This builds on the previous patch and also fixes the mouse move problem.

I also tweaked invalidateClick for the case where a widget toolkit does end up going directly to a specific subframe so that m_mousePressed won't get "stuck" but clears immediately as mouse down delivers events to subframes.

So now we basically have three kinds of capture: frame capture, widget capture and node capture.  Frame and node capture are at least using a similar cross-platform mechanism.  

Widget capture is basically buggy on the Mac now, but we've wallpapered over the most obvious bugs by getting frame capture right.  Eventually Mac will start sending events right to subviews, but in the mean time adding this extra frame capture layer seems pretty straightforward and safe.
Comment 14 Dave Hyatt 2007-04-05 23:13:27 PDT
Comment on attachment 13975 [details]
Build upon previous patch

Hmmm still have some issues with this patch.

(1) It's bad to do extra hit testing if you are already capturing.  Should restructure the code to not waste time doing that.

(2) Clearing capturing has to be robust and crawl up the frame tree to future-proof for widget toolkits sending events straight to specific subframes.
Comment 15 Dave Hyatt 2007-04-06 14:55:28 PDT
Fixed in r20761.