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.

Sign up for GitHub

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

Jump to bottom

Should commands receive an event that triggered them? #554

Open
krassowski opened this issue Mar 12, 2023 · 2 comments
Open

Should commands receive an event that triggered them? #554

krassowski opened this issue Mar 12, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@krassowski
Copy link
Member

krassowski commented Mar 12, 2023 •
edited

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
Copy link
Member

fcollonval commented Mar 15, 2023 •
edited

Link to the code mentioned above:

lumino/packages/widgets/src/menu.ts

Line 316 in 133d872

this.commands.execute(command, args);

Somehow related for the discussion: #180

@fcollonval fcollonval mentioned this issue Mar 16, 2023
Closed
@krassowski krassowski mentioned this issue Apr 1, 2023
Open
@krassowski
Copy link
Member Author

krassowski commented Apr 1, 2023 •
edited

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@krassowski @fcollonval