clientIO / joint

A proven SVG-based JavaScript diagramming library powering exceptional UIs
https://jointjs.com
Mozilla Public License 2.0
4.72k stars 853 forks source link

[Bug]: Changing link target or source breaks CommandManager in Angular #2719

Closed ArthurWietzorek closed 3 months ago

ArthurWietzorek commented 4 months ago

Current versus expected behaviour

When running the kitchensink application in Angular 16, changing the target or source of a link breaks the CommandManager. The change itself and any changes made after that action are no longer put on the undo stack. No error is shown in the browser console.

This bug occurs in both JointJS+ 4.0.0 and 4.0.1, but only in the Angular kitchensink app and not it the plain JS one. It does not occur in the previous 3.7.3 version.

Other changes to links to not cause this problem, vertices for example can be added, moved and removed and the CommandManager works just fine.

We have an application based on both Angular and JointJS+ and we encountered the same problem there.

Steps to reproduce

  1. Launch kitchensink application in Angular 16, either on JointJS+ 4.0.0 or 4.0.1.
  2. Move some elements around to fill the undo stack.
  3. Change the target or source of a link.
  4. Click the "Undo" button
  5. Not the change to the link but the change before that is reverted.
  6. Any other changes made to the graph are also no longer being put on the command manager stack after that.

Version

4.0.1

What browsers are you seeing the problem on?

Firefox, Chrome

What operating system are you seeing the problem on?

Linux

kumilingus commented 4 months ago

We can reproduce the issue.

Here's a workaround for the time being:

this.paper.on('link:connect', () => {
    this.commandManager.storeBatchCommand()
});
kumilingus commented 4 months ago

Here's a proper patch that you need to apply locally until we release a new version.

  joint.dia.CellView.prototype.pointerup = function(evt, x, y) {

      const { graph = this.model.graph } = this.eventData(evt);

      this.notify('cell:pointerup', evt, x, y);

      if (graph) {
          // we don't want to trigger event on model as model doesn't
          // need to be member of collection anymore (remove)
          graph.stopBatch('pointer', { cell: this.model });
      }
  }

It's a regression caused by this PR.

kumilingus commented 4 months ago

It will be fixed by https://github.com/clientIO/joint/pull/2720.

ArthurWietzorek commented 4 months ago

Thank you once again for the quick help, the patch works perfectly fine in our application as well. Looking forward to the next update! :+1: