[webkit-reviews] review denied: [Bug 18930] mouseenter and mouseleave events not supported : [Attachment 143540] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 18930: mouseenter and mouseleave events not supported
https://bugs.webkit.org/show_bug.cgi?id=18930

Attachment 143540: Patch
https://bugs.webkit.org/attachment.cgi?id=143540&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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.