Blazor-Diagrams / Blazor.Diagrams

A fully customizable and extensible all-purpose diagrams library for Blazor
https://blazor-diagrams.zhaytam.com
MIT License
921 stars 177 forks source link

Wrong call in EventsBehavior.cs ? #204

Closed xevoryn closed 1 year ago

xevoryn commented 2 years ago

I stumbled upon a pretty subtle but from my point of view just wrong behaviour. Inside of EventsBehavior.cs is a check if a mouse movement occurred. After that in line 56 there is a invokation of OnMouseClick where I would rather expect OnMouseUp because it is the event handler of the MouseUp event. This "wrong" event call leads to some weird behaviour in our application. I was also able to verify that this is the problem by unregistering the EventsBehavior from the diagram.

Is this just wrong or is there more to this call?

xevoryn commented 2 years ago

So I referenced the project within ours and just swapping the event doesn't resolve the issue. It sadly breaks other things so there really is more to it then I thought.

What also came into my mind: Wouldn't it be possible to just use the ondblclick JS event on the diagram canvas to detect double clicks? Wouldn't this make the EventsBehavior.cs obsolete?

zHaytam commented 2 years ago

Hello,

MouseClick is the correct event to fire there. MouseUp and all other "plain" events are fired from each component (node, link, etc), but mouse clicks and especially double clicks require this custom logic.

It is not possible to use the browser's ondblclick event because it's not available on all the elements that might be used within a diagram (e.g. svg elements).

Can you explain your issue and see if it has anything to do with EventsBehavior?

xevoryn commented 2 years ago

In our application we have a settings menu on the right side of the diagram. Within it there are text fields. We built this menu in a way so that it only has content when at least one diagram element is selected. If you select the text inside one of the textboxes and accidentally move over the diagram while still pressing the mouse button and you let go of the button with the cursor hovering over the diagram it will hide the content.

The weird part is that the selection of the diagram element persists but the menu remains hidden until I actively deselect and reselect the element within the diagram. By completely unregistering the EventsBehavior this is not the case. It works as expected and does not hide the content of the menu.

It does not appear to me that ondblclick isn't available for SVG: look here. And in case the table is suspicious: try this svg fragment on i.e. code pen.

<svg ondblclick="console.log('test')" width="400" height="180">
  <rect x="50" y="20" rx="20" ry="20" width="150" height="150" style="fill:red;stroke:black;stroke-width:5;opacity:0.5" />
  Sorry, your browser does not support inline SVG.
</svg>
zHaytam commented 2 years ago

How do you show/hide the menu based on selection? If I understand correctly, EventsBehavior shouldn't really do anything to that logic unless you're using the mouse events?

xevoryn commented 2 years ago

You're right. We use the mouse events as an indicator to re-check what's selected. If something changed we re-process the content of the menu (show/hide/change).

zHaytam commented 2 years ago

Is there a reason why you don't use the selection events/properties?

xevoryn commented 2 years ago

I thought about the exact same thing when typing the previous message. I have checked again now and we actually do use them to update our menu. I also stated before that the selection remains as is when the issue occurs. I set a breakpoint at our event handler for the selection changed event and it is not fired in that exact moment.

So, while writing this message a colleague brought up some code I clearly missed. We do some stuff within the "diagram clicked" event which does effect our menu in some cases. One of these cases is the mentioned issue. So we could clearly fix this behaviour on our side. But I am still on the side of "something is wrong with the library behaviour". It just does not seems right that a simple mouseUp event fires a mouseClick event.

zHaytam commented 2 years ago

Why doesn't it seem right? MouseDown+Up equals a click in the browser as well, unless I'm mistaken. If you still have an issue and you can reproduce it with a simple example I will be more than happy to look into it!

xevoryn commented 2 years ago

I am currently trying to work out a good example to show the issue but it comes to me that I may did not explain well enough what the problem is. So let me explain again.

The setup is simple: DiagramCanvas on the left and a menu containing text input boxes on the right. If a node inside the canvas is selected or deselected the menu is updated. This also includes a simple click on the empty diagram area because it deselects every node.

When I try to select the content of the text box (by click & drag) and accidentally drag over to the diagram it sometimes happens that I overshoot the left end of the text box and drag over the canvas while still holding down the mouse button. If that happens and I release the mouse button hovering over the diagram I would expect that only the mouse up event will be fired. Instead a complete mouse click will be fired although I only released the mouse button over the diagram and did not press it there.

This is the first issue but it also causes the second one as follow up.

The mouse click event should indicate that the mouse has been clicked completely inside the diagram canvas without leaving it during the click. This is the case when I click the empty area which also deselects every existing selection. The before mentioned issue doesn't do that. I fires the click to the "outside" subscribers but does not activate any internal logic (i.e. deselecting nodes). This results in a fired mouse click but without any selection change. This not really a problem in itself because it can also happen when a node is clicked but it's still a mouse click without model which indicates that the empty canvas area was clicked.

All this is confusing and not logical. To answer your question: It is completely right that mouse down + mouse up indicates a click but there is no preceding mouse down only a mouse up which causes the behaviour I tried to describe.

If you are still questioning the problem I will happily provide some kind of sample code to show what I mean. Please let me now.

xevoryn commented 2 years ago

I have worked out an example. You can find it in this branch. Its based on an older version of the library but it still works as I described.

The workflow to reproduce the issue I am talking about is the following:

  1. Generate some "blocks"
  2. Select some of the block by holding down CRTL
  3. Select the text inside the textbox on the right with the mouse
  4. Hold down the mouse button and drag further until hovering over the diagram area
  5. Release the mouse button over some empty space on the diagram canvas
  6. Repeat steps 3-5 as much as you want

If you click on the diagram canvas normally there will be selection changed > mouse down > mouse click > mouse up. As you will see after step 5 all selected blocks will still be selected and on the right there will be a mouse click and a mouse up event inside the log. But there should only be a mouse up occurrence.

I hope the example helps with understanding the issue we are experiencing.

zHaytam commented 2 years ago

Indeed, now that you've explained the issue, I see it clearly. MouseUp shouldn't fire a MouseClick unless MouseDown happened as well, otherwise something like you described (clicking outside, moving then releasing) would trigger a click when it shouldn't!

I will take a look at you rexample.

xevoryn commented 1 year ago

How is your progress? Did the example help?

zHaytam commented 1 year ago

It will be fixed in the new major version. Would you like a quick fix for it with the current version? I can write you a modified EventsBehavior.