whatwg / dom

DOM Standard
https://dom.spec.whatwg.org/
Other
1.58k stars 295 forks source link

Allow custom "get the parent" algorithms for EventTargets #583

Open zandaqo opened 6 years ago

zandaqo commented 6 years ago

In the future we could allow custom get the parent algorithms. Let us know if this would be useful for your programs. For now, all author-created EventTargets do not participate in a tree structure.

I'd like to see that future. That is, the ability to create a tree structure for author-created EventTargets like the one we have with Nodes, with bubbling and capturing, and so forth. Two points in support of this:

  1. It would just make the platform more consistent when all EventTargets behave similar or have ways to do so. Right now we have a "weird" limitation to keep in mind when we work with custom EventTargets.
  2. Listening just to a parent instead of each of its children simplifies our code when we work with Nodes, and same can apply to author-created EventTargets.

An example use case: Let's take the classic Todo app example. Imagine that each todo item is a custom EventTarget emitting "change" events, and the whole list is an array. Now, if we need to respond to any change in the list, say, recalculate the number of items, we have to add event listeners to each item. However, if we could define our array as the parent for each item where events could bubble to (or be captured on) the parent, we could get away with just one listener on the array. Since the items and the array already have strong coupling, adding an extra "parent" pointer wouldn't make difference in terms of the design.

annevk commented 6 years ago

To what extent are you interested in also having shadow tree capabilities for your custom event targets? E.g., being able to hide event targets from composedPath() and such. (This came up in #684.)

zandaqo commented 6 years ago

@annevk I cannot think of a use case for shadow tree capabilities. If we are building tree structures out of these custom EventTargets solely to propagate events, having a capability of hiding/skipping certain nodes seems superfluous--one can just avoid putting those hidden nodes into the said tree structure.

aayushch commented 5 years ago

We are building web extensions which would like to implement events, to propagate critical occurrences from the browser api event listeners back to the extension business logic layer.

We would want to leverage the EventTarget implementation to build propagation, rather doing it in an ad-hoc way.

The ability to bubble the events is going to be a huge advantage given we can override/implement a custom 'get the parent' algorithm. The shadow tree capabilities are not really necessary for author created event targets.

clshortfuse commented 4 years ago

I'm currently using a polyfill for EventTarget in NodeJS environments. I see Deno will support EventTarget out of the box, so at least we're going to get nice usage from EventTarget.

But, for my polyfill, I'm creating a custom "get the parent" implementation via WeakMap as to not pollute the the property list.

/**
 * @typedef {Object} EventDispatcherEventTargetPrivateFields
 * @prop {Map<string, Listener[]>} listeners
 * @prop {function():EventDispatcherEventTarget} getParent
 */

/** @type {WeakMap<any, EventDispatcherEventTargetPrivateFields>} */
export const privateEventTargetFields = new WeakMap();

export default class EventDispatcherEventTarget {
  constructor() {
    privateEventTargetFields.set(this, {
      listeners: new Map(),
      getParent: () => null,
    });
  }

  /* ... */

getParent() gets checked and called during dispatchEvent(), much like how browsers do it. The problem is my custom implementations has to know to set privateEventTargetFields.get(eventTarget).getParent = callback;

The reason is I'm using a context => listener => client configuration for socket connections. Client connections themselves emit events which can be captured by the listener (TCP or WebSocket). Propagation can be stopped or it can continue bubble to the context (service that spawns the listeners).

It makes sense because the application architecture is tree-like already. Unfortunately, if we decide to port to Deno we will not be able to the official EventTarget implementation because spec has no "get the parent" function and no standard as to how to pass the "get the parent" function in the constructor.

Edit: I just want to add that this continues to make more sense considering we can do new EventTarget() now which I use for custom events that has nothing to do with the DOM. Also, some objects (eg: service workers, window, navigator) extend EventTarget but have little to no relation to the DOM (and workers can't even access the DOM), so I would say Events has branched outside the basic DOM scope already.

jchayan commented 3 years ago

+1 for this... In my case, it's just a whim. I'm currently writing a tiny widget's library where widgets are tree nodes (they have a parent and children), so I wanted to extend EventTarget and add event bubbling support, I stumbled upon this because I tried overriding dispatchEvent, so that:

class ElementNode extends EventTarget {
  constructor () {
    super();
    this.parent   = null;
    this.children = [ ];
  }

  append (nodes) {...}
  remove (node) { ... }
  walk (visitor) { ... }
  dispatchEvent (event) {
    super.dispatchEvent(event);
    if (event.bubbles && this.parent) {
      this.parent.dispatchEvent(event); // Losing event.target here :(
    }
  }
}

The original event.target target is lost across calls because I'm manually dispatching the event in the parent, which then sets the target to be the parent. After reading the spec (https://dom.spec.whatwg.org/#concept-event-dispatch) I realized that the get the parent function can't be overridden and besides I can't just reassign event.target as it's read-only (and I understand why)

If I were to think of the benefits I'd say that we would gain standardization... Allowing this customization can open the doors for other libraries or frameworks to rely on the EventTarget and CustomEvent APIs instead of everyone rolling their own "event emitter" solutions.

clshortfuse commented 3 years ago

Deno actually uses a somewhat custom method of "get the parent" that allows custom overloading. The check occurs by checking if the EventTarget "is a node" by checking if has a nodeType property. Then it uses .parentNode. That means an Javascript Object that has nodeType can potentially return a parent (via .parentNode).

https://github.com/denoland/deno/blob/704e1e53307130012da974f42d2cad94e07b5664/op_crates/web/02_event.js#L438

This is piggybacking off Node, but to write a proper shim for it (I've tried it), you have to implement Node, ChildNode, ParentNode, Document. and some others that my escape me at the moment. That would be a lot of work for user agents to include and force them to require support something far more complicated just EventTarget. The Deno team is using something custom as is for lack of a standard, and I can totally understand doing it like that instead of writing all of Document.

I can only realistically conclude that the EventTarget may be constructed with a eventTargetInit object that may have a get the parent value. So you can maybe do something like new EventTarget({ getParent: myMasterParentFn }), for example. It keeps it strictly within EventTarget without telling user agents to support more complex types, or having developers need to add on something heavy like JSDom.

rniwa commented 3 years ago

Hm... we don't want to run arbitrary scripts just to get the parent. It needs to be some kind of setter/getter that UA would implement on EventTarget interface.

jchayan commented 3 years ago

@rniwa Do you think that having that setter/getter is a simple thing to do? And if so, what determines if that should be added to the spec?

To be honest, I'm just curious

rniwa commented 3 years ago

@rniwa Do you think that having that setter/getter is a simple thing to do? And if so, what determines if that should be added to the spec?

Basically, you need to get at least two implementors interested in your proposal, write a DOM/HTML spec PR and WPT tests.

clshortfuse commented 3 years ago

My concerns with a getter/setter is a hard reference could leave an object in memory, prohibiting it from being garbage collection. With DOM nodes, once a child is detached from its parent, .parentNode becomes null (or undefined, not sure). The parent can then be garbage collected if it's no longer referenced. That's one of the reasons why I leaned to a function.

The other idea I had with my custom implementation is a EventTarget parent registry. Basically, instead of the child explicitly stating what it's parent is, a global registry can be used. eg: parent = EventTargetParentRegistry.get(child). Here, a user agent can basically keep a dictionary of sorts since no child can have multiple parents.

A pure JavaScript solution (polyfill) could look something like this:

class EventTargetParentRegistry {
  /** @type {WeakMap<EventTarget, WeakRef<EventTarget>>} */
  static #registry = new WeakMap();

  static get(child) {
    return this.#registry.get(child)?.deref();
  }

  static set(parent, child) {
    this.#registry.set(child, new WeakRef(parent));
  }

  static delete(child) {
    this.#registry.delete(child);
  }
}

During the get the parent phase, the user agent (or polyfill) would use EventTargetParentRegistry.get(child) for EventTargets that are not Node objects.

rniwa commented 3 years ago

My concerns with a getter/setter is a hard reference could leave an object in memory, prohibiting it from being garbage collection. With DOM nodes, once a child is detached from its parent, .parentNode becomes null (or undefined, not sure). The parent can then be garbage collected if it's no longer referenced. That's one of the reasons why I leaned to a function.

I'm not sure how that is consistent with the idea that EventTarget can have a parent. Is the idea that the event will propagate to the parent object only if that's still alive?? Literally nothing in the web platform works like that. I'm not certain we want to add API like this given its behavior will be highly dependent on a particular implementation of GC.

keithamus commented 1 year ago

In https://github.com/whatwg/dom/issues/1146 I suggested a method that could be available on EventTarget:

It would be great to introduce some kind of mechanism for EventTargets to associate with one another to allow for event bubbling and capturing. Perhaps EventTarget could have something like associateAncestor(ancestorEventTarget, { capture: true, bubble: true }) which could do the necessary internal wiring to make an event targets event capture & bubble upward to an ancestor? Existing EventTargets could throw when this is called, but it would allow for designs in userland to make use of a fairly large feature within EventTarget.

Having a method on the child event target allows it to perform necessary cleanup if called multiple times, and allows for customisation of the different phases. For example I can imagine some implementations opting out of the capturing phase and only allowing bubbling to occur.

I am not precious about the implementation, but I'm happy to put the work in provided implementer interest. @rniwa is your commentary on this issue an indication of interest? Perhaps @mfreed7 or @domenic could also provide some insight on to interest within Chrome?

smaug---- commented 1 year ago

It would help implementers (at least Mozilla) if there were a bit more concrete proposal here.

It sounds like the use cases require effectively a DOM node tree, without the objects being actual nodes, but some other types of EventTargets. And one should be able to modify the tree using similar concepts as DOM tree. And when dealing with mutations, the operations need to ensure one doesn't cause any cycles.

Minimal API from Event handling point of view would be something like setParentEventTarget(EventTarget? parent). That should throw if 'this' is some other native EventTarget than EventTarget itself, or if parent is found in the parent chain or parent is 'this'. Calling setParentEventTarget(null) would be left to the user of the API - like the rest of the possible tree operations. And event target path doesn't care about bubble or capture. The path is path, and how an event propagates through it depends on the properties of the event itself.

keithamus commented 1 year ago

An event only has a bubble flag which determines if it executes the bubbling & capturing phases. Capturing & bubbling phases are not both always desirable in a tree, so it would be useful to be able to turn one or the other off without huge workarounds.

clshortfuse commented 1 year ago

About two years later, I better understand @rniwa 's point about GC complexity.

A class that extends EventTarget can have a getter setter related to getting the parent. The Node (class) implementation can return Node.prototype.parentNode. That allows our (authors') extensions of EventTarget to handle the tree ourselves along with removal and concerns about GC (I'd probably use WeakRef internally). Implementers (Apple/FireFox coders) just have to invoke the getter and that's it.

If that's the right idea, let me know and I can write up a proposal draft. Something like

class MyCustomEventTarget extends EventTarget {

  /** @override */
  get parentEventTarget() {
    return AUTHOR_EVENTTARGET_WEAKMAP.get(this);
  }
}

IIRC, bubbling and capture are part of composedPath which shouldn't need any extra code other than a "get the parent" function/getter.

annevk commented 1 year ago

@keithamus events always perform the capturing phase. Making that optional at dispatch somehow would be a distinct feature request we should track separately.

@clshortfuse I don't think user agents should have to invoke JavaScript to traverse the tree. As @smaug---- suggests it should be possible for the user agent to have access to the entire path and impose constraints on it.

LeaVerou commented 1 year ago

Came here to file this exact same issue after naively trying to just set parentNode, hoping it may Just Work™ (narrator: It didn’t). It would be quite useful for authors to be able to reflect hierarchical relationships between data and have events propagate.

keithamus commented 1 year ago

How about overloading the constructor of an EventTarget to allow assignment of a parent EventTarget?

[Exposed=*]
interface EventTarget {
  constructor(parent?: EventTarget);

  // ...
}

If the parent is undefined or null then the EventTarget is considered to have no parent, otherwise assert that parent is EventTarget. For Node, ShadowRoot and Document, the constructor remains as-is and they continue to use their custom get the parent algos.

annevk commented 1 year ago

I think that is probably a workable API (no need for null though, optional parent argument or dictionary with optional parent member seems fine), but we will also need to add loop detection in https://dom.spec.whatwg.org/#concept-event-dispatch. Perhaps that should lead to an exception? I think you can only ever reach it from dispatchEvent() so that should be okay.

keithamus commented 1 year ago

Would something like https://github.com/whatwg/dom/commit/c88f115844a9ce4026ae7afe9ee945af044e9a82 suffice? Should I raise this and work on some WPT/implementations?

annevk commented 1 year ago

That's a start, but it'll need some more work to formalize it properly. (In particular you probably want to define some kind of internal field that holds the parent and that "get the parent" ends up returning. And the IDL syntax you're looking for is optional EventTarget parent.)

clshortfuse commented 1 year ago

Do we want EventTargetInit in case we want more options in the future? Or would being an instance of EventTarget be enough to create an init object later down the road, if ever?

@keithamus Thanks for working on this.

I used init before and used a function instead, which is where we got to the garbage collection talk. We should be able to orphan and move those event targets, which means nulling and setting that value later during runtime. For my arbiter=>server=>client setup, orphaning is common.

keithamus commented 1 year ago

An init object could make sense. My understanding is that GC can dispose of the init object while retaining a reference to the parent, provided it is in the constructor. Getters can be retrieved during construction to capture the parent in an internal field.

LeaVerou commented 1 year ago

What about enabling parentNode to just work instead of adding more config? It should be fairly easy to investigate if this would be web compatible, but since bubbling is opt-in, I suspect it could be since there's no point in dispatching events that bubble on non-DOM nodes today, so even if objects have a parentNode property, it wouldn't do anything special.

This also means that we could even eventually port other DOM concepts to arbitrary objects with a tree-like structure (e.g. things like append() etc).

For objects that use another property to point to the parent object (e.g. parent), all this needs is

get parentNode() {
    return this.parent;
}

set parentNode (node) {
    this.parent = node;
}

Benefits:

Drawbacks:

Note that there is ample precedent around special property or method names doing special things rather than specifying properties in some init object, especally in Custom Elements.

Another idea to avoid additional config could be to insert another subclass between EventTarget and Node, e.g. AbstractNode, for objects that have parentNode and other tree methods, but are not DOM nodes.

That said, if any of this is entirely out of the question, it should definitely be an init object instead of a positional argument, see TAG principle 6.4 Accept optional and/or primitive arguments through dictionaries

clshortfuse commented 1 year ago

@LeaVerou What you suggest is how Deno already works. The code complexity around polyfilling Node is a quite a bit. I've tried it and ended up just wrapping EventTarget instead.

This is piggybacking off Node, but to write a proper shim for it (I've tried it), you have to implement Node, ChildNode, ParentNode, Document. and some others that my escape me at the moment. That would be a lot of work for user agents to include and force them to require support something far more complicated just EventTarget

https://github.com/whatwg/dom/issues/583#issuecomment-814495296

Looking at my code, I have a bastardization of Node as a shim for the tree, without all the document stuff.

Subclassing between EventTarget and Node is probably better, though we'd have to define what goes in and what doesn't (eg: do we need to include insertBefore).

The only "concern" I can see about passing an EventTarget instead (new EventTarget(parentEventTarget)) of an init object is that EventTarget instances can be overloaded with their own properties, and I would want to avoid the possibilty of that custom EventTarget object later conflicting with an EventTargetInit property. For example:

class FooEventTarget extends EventTarget {
  bubbles = false;
}

Then if we make an EventTargetInit later with {bubbles: true, parent: foo} , it would be unclear if they're passing the parent or an EventTarget instance that has a property when doing new EventTarget(new FooEventTarget()).

domenic commented 1 year ago

Note that the design proposed here, of requiring an EventTarget parent object instead of a "function that returns an EventTarget" get-the-parent algorithm, means that this will only be usable when one is constructing one's tree of EventTargets in a top-down manner, where parents exist before children.

+1 to not conflating parentNode (part of the public Node interface) and "get the parent" (part of the EventTarget internal implementation).

annevk commented 1 year ago

@LeaVerou the way custom elements handle this is very specific though to avoid crossing the C++/JS bridge too many times. Having to run a lot of script while building up an event's path isn't acceptable.

It does seem like there are a number of use cases where you want to be able to manipulate the parent post-construction. That argues for some kind of revealing constructor pattern or something similar to ElementInternals, e.g.,

[Exposed=*]
interface EventTargetInternals {
  attribute EventTarget parent;
};

callback EventTargetCallback = undefined (EventTargetInternals internals);

partial interface EventTarget {
  constructor(optional EventTargetCallback cb);
};
smaug---- commented 1 year ago

I don't understand the comments about mixing bubbles and EventTarget. Bubbling is a property of the Event, not EventTarget (and it just tell that the event dispatch does only capturing and target phases).

And however we get the parent object, it needs to be very fast. Native implementation must not be required to touch any JS at that point (event dispatch is extremely performance critical).

keithamus commented 1 year ago

Tried to write up a spec around @annevk's IDL sketch of an EventTargetInternals: https://github.com/whatwg/dom/pull/1230/files. I'll noodle around with an implementation in the coming weeks.