Bendpoints module introduces an "unexpected" side-effect

[AdamBanham]

While building out an modeller for myself, I came across the following side-effect. Is it intended or can it be avoid via a custom interaction service, i.e. 'bendpoints.mousemove'?

Describe the Bug

Including the 'Bendpoints' Module prevents interaction events being used by other services, such as interaction services consuming 'element.mousemove' as suggested by the 'InteractionEvents' module.

'Bendpoints' module's likely lines related to issue:

diagram-js/lib/features/bendpoints/Bendpoints.js

Lines 143 to 149 in a74d522

	function bindInteractionEvents(node, eventName, element) {
	domEvent.bind(node, eventName, function(event) {
	interactionEvents.triggerMouseEvent(eventName, event, element);
	event.stopPropagation();
	});
	}

UPDATE: another line that likely needs to checked is the element.out event handler that removes interaction events; it seems like it should only remove interactions if the element is a connection.

diagram-js/lib/features/bendpoints/Bendpoints.js

Lines 386 to 388 in 53a43dd

	eventBus.on('element.out', function(event) {
	interactionEvents.unregisterEvent(event.gfx, 'mousemove', 'element.mousemove');
	});

'InteractionsEvents' module contract:

diagram-js/lib/features/interaction-events/InteractionEvents.js

Lines 48 to 71 in a74d522

	/**
	* A plugin that provides interaction events for diagram elements.
	*
	* It emits the following events:
	*
	* * element.click
	* * element.contextmenu
	* * element.dblclick
	* * element.hover
	* * element.mousedown
	* * element.mousemove
	* * element.mouseup
	* * element.out
	*
	* Each event is a tuple { element, gfx, originalEvent }.
	*
	* Canceling the event via Event#preventDefault()
	* prevents the original DOM operation.
	*
	* @param {EventBus} eventBus
	* @param {ElementRegistry} elementRegistry
	* @param {Styles} styles
	*/
	export default function InteractionEvents(eventBus, elementRegistry, styles) {

It feels like the 'Bendpoint' module is intended to consume element.mousemove on connections, which is fine, but has a side-effect of consuming all mousemove for all shape types. In making my modeler, I made connections below the shapes so they will overlap with shape bounding boxes and seems like the common approach to handle connections, albeit a bit lazy. It would be nice if mousemove events could be propagated to shapes. Within my modeller, I consume both 'element.mousemove' and 'connect.move', its seem that only 'element.mousemove' are being consumed.

Steps to Reproduce

Steps to reproduce the behavior:

1. Add an interaction service, which does some foobar function on element.hover, e.g. changing state for rendering.
2. Include the 'Bendpoints' module within the modules for the diagram.
3. Attempt to trigger element.mousemove and notice that the state has not changed.

I have reconstructed the issue in a codepen: https://codepen.io/AdamBanham/pen/JojmJyz

Demonstrative Evidence

In the modeler I am building, I can see the following behaviour between an interaction service and the addition of the Bendpoints module.

Reported Behavior Before Bendpoints

On 'element.hover' and 'element.mousemove', the role within the fact type changes colour to show if the role is available or connected.
(This foobar flips state on the shape for a fact type, then when rendering a opacity overlay is added).
https://github.com/user-attachments/assets/b17cadd3-af9c-46f8-806c-0f2d0d5c40a0

Reported Behavior After Bendpoints

When bendpoints is added the hover and mousemove events seem to be not triggered.
https://github.com/user-attachments/assets/25d68f84-437e-4659-bf89-4560e9994a05

Expected Behaviour

I expected that the interaction services would still get their events propagated down the dom tree to my custom service. It seems that 'element.hover' is propagated but 'element.mousemove' is not.

Environment

Please complete the following information:

• Browser: vscode-extension inside a webview using node 18
• OS: Windows 11
• Library version: 15.2.4

[philippfromme]

I've had a look and the problematic part is this:

diagram-js/lib/features/bendpoints/Bendpoints.js

Lines 386 to 388 in 53a43dd

	eventBus.on('element.out', function(event) {
	interactionEvents.unregisterEvent(event.gfx, 'mousemove', 'element.mousemove');
	});

I'm not even sure what we're trying to do there and how the element.mousemove event should be any different from other events. Commenting that out doesn't make any difference except for fixing the issue with element.mousemove being fired again.

We'll have to investigate and see whether it's that easy to fix or whether I'm missing something here.

[AdamBanham]

This issue has been sitting there for a bit. Is there any movement at all with the issue? I have some spare time would be happy to contribute towards working out a change to move the issue forward.

The module has a lot of mixed concerns in its current form, it might be an ideal option to do some refactoring while also making some examples for documentation. It would be helpful for those looking to build something with diagram-js (like myself). For the most part, the code self documents but some more involved examples would be super helpful.

[barmac]

We are happy to accept contributions. Please make sure to read the contributing guidelines.

[AdamBanham]

So I did some digging into the history of the bendpoints module (which goes back to 2015). I thought I might share my findings here first before doing any modification for a possible PR.

Structure

The main entry point for the module is Bendpoint.js, however there are several sub-modules that handle the nicer interactions. Two dragging module extensions BendpointMove.js and ConnectionSegmentMove.js which handle when either a Bendpoint or Dragger are clicked and dragged to change the connection's waypoints. Consuming events from these dragging extensions
is BendpointMovePreview.js, which skins the connection when a dragging action is active.

Then two utility/helper files, GeometricUtil.js and BendpointUtil.js are used to offload some of the more often used work. The former is very straightforward in terms of the logic included. However, the latter file contains a mix of concerns. BendpointUtil.js contains the render styling for Bendpoint and Dragger, as well as some hit logic to handle user events.

Events (Listened/Emitted)

The following is the breakdown of events that are listened and emitted by the entry point of the module Bendpoints.js:

                                                Bendpoints.js
                             ________________________________________________________                         
  connection.changed    ---> |                                                      |
  connection.remove     ---> |                                                      |
  element.marker.update ---> |  !addHandles(element)                                |
  element.mousemove     ---> |  getBendpointsContainer                              |
                             |    bindInteractionEvents(gfx, 'mousedown', element)  |
                             |    bindInteractionEvents(gfx, 'click', element)      |
                             |    bindInteractionEvents(gfx, 'dblclick', element)   |
  element.mousedown     ---> |                                                      | ===> conSegDrag.init | bendDrag.init | null   
  selection.changed     ---> |  !addHandles(element)                                |
  element.hover         ---> |  !addHandles(element)                                | ===> registerEvent   'element.mousemove'
  element.out           ---> |                                                      | ===> unregisterEvent 'element.mousemove'
  element.updateId      ---> |                                                      |
                             |  (addHandles(conect))                                |        
                             |    createSegmentDraggers(gfx, conect)                |        
                             |      !bindInteractionEvents(segGfx, 'mousemove', conect) |                                                                                                                               
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~   

Issue

It seems that the issue stems from the module using raw dom queries to insert svg elements into the canvas opposed to bendpoints and the dragger being elements in the registry. Because they are not elements, they do not emit interaction events. Because they do not emit interaction events, the dragging module cannot trigger on-the-fly movement corrections (this is my assumption). So the workout around is to manually bind interactions to the connection's gfx on element.hover. Notably a bendpoint container with a dragger is made new for each connection on hover (if not already existing on the canvas). It is not clear why the 'mousemove' interaction event used by the dragger is specifically removed, while the mousemove click dbclick for bendpionts are not. The diagram above outlines function calls that interact with the interaction events module. The follow on effect from unregisterEvent being called is that the eventBus will not push element.mousemove for connections after they have been hovered once (linking back to my original issue).

The very dangerous issue that I think should be addressed with a hotfix: is that including the bendpoints module, introduces the the following callback to ALL elements, meaning that once they are hovered once the interaction event mousemove is forever lost! There should be at least a type check in this callback to check that the element hovered is a CONNECTION. This would limit the damage of the callback with the likely original intent of the line. But it seems unneeded, as bendpoint interactions are never removed.

diagram-js/lib/features/bendpoints/Bendpoints.js

Lines 386 to 388 in 53a43dd

	eventBus.on('element.out', function(event) {
	interactionEvents.unregisterEvent(event.gfx, 'mousemove', 'element.mousemove');
	});

This suggestion as the hard fix for the above code, worked for my editor. By adding the type check for connections, I noted no further side effects in my editor after the change.

A Possible Longer Path

It seems that in order to support snapping and previews (which are supported for elements in registry) for module, they needed to somewhat replicated in the module. Furthermore, interaction events have to been manually binded and possible unbinded to support the key functionality. The longer path to a clear fix would to make bendpoints and the dragger instantiated elements within the modeller. This would likely entail making the following additions/refactors to the module:

• add bendpoint and dragger object shapes (types)
• add/extend factory to make basic instances
• add renderer solely for bendpoints and dragger
• add/make separate interaction providers
• add cache for bendpoints and have one single dragger (hopefully reducing element load on the canvas)
• see/test if existing previewSupport and Snapping module work as is using new shapes
• update dragging extensions in module or see if move module can be used as-is instead

This would be a rather problematic change but seems like a way to ensure that the module can move with the other parts of the library in the future. I also imagine that the owners might not be interested in such a possibly breaking change to the module. @barmac @philippfromme could you advise if such an aggressive change would be interested or would a simple PR to add the type check be preferred? Happy to help on both ideas or another less aggressive option for changes.

[philippfromme]

I will look into this again. What I can tell you already is that we will not make utilities like drag handles diagram shapes. That makes no sense from a semantics point of view. Diagram shapes and connections are strictly reserved for elements that are part of a diagram regardless of whether you open it in a viewer (no drag handles) or a modeler.

[philippfromme]

This behavior was introduced 10 years ago. At that time it might not have caused any issues since a lot of other features didn't exist.

[barmac]

I noticed that we did not register the mousemove event in the Interaction Events per default at the time when Bendpoints registration was implemented, cf. https://github.com/bpmn-io/diagram-js/blob/v0.18.0/lib/features/interaction-events/InteractionEvents.js#L102
Given that we do it per default now, I assume that we could safely remove the offending code. We'd need to verify that it does not break anything in the downstream libraries: bpmn-js and dmn-js.

[barmac]

Regarding the mouse event, we went the way of stopping propagation, but this means that nobody can react to the event. That's unfortunate. Probably event.preventDefault could be a better choice, but that would mean a breaking change for any listeners which did not care about Bendpoints before.

[AdamBanham]

What I can tell you already is that we will not make utilities like drag handles diagram shapes. That makes no sense from a semantics point of view. Diagram shapes and connections are strictly reserved for elements that are part of a diagram regardless of whether you open it in a viewer (no drag handles) or a modeler.

Sounds reasonable. I can see why adding them to registry would be painful and problematic. It was just an idea.

Regarding the mouse event, we went the way of stopping propagation, but this means that nobody can react to the event. That's unfortunate. Probably event.preventDefault could be a better choice, but that would mean a breaking change for any listeners which did not care about Bendpoints before.

The interaction event chain for listeners supports returning a falsely value, no? This would prevent the event listener chain inside diagram-js from propagating to further down the chain to lower/same priority listeners in the event bus chain. What web browser/container feature listens to mousemove and requires a hard prevent?

With the downstream libraries dependencies, the issues does not seem a good PR for an outsider like me to make. But I hope the mini RCA was helpful for resolving some of the oddities in the module, likely due to a bit of aging.

I still think the smallest change is just adding the previously mentioned type check without causing major headaches.