Summary: | REGRESSION: Mouse events are not always sent to the "captured" subframe when dragging | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dex Deacon <occupant4> | ||||||||||
Component: | Forms | Assignee: | Dave Hyatt <hyatt> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap | ||||||||||
Priority: | P1 | ||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Dex Deacon
2007-04-03 16:48:05 PDT
Created attachment 13940 [details]
test page
Created attachment 13941 [details]
subframe for test page
Save this as "subframe.html" in the same place as the above file.
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 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).
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. 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. 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). 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. 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. Safari 2 behaves correctly regarding capture behavior, so this is a P1 regression. 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. 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. 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 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.
Fixed in r20761. |