Polymer / polymer

Our original Web Component library.
https://polymer-library.polymer-project.org/
BSD 3-Clause "New" or "Revised" License
22.04k stars 2.01k forks source link

Wrap nodes before calling {add,remove}EventListener. #5663

Closed bicknellr closed 4 years ago

bicknellr commented 4 years ago

This PR is to prepare for webcomponents/polyfills#332, which will manually dispatch all events with targets that are disconnected in the real tree, even if they are connected in the user-facing tree. Before this change, all events (except focus and blur) were dispatched natively, which meant that events dispatched to nodes that were disconnected in the real tree but connected in the user-facing tree would incorrectly propagate only through the disconnected piece of the tree containing the node. Dispatching these events manually in Shady DOM fixes this issue, but only if it's aware all of the listeners it needs to call, which requires wrapping with ShadyDOM.wrapIfNeeded before calling addEventListener/removeEventListener.

These particular changes were needed to fix some failures that showed up in internal testing of webcomponents/polyfills#332, but there are other places in Polymer that use addEventListener/removeEventListener without wrapping first. AFAIK, these weren't wrapped originally as a perf optimization and I think that some of them could stay that way if necessary, but I'm not sure about all of them. Particularly:

https://github.com/Polymer/polymer/blob/master/lib/utils/flattened-nodes-observer.js#L286-L312

This one seems relatively likely to need to be wrapped given that slotchange bubbles.

https://github.com/Polymer/polymer/blob/master/lib/utils/gestures.js

The addEventListener on window is only used for testing if passive is supported, so that doesn't need to be wrapped, but addEventListener is also called on document and other nodes in general to listen for mouse events. I don't think it's likely that these need to be wrapped because the user can't interact with disconnected nodes.

bicknellr commented 4 years ago

I think dispatchEvent also requires wrap and there are a few places in Polymer that don't wrap things they're dispatching events to, should I update those here?