Bug 18930

This would be incredibly usefull to have, imho. In the mean time, use something like this: http://blog.stchur.com/2007/03/15/mouseenter-and-mouseleave-events-for-firefox-and-other-non-ie-browsers/

I agree that this is useful and it is part of the latest draft of DOM Level 3 Events. When thinking through how to implement this we realized that there are issues with capturing events. Should we dispatch an event on every ancestor of the target? How many times does a capturing listener get called during a mouseenter? I don't have access to IE9 at the moment but I'm curious about the behavior there.

> would be incredibly useful Could someone please post a rationale for having these events? Lots of devices don't even have a mouse these days. I'm not saying that these events shouldn't be implemented, but there's likely a difficult discussion ahead, and listing the reasons for having the events is essential.

Currently, it's slow, error-prone, and requires extra bytes to implement standard idioms (e.g, a context menu). mouseenter/mouseleave mirror CSS :hover. Quirks mode has a good discussion of how the events work, and the boilerplate needed to test an onmouse[over/out] event to see if the mouse actually entered/left: http://www.quirksmode.org/js/events_mouse.html http://www.quirksmode.org/dom/events/mouseover.html

(In reply to comment #3) > > would be incredibly useful > > Could someone please post a rationale for having these events? Lots of devices don't even have a mouse these days. > > I'm not saying that these events shouldn't be implemented, but there's likely a difficult discussion ahead, and listing the reasons for having the events is essential. Why would there be a difficult discussion? It seems like a no-brainer. It's a standard event and most browsers support it. Web devs want it. What's difficult?

For the context of how I found this bug. We already hand-roll it in at least one place: http://google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/shadow/MediaControls.cpp&exact_package=chromium&l=543 And this implementation is flawed, because it doesn't account for multiple mouseover events fired, thus unnecessarily calling updateControlVisibility().

(In reply to comment #5) > (In reply to comment #3) > > > would be incredibly useful > > > > Could someone please post a rationale for having these events? Lots of devices don't even have a mouse these days. > > > > I'm not saying that these events shouldn't be implemented, but there's likely a difficult discussion ahead, and listing the reasons for having the events is essential. > > Why would there be a difficult discussion? It seems like a no-brainer. It's a standard event and most browsers support it. Web devs want it. What's difficult? DOOH. Only IE supports them. Sorry about that :) Still, it seems that it's a pretty needed event. See ppk's shouting and search for jquery mouseenter/mouselave.

> Why would there be a difficult discussion? I explained it in comment 3. There is no mouse on many devices used to access the Internet these days. If these are to be added to WebKit, it's compatibility and not "incredible usefulness" that should drive adding them, in my opinion.

(In reply to comment #8) > > Why would there be a difficult discussion? > > I explained it in comment 3. There is no mouse on many devices used to access the Internet these days. If these are to be added to WebKit, it's compatibility and not "incredible usefulness" that should drive adding them, in my opinion. Can you explain a bit more here? For no-mouse devices, sure, these events would not be very useful. But there are plenty (still the absolute majority, right?) of devices with mice and there's lots of JS code written trying to solve (inefficiently) this same problem. Why not help out the Web developers?

(In reply to comment #8) > > Why would there be a difficult discussion? > > I explained it in comment 3. There is no mouse on many devices used to access the Internet these days. Well sure but Webkit supports *all* other mouse event types (even mousewheel) and mouseenter and mouseleave are part of the DOM Level 3 events[0], so I'm not sure the objection still has much (if any) relevance, especially since mouseenter and mouseleave are far more useful than *already included* mouse events (namely mouseover and mouseout) > If these are to be added to WebKit, it's compatibility and not "incredible usefulness" that should drive adding them, in my opinion. Why would "incredible usefulness" not play a part? Because they *are* incredibly useful (in that they're useful at all, which mouseover and mouseout aren't exactly, you usually have to hack around *a lot* to make them useable) [0] http://www.w3.org/TR/DOM-Level-3-Events/#event-type-mouseenter

Consider the 4 year old bug #13343, incorrect margin right computed style values. It was old, there were workarounds everywhere, and devs were dealing with it. It didn't stop us from fixing it. Now both IE and Opera 11 support these events. Let's leave Mozilla in the dust and just do it, or likely the opposite will occur.

(In reply to comment #11) > Now both IE and Opera 11 support these events. Let's leave Mozilla in the > dust and just do it, or likely the opposite will occur. Heh. Just FYI, Webkit is now the only engine not supporting mouseenter/leave. There are some differences in the implementations, and I'll file also some (HTML) spec bugs.

FF is already supporting those events....

Created attachment 143540 [details] Patch

I don't understand how the patch works. You may need to send multiple events if mouse is moved to be on top of some DOM subtree to be over another DOM subtree. All the elements in the subtrees get mouseleave or mouseenter.

Comment on attachment 143540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143540&action=review > Source/WebCore/page/EventHandler.cpp:2206 > + // sned mouseleave event to the old node if the new node is not a descendant of the old node. Typo: s/sned/send Wouldn't the proper behavior be to fire the events on all nodes, not just the "old node"? Any of the nodes could have a mouseenter/mouseleave event handler attached. Maybe I'm missing something, but a simple explanation is warranted I believe.

Comment on attachment 143540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143540&action=review > Source/WebCore/page/EventHandler.cpp:2215 > + // send mouseenter event to the new node if the old node is not a descendant of the new node. Same here, again wouldn't you fire the event on all nodes and not just the "new node"?

Comment on attachment 143540 [details] Patch Attachment 143540 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12769225 New failing tests: fast/events/mouseover-mouseout2.html

Created attachment 143560 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 143540 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143540&action=review Glad to see you are trying to fix this! This is a bit trickier than it seems at first. Here are a bunch of comments you may find useful: > Source/WebCore/ChangeLog:6 > + Implemented mouseenter and mouseleave events, because firefox has already supported these I see Firefox is getting no respect -- all lower-case :) Actually, a more accurate statement would be "because all other browsers support them". > Source/WebCore/ChangeLog:28 > + can-bubble and not cancelable. These are different from other mouse events. "are not can-bubble" -> don't bubble. The last sentence is extraneous. > Source/WebCore/ChangeLog:32 > + Added mouseenterAttr nad mouseleaveAttr in the same manner as mouseoverAttr Just "added respective attributes" is enough. > Source/WebCore/ChangeLog:37 > + Added mouseover and mouseleave interfaces. They are not interfaces. Just attributes. >> Source/WebCore/page/EventHandler.cpp:2206 >> + // sned mouseleave event to the old node if the new node is not a descendant of the old node. > > Typo: s/sned/send > > Wouldn't the proper behavior be to fire the events on all nodes, not just the "old node"? Any of the nodes could have a mouseenter/mouseleave event handler attached. Maybe I'm missing something, but a simple explanation is warranted I believe. Jarred and Olli are right. You need to consider this: what effect does entering/leaving node A to B have on ancestors of A and B? What does it mean in terms dispatching events? Additionally, since you will be dispatching multiple events in an existing code path, we must consider the performance impact of this change. I would be very careful here and make sure we don't regress. > LayoutTests/fast/events/mouseenter-mouseleave.html:39 > +<div id="inner2" style="width:20px; height:20px; background-color:yellow; top:30px; left:60px; position:absolute" Probably need a few more tests based on discussion so far.

any news here?

Created attachment 165998 [details] Patch A new modified version of Takashi's patch. The logic for issuing the events have been rewritten to be more correct with respect to all the nodes that should and should not have these events issued, but has an optimization where they are not issued when there is no listeners for them. This last part should avoid any performance regressions, but at the cost that mouseevent and mouseleave will not always working as might be expected for capture phase event listeners installed on ancestor elements (They will just have to rely on mouseover and mouseout instead).

Comment on attachment 165998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165998&action=review > Source/WebCore/dom/Node.cpp:2739 > + if (!hovered() && hasEventListeners(eventNames().mouseleaveEvent)) { hasEventListeners is not correct. We need to check if any of the ancestors have an event listener for this.

(In reply to comment #23) > (From update of attachment 165998 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165998&action=review > > > Source/WebCore/dom/Node.cpp:2739 > > + if (!hovered() && hasEventListeners(eventNames().mouseleaveEvent)) { > > hasEventListeners is not correct. We need to check if any of the ancestors have an event listener for this. Not when the event doesn't bubble, of course if we want capture to work, then we would need to check all the ancestor.

Comment on attachment 165998 [details] Patch Attachment 165998 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14035847

Created attachment 166005 [details] Patch Update the test-case so it actually tests a case where a single mouseout/mouseover event becomes multiple mouseenter/mouseleave events.

Created attachment 166007 [details] Patch Fix build issue on Chromium.

(In reply to comment #22) > ...will not always working as might be expected for capture phase event listeners installed on ancestor elements (They will just have to rely on mouseover and mouseout instead). That seems unacceptable to me. (In reply to comment #24) > Not when the event doesn't bubble, of course if we want capture to work, then we would need to check all the ancestor. We do want capture to work. We need to potentially dispatch an event on every ancestor. I think the cleanest thing to do here is to define a custom dispatch. This way we can gather the ancestors once instead of once per ancestor. This custom dispatch can check if there is a listener too. In the case where there are no listeners we walk the ancestors once. In the case where there are listeners the dispatch will still be O(n^2) where n is the depth of the tree.

(In reply to comment #28) > (In reply to comment #22) > > ...will not always working as might be expected for capture phase event listeners installed on ancestor elements (They will just have to rely on mouseover and mouseout instead). > > That seems unacceptable to me. > Okay, I thought it was a very clean and easily communicable limitation. It even has easy workaround for anybody who wants to use capture event listeners, they just need to add empty listeners to the elements they want events for. > (In reply to comment #24) > I think the cleanest thing to do here is to define a custom dispatch. This way we can gather the ancestors once instead of once per ancestor. This custom dispatch can check if there is a listener too. In the case where there are no listeners we walk the ancestors once. In the case where there are listeners the dispatch will still be O(n^2) where n is the depth of the tree. Checking the ancestors is not the only thing that cause O(n²) behaviour. There is also a Node::contains() check to avoid wrong mouseleave events. My plan for a correct solution is to eventually move the handling to Document::updateHoverActiveState, which has a list of nodes whose hover state changes. The problem is that it currently operates in hittesting and not in event-handling, so it does not have access to all the mouse-event parameters it needs to dispatch a mouse event. To avoid iterating over all the ancestors again and again, we would need a mechanism to collect the ancestors for event dispatching once, and reuse it for several events. The problem with all this is that it requires a few refactorings. The solution I posted here was a fast easy way to solve the problem with one clear and simple limitation, which can be used until we have a full implementation.

Any progress on this issue? Just asking because last activity happened 4 months ago.

Created attachment 197580 [details] Patch New rebased patch, now with support for capturing event handlers too

Discussion on webkit-dev raised no objections and Maciej wrote: 'Seems like a reasonable feature to add.'. The feature has also been discussed on blink-dev, and blink is just waiting for the final patch before porting. Now that the political side about whether we should have the feature in this form has been decided on, does anyone dare review the details of the implementation?

Comment on attachment 197580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=197580&action=review r=me > Source/WebCore/dom/Document.cpp:5994 > + bool seenCommonAncestor = false; This should be "sawCommonAncestor"

Created attachment 199805 [details] Patch

Comment on attachment 199805 [details] Patch Clearing flags on attachment: 199805 Committed r149173: <http://trac.webkit.org/changeset/149173>

All reviewed patches have been landed. Closing bug.