Bug 102681 – Event's relatedTarget re-targeting does not occur for manually fired mouse events created by event.initMouseEvent().

• Component: UI Events
• Status: RESOLVED FIXED

[Hayato Ito]

Event retargeting is correctly done for UA's mouse events, but is not done at all for manually generated mouse events.

See https://bugs.webkit.org/show_bug.cgi?id=102663 for example.

[Hayato Ito]

Created attachment 175178 [details]
WIP. Breaks a user generated event which sets a relatedTarget be the same to the target.

[Hayato Ito]

I filed a bug for the shadow dom spec.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=20017

We have to resolve the bug before we land this patch.

[Hayato Ito]

e.g. fast/events/set-event-to-null.html fails due to the https://www.w3.org/Bugs/Public/show_bug.cgi?id=20017.

[Hayato Ito]

Created attachment 175612 [details]
Updates an event ancorstors caluculation algorithm.

[Hayato Ito]

This patch will conflict a patch in https://bugs.webkit.org/show_bug.cgi?id=103058.

After https://bugs.webkit.org/show_bug.cgi?id=103058 is landed, let me resolve a conflict.

[Hayato Ito]

Created attachment 175921 [details]
Rebased. ready for review.

[Dimitri Glazkov (Google)]

Comment on attachment 175921 [details]
Rebased. ready for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=175921&action=review

> Source/WebCore/dom/Node.cpp:2589
> +    if (event->isMouseEvent())
> +        return EventDispatcher::dispatchEvent(this, MouseEventDispatchMediator::create(adoptRef(toMouseEvent(event.leakRef()))));

Whoa. Should we be doing this? It seems that we used to treat synthetic events as second class, always dispatching them as generic events. Now, we're actually trying to dispatch them as MouseEvents. I understand this seems like an improvement (web devs should be able to synthesize real-looking events), but it's also a compat change and as such, has platform implications.

[Hayato Ito]

Thank you for the review.

(In reply to comment #7)
> (From update of attachment 175921 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175921&action=review
> 
> > Source/WebCore/dom/Node.cpp:2589
> > +    if (event->isMouseEvent())
> > +        return EventDispatcher::dispatchEvent(this, MouseEventDispatchMediator::create(adoptRef(toMouseEvent(event.leakRef()))));
> 
> Whoa. Should we be doing this? It seems that we used to treat synthetic events as second class, always dispatching them as generic events. Now, we're actually trying to dispatch them as MouseEvents. I understand this seems like an improvement (web devs should be able to synthesize real-looking events), but it's also a compat change and as such, has platform implications.

I don't have an intention to introduce such a change. I just wanted to re-use code path of re-targeting.

I am afraid that I don't understand the impact of this change. Let me catch up what you said.

I guess this change is over-kill and we need another way to re-use code path of re-targeting, which will require some refactoring.

[Dimitri Glazkov (Google)]

(In reply to comment #8)

> I guess this change is over-kill and we need another way to re-use code path of re-targeting, which will require some refactoring.

I never said it was an overkill. It's possible that this is the exact change we need. I just didn't realize that we're introducing a change to how we dispatch synthetic events. I am not an oracle who bestows the final decision :)

Will this be detectable by the author?

[Hayato Ito]

Okay. As far as I can tell, this patch changes only the behavior of synthetic events in the meaning that retargeting is correctly done for synthetic events also.

Although I am sure that nothing has changed other than that, let me double-check the code path and update the bug soon. Al least, all existing layout tests pass.

(In reply to comment #9)
> (In reply to comment #8)
> 
> > I guess this change is over-kill and we need another way to re-use code path of re-targeting, which will require some refactoring.
> 
> I never said it was an overkill. It's possible that this is the exact change we need. I just didn't realize that we're introducing a change to how we dispatch synthetic events. I am not an oracle who bestows the final decision :)
> 
> Will this be detectable by the author?

[Hayato Ito]

According to MouseEventDispatchMediator::dispatchEvent(), the only difference user can notice is:

After the patch:
   'dblclick' event might be also fired if user creates a synthetic mouse event with parameters, (eventType =="click" and details=2) in addition to the 'click' event.

Before the patch:
   'dblclick' event is not fired in that case.

I think it's okay to fire a 'dblclick' event in this case so that the behavior of synthetic mouse events matches the behavior of native mouse events. But that should be out of scope of this patch.

Let me update the patch so that the patch does not change the behavior.
The difficult part is that it seems that we cannot tell whether the mouse event is an synthetic event or a native mouse event from the MouseEventDispatchMediator::dispatchEvent(). Let me investigate further.

[Dimitri Glazkov (Google)]

(In reply to comment #11)
> According to MouseEventDispatchMediator::dispatchEvent(), the only difference user can notice is:
> 
> After the patch:
>    'dblclick' event might be also fired if user creates a synthetic mouse event with parameters, (eventType =="click" and details=2) in addition to the 'click' event.
> 
> Before the patch:
>    'dblclick' event is not fired in that case.
> 
> I think it's okay to fire a 'dblclick' event in this case so that the behavior of synthetic mouse events matches the behavior of native mouse events. But that should be out of scope of this patch.

Interesting. Is dblclick fired in IE/Gecko in such case? Because if it does, this patch is actually a good thing for compatibility.

[Hayato Ito]

(In reply to comment #12)
> (In reply to comment #11)
> > According to MouseEventDispatchMediator::dispatchEvent(), the only difference user can notice is:
> > 
> > After the patch:
> >    'dblclick' event might be also fired if user creates a synthetic mouse event with parameters, (eventType =="click" and details=2) in addition to the 'click' event.
> > 
> > Before the patch:
> >    'dblclick' event is not fired in that case.
> > 
> > I think it's okay to fire a 'dblclick' event in this case so that the behavior of synthetic mouse events matches the behavior of native mouse events. But that should be out of scope of this patch.
> 
> Interesting. Is dblclick fired in IE/Gecko in such case? Because if it does, this patch is actually a good thing for compatibility.

I've confirmed that dblclick is *not* fired in Gecko (Firefox 17.0.1 on Mac) in this case.
So I think we should not change the current behavior of WebKit.

[Hayato Ito]

Created attachment 177686 [details]
Prevetns dblclick event

[Dimitri Glazkov (Google)]

Comment on attachment 177686 [details]
Prevetns dblclick event

View in context: https://bugs.webkit.org/attachment.cgi?id=177686&action=review

> Source/WebCore/dom/MouseEvent.cpp:212
> +PassRefPtr<MouseEventDispatchMediator> MouseEventDispatchMediator::create(PassRefPtr<MouseEvent> mouseEvent, bool isSyntheticMouseEvent)

Bool flag, yuck. Can you make it an enum? Also -- is there any information on the event itself that tells us it's a synthetic event?

[Hayato Ito]

Comment on attachment 177686 [details]
Prevetns dblclick event

View in context: https://bugs.webkit.org/attachment.cgi?id=177686&action=review

>> Source/WebCore/dom/MouseEvent.cpp:212
>> +PassRefPtr<MouseEventDispatchMediator> MouseEventDispatchMediator::create(PassRefPtr<MouseEvent> mouseEvent, bool isSyntheticMouseEvent)
> 
> Bool flag, yuck. Can you make it an enum? Also -- is there any information on the event itself that tells us it's a synthetic event?

Okay. Let me make it enum.

There is no available info on the event itself. That's sad.

[Hayato Ito]

Created attachment 177947 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177947 [details]
Patch for landing

Rejecting attachment 177947 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rom out/Release/obj/gen/webkit/bindings/V8DerivedSources04.cpp:54:
Source/WebCore/dom/MouseEvent.h: In member function 'bool WebCore::MouseEventDispatchMediator::isSyntheticMouseEvent() const':
Source/WebCore/dom/MouseEvent.h:126: error: statement has no effect
Source/WebCore/dom/MouseEvent.h:126: error: no return statement in function returning non-void
make: *** [out/Release/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources04.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/15157675

[Hayato Ito]

Created attachment 177960 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177960 [details]
Patch for landing

Clearing flags on attachment: 177960

Committed r136818: <http://trac.webkit.org/changeset/136818>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 104249

[Hayato Ito]

Created attachment 178129 [details]
Patch for landing

[Hayato Ito]

Let me reland, fixing content_browsertests in chromium port.

(In reply to comment #23)
> Created an attachment (id=178129) [details]
> Patch for landing

[WebKit Review Bot]

Comment on attachment 178129 [details]
Patch for landing

Clearing flags on attachment: 178129

Committed r136918: <http://trac.webkit.org/changeset/136918>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.