Fixed mouseenter/mouseleave event firing order.
Fixed mouseenter/mouseleave event firing order to match the spec:
http://www.w3.org/TR/DOM-Level-3-Events/#events-mouseevent-event-order .
We have been dispatching mouseenter and mouseleave events before mouseover and mouseout events, and also sending mouseenter events to child nodes before their parents. This patch decouples mouseenter/mouseleave from active/hover states to fix the problem. Also cleans up Document.cpp a bit.
BUG=470947
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192954
https://codereview.chromium.org/1047733002/diff/20001/Source/core/page/EventHandler.cpp File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1047733002/diff/20001/Source/core/page/EventHandler.cpp#newcode1908 Source/core/page/EventHandler.cpp:1908: // for 'mouseleave' might set a capturing 'mouseenter' handler, ...
5 years, 8 months ago
(2015-03-31 18:06:27 UTC)
#5
https://codereview.chromium.org/1047733002/diff/20001/Source/core/page/EventH...
File Source/core/page/EventHandler.cpp (right):
https://codereview.chromium.org/1047733002/diff/20001/Source/core/page/EventH...
Source/core/page/EventHandler.cpp:1908: // for 'mouseleave' might set a
capturing 'mouseenter' handler, odd as that might be).
On 2015/03/31 11:45:34, Mike West wrote:
> What about other mutations? I think we had some issues a while ago with
> use-after-frees generated by unexpected mutations from mouse events that
removed
> elements we'd targeted for events. I think oilpan will take care of the danger
> here, but doesn't seem to bring sanity to the event chain. That is, if a
> mouseleave event reparents a node, we'll end up firing mouseenter events on
the
> wrong chain, won't we?
You are right but this CL is not introducing any new issues here with respect to
mutations through event handlers.
- This comment block (along with the core logic in the method) here has been
moved from 2nd half of Document::updateHoverActiveState(). I am sort of
preserving the old behavior here, specially in the next patch (Patch Set #3)
where I brought back the support for
adding-capturing-mouseenter-listener-in-mouseleave (and the test). The only
change is decoupling event-sending from active/hover states.
- I think use-after-free problem won't happen since the two lists are of
ref-counted pointers, and the caller passes the Node* parameters to this method
updateMouseEventTargetNode from a ref-counted type.
- Re DOM changes in event handlers: I think the spec is silent (or at least
sloppy) about it:
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener...
so it makes sense to optimize for things that are not forbidden. The existing
code does exactly that, so does this CL.
(Supporting all possible dynamic scenarios would make the logic complicated
and slow, and I don't think we need to go for this complication given the
little/unlikely benefit. Here is an argument to support my claim: no matter what
optimization we do, we can always come up with an event handler that would act
as an adversary. E.g., in the existing implementation, it is possible to change
the DOM so that the two lists and/or the common ancestor becomes useless, so we
will be forced to send events to /all/ ancestors and force the event mechanism
pay extra cost.)
Mike West
Would you mind adding a comment about our behavior with regard to DOM mutations in ...
5 years, 8 months ago
(2015-04-01 10:00:58 UTC)
#6
Would you mind adding a comment about our behavior with regard to DOM mutations
in event handlers, just so that it's clear in the code that the behavior might
or might not actually be correct?
As a followup, it would be valuable to figure out what other browser vendors do
for these kinds of mutations, and to work to ensure that we're interoperable.
I won't block this CL on that, however, since you correctly pointed out that
we're probably already doing the wrong thing. :)
LGTM, thanks for going another round.
mustaq
On 2015/04/01 10:00:58, Mike West wrote: > Would you mind adding a comment about our ...
5 years, 8 months ago
(2015-04-01 14:28:30 UTC)
#7
On 2015/04/01 10:00:58, Mike West wrote:
> Would you mind adding a comment about our behavior with regard to DOM
mutations
> in event handlers, just so that it's clear in the code that the behavior might
> or might not actually be correct?
>
> As a followup, it would be valuable to figure out what other browser vendors
do
> for these kinds of mutations, and to work to ensure that we're interoperable.
>
> I won't block this CL on that, however, since you correctly pointed out that
> we're probably already doing the wrong thing. :)
>
> LGTM, thanks for going another round.
Added a comment to clarify the possible issue, along with a FIXME.
Rick, does my new comment sound right w.r.t. what we discussed yesterday?
mustaq
Patchset #4 (id:60001) has been deleted
5 years, 8 months ago
(2015-04-01 14:42:26 UTC)
#8
Patchset #4 (id:60001) has been deleted
mustaq
The CQ bit was checked by mustaq@chromium.org to run a CQ dry run
5 years, 8 months ago
(2015-04-01 14:56:26 UTC)
#9
5 years, 8 months ago
(2015-04-01 15:03:32 UTC)
#12
Patchset #4 (id:80001) has been deleted
Rick Byers
On 2015/04/01 14:28:30, mustaq wrote: > On 2015/04/01 10:00:58, Mike West wrote: > > Would ...
5 years, 8 months ago
(2015-04-01 15:12:15 UTC)
#13
On 2015/04/01 14:28:30, mustaq wrote:
> On 2015/04/01 10:00:58, Mike West wrote:
> > Would you mind adding a comment about our behavior with regard to DOM
> mutations
> > in event handlers, just so that it's clear in the code that the behavior
might
> > or might not actually be correct?
> >
> > As a followup, it would be valuable to figure out what other browser vendors
> do
> > for these kinds of mutations, and to work to ensure that we're
interoperable.
> >
> > I won't block this CL on that, however, since you correctly pointed out that
> > we're probably already doing the wrong thing. :)
> >
> > LGTM, thanks for going another round.
>
> Added a comment to clarify the possible issue, along with a FIXME.
>
> Rick, does my new comment sound right w.r.t. what we discussed yesterday?
Yeah that sounds fine. I don't know if I "think" the spec says anything about
candidate
event listeners for this particular case (since we're talking about multiple
events here),
but the justification for that logic seems to apply here as well. Eg. I wonder
if
you could construct a simple case that causes an infinite event loop in IE or
Firefox
(eg. reparenting a node during mouseenter dispatch).
Anyway I think this captures our understanding well enough at the moment, and
the FIXME (which is supposed to be spelled 'TODO' now by the way - but no big
deal, not worth aborting the commit for) is the important thing. We're not
changing behavior in this regard, but we recognize that we should dig into it
further at some point.
mustaq
The CQ bit was checked by mustaq@chromium.org
5 years, 8 months ago
(2015-04-01 15:50:43 UTC)
#14
Issue 1047733002: Fixed mouseenter/mouseleave event firing order.
(Closed)
Created 5 years, 8 months ago by mustaq
Modified 5 years, 8 months ago
Reviewers: Mike West
Base URL: https://chromium.googlesource.com/chromium/blink.git@master
Comments: 12