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

Composed and non bubbling event semantic dispatched in shadow tree #742

Closed
pmdartus opened this issue Mar 26, 2019 · 10 comments · Fixed by #750
Closed

Composed and non bubbling event semantic dispatched in shadow tree #742

pmdartus opened this issue Mar 26, 2019 · 10 comments · Fixed by #750
Labels
topic: events topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@pmdartus
Copy link
Member

What is the expected behavior for a composed and non-bubbling event when dispatched inside a shadow tree?

When an event listener is attached to the host element, and a composed and non-bubbling event is dispatched within a shadow tree, the listener attached on the host element is invoked. Here is an example: https://jsfiddle.net/pmdartus/6ft9gkc4/7/

This behavior is consistent on all the browsers implementing shadow DOM.

As far as I understand the recent changes made to the dispatch event algorithm (#686), the behavior changed and now the event will only invoke the host element event handler if the event bubbles (step 14). Is it the new expected behavior, or did I missed something?


Regardless of the outcome of this question, I think it worth adding more details to the non-normative section about composed.

@annevk annevk added topic: shadow Relates to shadow trees (as defined in DOM) topic: events labels Mar 26, 2019
@annevk
Copy link
Member

annevk commented Mar 26, 2019

So the problem is that we created events that only have a capture phase from the view of certain trees.

I think what's needed here is these changes:

  1. Step 5.14 of https://dom.spec.whatwg.org/#concept-event-dispatch should not be conditional upon "If event’s bubbles attribute is true".
  2. Step 5.14.2 should be conditional upon "If event’s bubbles attribute is true" and use continue there to go to the next struct.

This would make step 14 read:

For each struct in event’s path:

  1. If struct’s target is non-null, then set event’s eventPhase attribute to AT_TARGET.
  2. Otherwise:
    1. If event’s bubbles attribute is false, then continue.
    2. Set event’s eventPhase attribute to BUBBLING_PHASE.
  3. Invoke with struct, event, "bubbling", and legacyOutputDidListenersThrowFlag if given.

cc @whatwg/components

(As for composed, it affects building the event's path and that's it. It seems like you mostly didn't observe the event because you didn't have capture listeners.)

@pmdartus
Copy link
Member Author

Those changes look reasonable to me. Thanks for the details @annevk.

@caridy
Copy link

caridy commented Mar 26, 2019

As @pmdartus mentioned, the proposed changes look good, but I will like to get some clarification on why the current behavior was chosen in the first place. We discovered the issue described here while trying to test our synthetic shadow dom polyfill, and we were surprised by the behavior of all browsers. Why all composed events dispatched on the host elements in the event path regardless if the event bubbles or not? Seems counterintuitive, and leak internal implementation details of the component.

@annevk
Copy link
Member

annevk commented Mar 26, 2019

@caridy events have four phases: none, capturing, at target, and bubbling. Capturing and at target are always performed. If we did not perform at target for a composed event, it would actually leak that there was a component as the target for an event we could observe during capturing was not in our tree. (I realize this is the opposite of what you suggest, but it's unclear to me how you're arriving at your conclusion.)

@rniwa
Copy link
Collaborator

rniwa commented Mar 26, 2019

Capturing and at target are always performed.

Maybe we need capturing flag in EventInit which disables capturing phase as well. It is fundamentally weird that you can't dispatch an event within a component without it being observable outside of it since that leaks the implementation details of a component.

@annevk
Copy link
Member

annevk commented Mar 27, 2019

@rniwa that is what composed is for. By default an event's path doesn't cross the shadow boundary (except from something slotted into a shadow tree, but that's different).

@rniwa
Copy link
Collaborator

rniwa commented Mar 27, 2019

@annevk : oh, that's a good point. If the composed is set to false, we wouldn't have this issue.

@caridy
Copy link

caridy commented Mar 27, 2019

here is a very concrete example of the problem:

<div>
    #shadowRoot 
       <div>
            <x-foo>

If component <x-foo> dispatches an custom event with { composed: true, bubbles: false } via this.dispatchEvent(), it is extremely confusing that the event is observable from the outer div, but it is not observable by the inner div. From the teachability point of view, how do you teach that? It breaks all intuitions as far as I can tell.

From the technical point of view, the leaking aspect of it is pretty bad because the only way you have to prevent an event from leaking is by listening for it at the host level (during construction time), and stop immediate propagation, and you have to do so for all events dispatched by any of your child components in order to control the leakage, and still, there is a possibility that the host was upgraded after a listener for the event was added by someone else, in which case, you have no ways to prevent the leakage.

@annevk
Copy link
Member

annevk commented Mar 27, 2019

@caridy if you use addEventListener(yourEventNameHere, yourHandlerHere, true) on the inner <div> it is observable there too. I'd like to refer back to my comment about event phases.

annevk added a commit that referenced this issue Apr 10, 2019
This fixes a regression from #686.

Tests: various tests were failing because of this. web-platform-tests/wpt#16307 aligns the remaining failing test with the new model.

Fixes #742.
@annevk
Copy link
Member

annevk commented Apr 10, 2019

Fix at #750.

annevk added a commit that referenced this issue Apr 15, 2019
This fixes a regression from #686.

Tests: various tests were failing because of this. web-platform-tests/wpt#16307 aligns the remaining failing test with the new model.

Fixes #742.
rniwa added a commit to rniwa/web-platform-tests that referenced this issue Apr 15, 2019
annevk pushed a commit to web-platform-tests/wpt that referenced this issue Apr 16, 2019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 5, 2019
…ac.webkit.org/r236002 for whatwg/dom#742, a=testonly

Automatic update from web-platform-tests
DOM: more shadow tree event dispatching tests

WebKit test added in https://trac.webkit.org/r236002.

Helps with whatwg/dom#742.
--

wpt-commits: 151e1e4165dab9d279252be5b2ca64c39b9a4a56
wpt-pr: 16358
mykmelez pushed a commit to mykmelez/gecko that referenced this issue Jun 6, 2019
…ac.webkit.org/r236002 for whatwg/dom#742, a=testonly

Automatic update from web-platform-tests
DOM: more shadow tree event dispatching tests

WebKit test added in https://trac.webkit.org/r236002.

Helps with whatwg/dom#742.
--

wpt-commits: 151e1e4165dab9d279252be5b2ca64c39b9a4a56
wpt-pr: 16358
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…ac.webkit.org/r236002 for whatwg/dom#742, a=testonly

Automatic update from web-platform-tests
DOM: more shadow tree event dispatching tests

WebKit test added in https://trac.webkit.org/r236002.

Helps with whatwg/dom#742.
--

wpt-commits: 151e1e4165dab9d279252be5b2ca64c39b9a4a56
wpt-pr: 16358

UltraBlame original commit: 39ff6aa7f78129bda90cf2ee5a273233f2c666e2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…ac.webkit.org/r236002 for whatwg/dom#742, a=testonly

Automatic update from web-platform-tests
DOM: more shadow tree event dispatching tests

WebKit test added in https://trac.webkit.org/r236002.

Helps with whatwg/dom#742.
--

wpt-commits: 151e1e4165dab9d279252be5b2ca64c39b9a4a56
wpt-pr: 16358

UltraBlame original commit: 39ff6aa7f78129bda90cf2ee5a273233f2c666e2
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…ac.webkit.org/r236002 for whatwg/dom#742, a=testonly

Automatic update from web-platform-tests
DOM: more shadow tree event dispatching tests

WebKit test added in https://trac.webkit.org/r236002.

Helps with whatwg/dom#742.
--

wpt-commits: 151e1e4165dab9d279252be5b2ca64c39b9a4a56
wpt-pr: 16358

UltraBlame original commit: 39ff6aa7f78129bda90cf2ee5a273233f2c666e2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: events topic: shadow Relates to shadow trees (as defined in DOM)
Development

Successfully merging a pull request may close this issue.

4 participants