Should commands receive an event that triggered them?

krassowski commented 1 year ago

Problem

A number of commands, e.g. in jupterlab extensions (especially editor integrations such as spellchecker or LSP), need to know the node they were triggered on. Currently JupyterLab implements a custom caching logic that holds the most recent pointer event that triggered the context menu forever (until another context menu gets opened). This leads to a memory leak.

Proposed Solution

We could reserve a special argument in the args of commands for the event the command was triggered by (if any). Since most events contain a target property this would solve a major headache of extension developers.

This would mean that calls to CommandRegistry.execute would need to be adjusted to pass the event on. Conceptually this could be something like:

    // Close the root menu before executing the command.
    this.rootMenu.close();

    // Execute the command for the item.
-    let { command, args } = item;
+    let { command, originalArgs } = item;
+    const args = {...originalArgs, triggerEvent: event};
    if (this.commands.isEnabled(command, args)) {
      this.commands.execute(command, args);
    }

but in practice we would probably want to adjust the signature of execute to avoid repetition of this operation. We would also want to make similar adjustments in isEnabled and similar methods.

Additional context

fcollonval commented 1 year ago

Link to the code mentioned above: https://github.com/jupyterlab/lumino/blob/133d8725f4dd2c3a56b10d917f1a7b116ff17ce8/packages/widgets/src/menu.ts#L316

Somehow related for the discussion: https://github.com/jupyterlab/lumino/issues/180

krassowski commented 1 year ago

During the team call an issue of serialisation of commands (e.g. in command-linker in JupyterLab) was raised. If we just pass events as-is, it would potentially limit some functionality for use cases where commands are sent via JSON (or loaded from file); while for the use-case of retrieving context menu event this is not a new limitation, we could consider declaring a custom IEvent interface which would wrap the event and potentially expose some additional information, e.g. exposing a subset of event.target DOM node as a serialisable object. This could be:

interface INodeData {
  boundingBox: {x: number; y: number; height: number; width: number};
  htmlString: string;
  selector: string;
  classNames: readonly string[];
}

interface IEvent {
  target?: INodeData
  // other common event data
}

boundingBox this is essential for some commands but might be too expensive to generate for every event and it may be best avoided (though if event is generated by browser after layout it should not be that expensive... the problem is if we were to trigger forced style calculation because something changed in the DOM since the event was fired)
htmlString could be retrieved using standard XMLSerializer.serializeToString() but:
- is not sufficient as it would not store information on location of the node in the DOM tree, or its bounding box etc.
- can generate a very large string if user clicks e.g. on <body> leading to potential performance/memory issues
selector: there is a number of algorithms generating unique selectors see 1, 2, 3; these are all cheap because they traverse DOM tree upwards; this could be later used by the command to retrieve the target node cheaply:
- we would need to assume/document that no DOM modifications should happen between when selector is generated and command args are passed, which seems reasonable and doable
- this may not be able to identify nodes which are not elements, e.g. TextNode (but I am not sure here)
classNames it is a common use case to check if target has some className, so we could include it:
- the counterpoint is that it is more common to check if any element in path between the target node and document root has a class name, so not including it would prevent developers from shooting themselves in the foot

thoughts? CC @afshin

jupyterlab / lumino