Smarter ContextMenu items: pass contextmenu event to ContextMenu items

[telamonian]

Adds passDataset property to contextmenu items:

/**
 * Pass a selected node's dataset to an item's command.
 *
 * If this flag is set, when a contextmenu item is matched
 * to the target of a contextmenu event, the key-value pairs
 * in `target.dataset` (if present) will be added as
 * properties of `item.args`.
 *
 * Only meaningful for items of type `command`.
 */
 passDataset?: boolean;

The behavior of the flag is implemented via a few lines added to the matchItems function:

// Pass the target's dataset to the matched item's args, if requested.
if (item.passDataset && 'dataset' in target) {
  item.args = {...item.args, ...(target as any)['dataset']};
}

This PR doesn't change any existing behavior, it just adds some optional new behavior. See #354 for use cases/discussion.

[sccolbert]

I've been wondering if we should add something like $node: Node to the command args. While not JSON, it may be the pragmatic choice. I worry that if we just pass the dataset, then people will have to start adding uuids to the dataset just to recover which node was clicked.

Another alternative would be to have a static property somewhere which is set to the node for the duration of the command execution. The relevant command could then look at that node as needed.

[blink1073]

I like the idea of having this static method on CommandRegistry:

currentCommandTarget(): HTMLElement | null {
   return document.body.querySelector('[data-phosphorCommandTarget]');
}

[sccolbert]

Another options would be to add a third optional arg to the execute method:

execute(id: string, args: ReadonlyJSONObject = JSONExt.emptyObject, target?: HTMLElement): Promise<any>

And that would element would be set to the currentTarget property of the registry for the duration of the execute call.

[afshin]

https://developer.mozilla.org/en-US/docs/Web/API/Window/event

If only Firefox supported this, we'd be in the clear. In Chrome and Safari and IE, you can access window.event.target, e.g.:

  app.commands.addCommand(command, {
    label: 'Single-Document Mode',
    isToggled: () => app.shell.mode === 'single-document',
    execute: () => {
      console.log('target is', window.event && window.event.target);
      const args =
        app.shell.mode === 'multiple-document'
          ? { mode: 'single-document' }
          : { mode: 'multiple-document' };
      return app.commands.execute(CommandIDs.setMode, args);
    }
  });

results in:

[sccolbert]

That would be nice. However that's giving you the actual label div that was clicked. Allowing us to pass in the node gives us the option of passing in the "most relevant" node, which in this case would be the entire menu item node, instead of the child label.

[telamonian]

Here's another wrinkle: not just execute but also the the other callbacks in ICommandOptions may need access to the target node when they're called.

There's some relevant examples of isEnabled needing access to the target's info in JupyterLab's docmanager-extension. Basically, these isEnabled calls need to be able to determine if the target node is associated with an appropriate path.

[telamonian]

I like the idea of stashing the target node in a static property, since that will make it easy to access the target within various functions (eg execute, isEnabled, etc). I also like the idea of adding an optional third target?: HTMLElement arg to execute, since that seems like it will make it easier for the average developer to figure out how to write context menu commands that make use of the target node.

I think these two things could be combined into a single nice design via some code in Menu.triggerActiveItem that would pass target to isEnabled and execute (and also some similar code in MenuItem.isEnabled, etc).

[telamonian]

Okay, I've written/tested a new version of the PR. The passDataset flag has been completely removed. Instead, when a context menu is opened, each item will now store their target HTML nodes for the lifetime of the menu. The target is then passed to execute as a third optional target arg, as @sccolbert suggested. Here's an example implementation of a command that makes use of target:

    commands.addCommand(CommandIDs.showInFileBrowser, {
      label: () => `Show in File Browser`,
      isEnabled,
      execute: (args, target) => {
        // give 1st precedence to explicitly passed path
        let path = args['path'];
        if (!path && target) {
          // give 2nd precedence to path on the target node, if present
          path = target.dataset['path'];
        }
        if (!path) {
          // fall back to path of focused widget
          path = docManager.contextForWidget(shell.currentWidget).path;
        }
        if (!path) {
          return;
        }
  
       ...
        // do stuff with path
       ...
      }
    });

There's some more work to be done to make target available to isEnabled(), etc, but I figured this would be a good place to stop and get some feedback from the maintainers. If someone could review the code in the latest commit (maybe @sccolbert or @afshin?), that would be really helpful.

[afshin]

What if the command registry has an attached property so that the API surface area of the command functions doesn't change. Suppose you could do:

commands.addCommand(CommandIDs.showInFileBrowser, {
  label: () => `Show in File Browser`,
  isEnabled,
  execute: (args) => {
    const target = commands.targetProperty.get(args);
    // etc.
  }
});

[telamonian]

I can definitely make target into a command registry property. It'll be a little tricky to still support the fact that different items in the same menu can have different targets (depending on their selector), but I think I know how it can be done.

I am a little confused by something in your example code, though @afshin. Could you please clarify what you mean by .get(args) in the line const target = commands.targetProperty.get(args);?

[afshin]

@telamonian I would say hold off on doing that to see what @sccolbert thinks. But the targetProperty in my code block would be a phosphor AttachedProperty instance in the CommandRegistry namespace. So that individual args being deployed to a command execution would have a target property attached to them that can be retrieved by commands.targetProperty.get(args),

[telamonian]

Huh. I don't think I've ever come across a pattern quite like that of AttachedProperty. But I get it, and it does seem like a nice fit for this use case.

[telamonian]

Okay, here's the 3rd stab at this PR. This version is written along the lines of the discussion I had with @sccolbert at the Jupytercon sprint. contextEvent is now a property of CommandRegistry. Some notes:

• I had to subclass the Menu that ContextMenu wraps in order to get the timing of the cleanup of contextEvent correct. Originally I tried connecting a cleanup function to the Menu._aboutToClose signal, but then cleanup happens too early in Menu.triggerActiveItem.

• I also added a contextEventTarget(selector: string): Element | null convenience function to CommandRegistry. It's basically just recycled code from matchItems. I put this in since it'll be useful for some of the stuff I want to do, and it seems to be non-trivial to come up with a good implementation. If contextEventTarget doesn't seem like it'll be of general enough use, though, I can always take it out