Bug 102681 - Event's relatedTarget re-targeting does not occur for manually fired mouse events created by event.initMouseEvent().
Summary: Event's relatedTarget re-targeting does not occur for manually fired mouse ev...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on: 104249
Blocks: 59805 102069 102663 102666 103479
  Show dependency treegraph
 
Reported: 2012-11-19 05:25 PST by Hayato Ito
Modified: 2012-12-06 19:04 PST (History)
5 users (show)

See Also:


Attachments
WIP. Breaks a user generated event which sets a relatedTarget be the same to the target. (4.68 KB, patch)
2012-11-20 03:01 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Updates an event ancorstors caluculation algorithm. (7.72 KB, patch)
2012-11-22 00:53 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Rebased. ready for review. (7.55 KB, patch)
2012-11-25 21:46 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Prevetns dblclick event (13.03 KB, patch)
2012-12-05 00:31 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (13.34 KB, patch)
2012-12-05 23:10 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (13.34 KB, patch)
2012-12-06 00:17 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (13.49 KB, patch)
2012-12-06 18:24 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2012-11-19 05:25:44 PST
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.
Comment 1 Hayato Ito 2012-11-20 03:01:02 PST
Created attachment 175178 [details]
WIP. Breaks a user generated event which sets a relatedTarget be the same to the target.
Comment 2 Hayato Ito 2012-11-20 03:28:40 PST
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.
Comment 3 Hayato Ito 2012-11-20 03:35:01 PST
e.g. fast/events/set-event-to-null.html fails due to the https://www.w3.org/Bugs/Public/show_bug.cgi?id=20017.
Comment 4 Hayato Ito 2012-11-22 00:53:35 PST
Created attachment 175612 [details]
Updates an event ancorstors caluculation algorithm.
Comment 5 Hayato Ito 2012-11-22 05:40:17 PST
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.
Comment 6 Hayato Ito 2012-11-25 21:46:53 PST
Created attachment 175921 [details]
Rebased. ready for review.
Comment 7 Dimitri Glazkov (Google) 2012-12-03 10:43:35 PST
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.
Comment 8 Hayato Ito 2012-12-03 18:11:05 PST
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.
Comment 9 Dimitri Glazkov (Google) 2012-12-03 18:16:30 PST
(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?
Comment 10 Hayato Ito 2012-12-04 00:21:19 PST
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?
Comment 11 Hayato Ito 2012-12-04 04:47:32 PST
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.
Comment 12 Dimitri Glazkov (Google) 2012-12-04 07:40:33 PST
(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.
Comment 13 Hayato Ito 2012-12-04 22:15:39 PST
(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.
Comment 14 Hayato Ito 2012-12-05 00:31:15 PST
Created attachment 177686 [details]
Prevetns dblclick event
Comment 15 Dimitri Glazkov (Google) 2012-12-05 07:42:42 PST
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?
Comment 16 Hayato Ito 2012-12-05 21:59:44 PST
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.
Comment 17 Hayato Ito 2012-12-05 23:10:12 PST
Created attachment 177947 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-12-05 23:34:50 PST
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
Comment 19 Hayato Ito 2012-12-06 00:17:53 PST
Created attachment 177960 [details]
Patch for landing
Comment 20 WebKit Review Bot 2012-12-06 00:53:43 PST
Comment on attachment 177960 [details]
Patch for landing

Clearing flags on attachment: 177960

Committed r136818: <http://trac.webkit.org/changeset/136818>
Comment 21 WebKit Review Bot 2012-12-06 00:53:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 WebKit Review Bot 2012-12-06 03:47:15 PST
Re-opened since this is blocked by bug 104249
Comment 23 Hayato Ito 2012-12-06 18:24:05 PST
Created attachment 178129 [details]
Patch for landing
Comment 24 Hayato Ito 2012-12-06 18:25:08 PST
Let me reland, fixing content_browsertests in chromium port.

(In reply to comment #23)
> Created an attachment (id=178129) [details]
> Patch for landing
Comment 25 WebKit Review Bot 2012-12-06 19:04:21 PST
Comment on attachment 178129 [details]
Patch for landing

Clearing flags on attachment: 178129

Committed r136918: <http://trac.webkit.org/changeset/136918>
Comment 26 WebKit Review Bot 2012-12-06 19:04:28 PST
All reviewed patches have been landed.  Closing bug.