Bug 160853 – WebKit should unset event propagation flags after dispatch

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

WebKit should unset event propagation flags after dispatch to reflect the latest DOM specification:
- https://github.com/whatwg/dom/commit/806d4aab584f6fc38c21f8e088b51b8ba3e27e20

[Chris Dumez]

Created attachment 286132 [details]
WIP patch

Looks like some web-platform-tests need to be fixed.

[Chris Dumez]

(In reply to comment #1)
> Created attachment 286132 [details]
> WIP patch
> 
> Looks like some web-platform-tests need to be fixed.

https://github.com/w3c/web-platform-tests/pull/3469

[Chris Dumez]

Created attachment 286431 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 286431 [details]
Patch

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

> Source/WebCore/dom/EventDispatcher.cpp:190
> -    event.setEventPhase(0);
> +    event.resetPropagationFlags();
> +    event.setEventPhase(Event::NONE);

Should we combine these two functions? e.g. resetEventPhaseAndPropagationFlags?

[Chris Dumez]

Comment on attachment 286431 [details]
Patch

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

>> Source/WebCore/dom/EventDispatcher.cpp:190
>> +    event.setEventPhase(Event::NONE);
> 
> Should we combine these two functions? e.g. resetEventPhaseAndPropagationFlags?

I don't know if it is worth it. We anyway need to keep the setEventPhase() setter.

[Ryosuke Niwa]

(In reply to comment #5)
> Comment on attachment 286431 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286431&action=review
> 
> >> Source/WebCore/dom/EventDispatcher.cpp:190
> >> +    event.setEventPhase(Event::NONE);
> > 
> > Should we combine these two functions? e.g. resetEventPhaseAndPropagationFlags?
> 
> I don't know if it is worth it. We anyway need to keep the setEventPhase()
> setter.

But whenever we call resetPropagationFlags, we should be also resetting the phase, right?

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 286431 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=286431&action=review
> > 
> > >> Source/WebCore/dom/EventDispatcher.cpp:190
> > >> +    event.setEventPhase(Event::NONE);
> > > 
> > > Should we combine these two functions? e.g. resetEventPhaseAndPropagationFlags?
> > 
> > I don't know if it is worth it. We anyway need to keep the setEventPhase()
> > setter.
> 
> But whenever we call resetPropagationFlags, we should be also resetting the
> phase, right?

Yes, among other things, like clearing the event path and the currentTarget. Yet I don't feel they are strictly related.

From the spec:
[...]
- 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.
- Set event’s path to the empty list.

[Ryosuke Niwa]

FWIW, I don’t feel strongly about so you can go ahead & land it. It was just a suggestion.

[WebKit Commit Bot]

Comment on attachment 286431 [details]
Patch

Rejecting attachment 286431 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 286431, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
org/repository/webkit/trunk ...
RA layer request failed: Unable to connect to a repository at URL 'http://svn.webkit.org/repository/webkit/trunk': OPTIONS of 'http://svn.webkit.org/repository/webkit/trunk': could not connect to server (http://svn.webkit.org) at /Applications/Xcode.app/Contents/Developer/usr/libexec/git-core/git-svn line 1032.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/1896706

[Chris Dumez]

Comment on attachment 286431 [details]
Patch

Clearing flags on attachment: 286431

Committed r204630: <http://trac.webkit.org/changeset/204630>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.