Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix event path iteration #686

Merged
merged 2 commits into from
Mar 1, 2019
Merged

Fix event path iteration #686

merged 2 commits into from
Mar 1, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 6, 2018

Instead of shoehorning all target handling into the bubbling iteration, this separates "capturing" iteration from "bubbling" iteration and the Event object's phase is set to target as appropriate in both.

This also invokes the event listeners in a more natural order.

Tests: ...

Fixes #685.


Preview | Diff

@annevk annevk added topic: shadow Relates to shadow trees (as defined in DOM) needs tests Moving the issue forward requires someone to write tests interop Implementations are not interoperable with each other topic: events labels Sep 6, 2018
@annevk
Copy link
Member Author

annevk commented Sep 6, 2018

@whatwg/components I think this is what we want here, but careful review appreciated. If I did it correctly this does seem like a much nicer model.

<a for=Event/path>touch target list</a> is a <a for=/>node</a> and its <a for=tree>root</a> is a
<a for=/>shadow root</a>, and false otherwise.

<li><p>Set <var>event</var>'s {{Event/eventPhase}} attribute to {{Event/CAPTURING_PHASE}}.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell "legacy-pre-activation behavior" cannot trigger side effects so setting this later is not observable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it looks like we fire change event on radio buttons & checkboxes so the event phase change is observable. Gecko follows the current spec and WebKit & Blink uses the new behavior.

https://gist.github.com/rniwa/6c502dca3e16d5816db7958ce7bab4d7

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, we should add that test to the test suite. None seems like a reasonable phase to me given that traversal hasn't started yet, WDYT @smaug----?

Copy link
Collaborator

@rniwa rniwa Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that none seems more reasonable than capturing given the event dispatching hasn't even started yet.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From API user's point of view Gecko's behavior makes more sense, since for them the event dispatch clearly has started already once dispatchEvent is called.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. Just because we're in dispatchEvent, doesn't mean eventPhase needs to be something other than "none".

@rniwa
Copy link
Collaborator

rniwa commented Sep 6, 2018

The change looks sensible to me.

As pointed out in #685 this was a bit confusing.
@annevk annevk force-pushed the annevk/listener-calling-order branch from 30085f8 to 39b97c3 Compare September 11, 2018 15:47
kisg pushed a commit to paul99/webkit-mips that referenced this pull request Sep 14, 2018
… hosts

https://bugs.webkit.org/show_bug.cgi?id=174288
LayoutTests/imported/w3c:


Reviewed by Darin Adler.

* web-platform-tests/dom/events/Event-dispatch-handlers-changed-expected.txt: Rebaselined. This test's
expectation is now wrong because event listner 3 is added after the event listener list is cloned for
capturing event listeners but before cloned for bubbling event listeners. As a result, event listener 3
is now invoked. It used to be not called because both bubbling and capturing event listeners are called
after cloning the event listner list once, which didn't have event listener 3.

* web-platform-tests/dom/events/EventTarget-dispatchEvent-expected.txt: Rebaselined. This test expects
event listener 2, which is bubbling, to be called between two capturing event listeners 1 and 3, which
is no longer true after this patch.

Source/WebCore:

<rdar://problem/33530455>

Reviewed by Darin Adler.

Implemented the new behavior proposed in whatwg/dom#686 [1] to fix the problem
that capturing event listeners on a shadow host is invoked during bubbling phase when an event is
dispatched within its shadow tree.

To see why this is a problem, suppose we fire a composed event at span#target in the following DOM tree:
  section#hostParent
    + div#host -- ShadowRoot
                    - p#parent
                        - span#target
Then capturing and bubbling event listeners on #target, #parent, #host, and #hostParent are invoked in
the following order in WebKit & Chrome right now:

1. #hostParent, capturing, eventPhase: CAPTURING_PHASE
2. #parent, capturing, eventPhase: CAPTURING_PHASE
3. #target, capturing, eventPhase: AT_TARGET
4. #target, non-capturing, eventPhase: AT_TARGET
5. #parent, non-capturing, eventPhase: BUBBLING_PHASE
6. #host, capturing, eventPhase: AT_TARGET
7. #host, non-capturing, eventPhase: AT_TARGET
8. #hostParent, non-capturing, eventPhase: BUBBLING_PHASE

This is counter-intuitive because capturing event listeners on #host isn't invoked until bubblign phase
started. A more natural ordering would be:

1. #hostParent, capturing, eventPhase: CAPTURING_PHASE
2. #host, capturing, eventPhase: AT_TARGET
3. #parent, capturing, eventPhase: CAPTURING_PHASE
4. #target, capturing, eventPhase: AT_TARGET
5. #target, non-capturing, eventPhase: AT_TARGET
6. #parent, non-capturing, eventPhase: BUBBLING_PHASE
7. #host, non-capturing, eventPhase: AT_TARGET
8. #hostParent, non-capturing, eventPhase: BUBBLING_PHASE

This also happens to be the order by which Gecko's current shadow DOM implementation invoke event listners.
This patch implements this new behavior using the spec-change proposed in [1]. Note that this patch also
impacts the invocation order of event listeners when there is no shadow tree. Namely, before this patch,
event listeners on the event's target is invoked in the registration order. After this patch, all capturing
event listeners are invoked before bubbling event listeners are invoked.

To implement this behavior, this patch introduces EventTarget::EventInvokePhase indicating whether we're
in the capturing phase or bubbling phase to EventTarget::fireEventListeners. We can't use Event's eventPhase
enum because that's set to Event::Phase::AT_TARGET when we're at a shadow host.

Test: fast/shadow-dom/capturing-and-bubbling-event-listeners-across-shadow-trees.html

* Modules/modern-media-controls/media/media-controller-support.js:
(MediaControllerSupport.prototype.enable): Use capturing event listeners so that we can update the states of
media controls before author scripts recieve the event.
(MediaControllerSupport.prototype.disable): Ditto.
* dom/EventContext.cpp:
(WebCore::EventContext::handleLocalEvents const):
(WebCore::MouseOrFocusEventContext::handleLocalEvents const):
(WebCore::TouchEventContext::handleLocalEvents const):
* dom/EventContext.h:
* dom/EventDispatcher.cpp:
(WebCore::dispatchEventInDOM): Invoke capturing event listners even when target and current target are same.
This happens when the current target is a shadow host and event's target is in its shadow tree. Also merged
the special code path for the event's target with the code in the bubbling phase.
* dom/EventPath.cpp:
(WebCore::WindowEventContext::handleLocalEvents const):
* dom/EventTarget.cpp:
(WebCore::EventTarget::dispatchEvent): Invoke capturing and bubbling event listeners in the order.
(WebCore::EventTarget::fireEventListeners):
(WebCore::EventTarget::innerInvokeEventListeners): Renamed from fireEventListeners to match the spec. Use
EventInvokePhase to filter out event listeners so that we can invoke capturing event listners before bubbling
event listeners even when eventPhase is Event::Phase::AT_TARGET.
* dom/EventTarget.h:
* dom/Node.cpp:
(WebCore::Node::handleLocalEvents):
* dom/Node.h:
* html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::handleLocalEvents):
* html/HTMLFormElement.h:
* page/DOMWindow.cpp:
(WebCore::DOMWindow::dispatchEvent):

LayoutTests:


Reviewed by Darin Adler.

Added a W3C style testharness.js test and rebaselined two tests. See below for rationals of rebaselines.

* fast/shadow-dom/capturing-and-bubbling-event-listeners-across-shadow-trees-expected.txt: Added.
* fast/shadow-dom/capturing-and-bubbling-event-listeners-across-shadow-trees.html: Added.

* media/media-load-event-expected.txt: Rebaselined. The logging of oncanplaythrough event is now happening
before canplaythrough() is called because the logging is done by waitForEvent which uses a capturing event
listener whereas canplaythrough is called by a event handler, which is non-capturing.

* platform/ios-11/imported/w3c/web-platform-tests/dom/events/EventTarget-dispatchEvent-expected.txt:
* platform/ios/imported/w3c/web-platform-tests/dom/events/EventTarget-dispatchEvent-expected.txt:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@236002 268f45cc-cd09-0410-ab3c-d52691b4dbfc
annevk added a commit to web-platform-tests/wpt that referenced this pull request Sep 17, 2018
annevk added a commit to web-platform-tests/wpt that referenced this pull request Sep 17, 2018
annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 1, 2019
@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label Mar 1, 2019
annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 1, 2019
@annevk annevk force-pushed the annevk/listener-calling-order branch from 4e0c64d to 794e934 Compare March 1, 2019 10:48
Instead of shoehorning all target handling into the bubbling iteration, this separates "capturing" iteration from "bubbling" iteration and the Event object's phase is set to target as appropriate in both.

This also invokes the event listeners in a more natural order.

Tests: web-platform-tests/wpt#13030 & web-platform-tests/wpt#13031.

Fixes #685.
@annevk annevk force-pushed the annevk/listener-calling-order branch from 794e934 to 7aaff50 Compare March 1, 2019 10:49
annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 1, 2019
@annevk annevk merged commit 9943801 into master Mar 1, 2019
@annevk annevk deleted the annevk/listener-calling-order branch March 1, 2019 10:53
annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 2, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 1, 2019
…estonly

Automatic update from web-platform-tests
DOM: test listener invocation order

For whatwg/dom#686.

--

wpt-commits: 103df1bf782674889deb7136cdf7b1318dadb86b
wpt-pr: 13031
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 1, 2019
…tion behavior, a=testonly

Automatic update from web-platform-tests
DOM: eventPhase during legacy-pre-activation behavior

For whatwg/dom#686.
--

wpt-commits: 079928e6d608251333281d1a934f50d01ffeb901
wpt-pr: 13030
This was referenced Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: events topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging this pull request may close these issues.

3 participants