DOMWindow::dispatchEvent() does not reset the event's dispatch flag.
Created attachment 325118 [details] Patch
Comment on attachment 325118 [details] Patch Attachment 325118 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5008337 New failing tests: inspector/model/remote-object-dom.html
Created attachment 325127 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325118 [details] Patch Attachment 325118 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5008218 New failing tests: inspector/model/remote-object-dom.html
Created attachment 325128 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 325118 [details] Patch Attachment 325118 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5009081 New failing tests: inspector/model/remote-object-dom.html
Created attachment 325129 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 325118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325118&action=review > Source/WebCore/page/DOMWindow.cpp:2017 > + event.resetPropagationFlags(); Can we do this in EventDispatcher.cpp instead? That file should really have everything in https://dom.spec.whatwg.org/#concept-event-dispatch.
(In reply to Ryosuke Niwa from comment #8) > Comment on attachment 325118 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325118&action=review > > > Source/WebCore/page/DOMWindow.cpp:2017 > > + event.resetPropagationFlags(); > > Can we do this in EventDispatcher.cpp instead? That file should really have > everything in https://dom.spec.whatwg.org/#concept-event-dispatch. I think you are referring to another code path (which likely already does the right thing). This particular DOMWindow::dispatchEvent(Event&, EventTarget*) is called from places like DOMWindow::dispatchLoadEvent(), not EventDispatcher.
Created attachment 325163 [details] Patch
Comment on attachment 325163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325163&action=review > Source/WebCore/page/DOMWindow.cpp:2017 > + event.setCurrentTarget(nullptr); > + event.setEventPhase(Event::NONE); > + event.resetPropagationFlags(); It’s not a great pattern to have this list of three things that needs to be done; I wish there was some way to structure things so it was not possible to forget one of these. For example, is there any call site that needs to reset the propagation flags without also setting the event phase to none? If not, then it seems we should be calling one function that does both rather than calling two functions. We are treating the Event object as a sort of "data holder" here; maybe EventDispatcher could provide public functions that can do the slightly higher level combinations correctly? Or maybe Event itself could have a bit more policy and bit less "setters for every field" in the public functions
(In reply to Darin Adler from comment #11) > Comment on attachment 325163 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=325163&action=review > > > Source/WebCore/page/DOMWindow.cpp:2017 > > + event.setCurrentTarget(nullptr); > > + event.setEventPhase(Event::NONE); > > + event.resetPropagationFlags(); > > It’s not a great pattern to have this list of three things that needs to be > done; I wish there was some way to structure things so it was not possible > to forget one of these. For example, is there any call site that needs to > reset the propagation flags without also setting the event phase to none? If > not, then it seems we should be calling one function that does both rather > than calling two functions. > > We are treating the Event object as a sort of "data holder" here; maybe > EventDispatcher could provide public functions that can do the slightly > higher level combinations correctly? Or maybe Event itself could have a bit > more policy and bit less "setters for every field" in the public functions EventDispatcher::dispatchEvent() and DOMWindow::dispatchEvent() both following the same pattern: event.setCurrentTarget(nullptr); event.resetPropagationFlags(); event.setEventPhase(Event::NONE); This matches pretty well the spec which says: - Unset event’s dispatch flag, stop propagation flag, and stop immediate propagation flag. - Set event’s eventPhase attribute to NONE. - Set event’s currentTarget attribute to null. The duplication between EventDispatcher::dispatchEvent() and DOMWindow::dispatchEvent() is unfortunate we should probably address this. However, I like that our code matches the specification fairly closely.
FYI, calling initEvent() on an event that has already been dispatched has been supported for a while (in the regular EventDispatcher code path). Firefox, Chrome, Safari and the specification all agree on this. Tested via: http://jsbin.com/mekobayoxu/edit?html,output
Comment on attachment 325163 [details] Patch Clearing flags on attachment: 325163 Committed r224125: <https://trac.webkit.org/changeset/224125>
All reviewed patches have been landed. Closing bug.
(In reply to Chris Dumez from comment #13) > FYI, calling initEvent() on an event that has already been dispatched has > been supported for a while (in the regular EventDispatcher code path). > > Firefox, Chrome, Safari and the specification all agree on this. Tested via: > http://jsbin.com/mekobayoxu/edit?html,output I however started a discussion upstream out possibly updating the specification: - https://github.com/whatwg/dom/issues/523 Feel free to pitch in.
I looked at this topic again and here are some additional inconsistencies I have noticed in our event handling code: IDBEventDispatcher::dispatch does not call resetPropagationFlags once it is done; I think it probably should. EventTarget::dispatchEvent does not call setCurrentTarget(null) once it is done; I think it probably should. I understand how appealing it is to have code that exactly corresponds to what the specification calls for in a direct way, but I do think it makes it easier to get these details wrong, doing some of the resetting, but not all of it.
(In reply to Darin Adler from comment #17) > I looked at this topic again and here are some additional inconsistencies I > have noticed in our event handling code: > > IDBEventDispatcher::dispatch does not call resetPropagationFlags once it > is done; I think it probably should. > EventTarget::dispatchEvent does not call setCurrentTarget(null) once it > is done; I think it probably should. > > I understand how appealing it is to have code that exactly corresponds to > what the specification calls for in a direct way, but I do think it makes it > easier to get these details wrong, doing some of the resetting, but not all > of it. I’m going to fix these.
(In reply to Darin Adler from comment #18) > (In reply to Darin Adler from comment #17) > > I looked at this topic again and here are some additional inconsistencies I > > have noticed in our event handling code: > > > > IDBEventDispatcher::dispatch does not call resetPropagationFlags once it > > is done; I think it probably should. > > EventTarget::dispatchEvent does not call setCurrentTarget(null) once it > > is done; I think it probably should. > > > > I understand how appealing it is to have code that exactly corresponds to > > what the specification calls for in a direct way, but I do think it makes it > > easier to get these details wrong, doing some of the resetting, but not all > > of it. > > I’m going to fix these. Thank you.
<rdar://problem/35567960>