Bug 178897 – DOMWindow::dispatchEvent() does not reset the event's dispatch flag

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Chris Dumez]

DOMWindow::dispatchEvent() does not reset the event's dispatch flag.

[Chris Dumez]

Created attachment 325118 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 325163 [details]
Patch

[Darin Adler]

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

[Chris Dumez]

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

[Chris Dumez]

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

[WebKit Commit Bot]

Comment on attachment 325163 [details]
Patch

Clearing flags on attachment: 325163

Committed r224125: <https://trac.webkit.org/changeset/224125>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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

[Darin Adler]

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.

[Darin Adler]

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

[Chris Dumez]

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

[Radar WebKit Bug Importer]

<rdar://problem/35567960>