Bug 102681

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

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.

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

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

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.

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

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.

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.

(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?

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?

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.

(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.

(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.

Created attachment 177686 [details] Prevetns dblclick event

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 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.

Created attachment 177947 [details] Patch for landing

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

Created attachment 177960 [details] Patch for landing

Comment on attachment 177960 [details] Patch for landing Clearing flags on attachment: 177960 Committed r136818: <http://trac.webkit.org/changeset/136818>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 104249

Created attachment 178129 [details] Patch for landing

Let me reland, fixing content_browsertests in chromium port. (In reply to comment #23) > Created an attachment (id=178129) [details] > Patch for landing

Comment on attachment 178129 [details] Patch for landing Clearing flags on attachment: 178129 Committed r136918: <http://trac.webkit.org/changeset/136918>

All reviewed patches have been landed. Closing bug.