Ensure firing out event

[pinussilvestrus]

This ensures we definitely fire an element.out event between two hover for different elements. This way we make sure cursor and marker cleanup really got to run, despite the fact some browsers swallow certain mouseout events.

Related to camunda/camunda-modeler#1383

[pinussilvestrus]

This does not affect bpmn-io/bpmn-js#1123 (despite the fact this bug may also be caused by a missing fired event)

[nikku]

This fix is substantially different from the HoverFix:

• it emits a fake element.out event
• it hooks into low-level DOM events to get the job done

The hover fix on the other hand gets along with:

• Using already exposed events
• Faking a dragging.hover event, if non existed

Is there a reason why we're going for a different route for the out fix?

Is there a good reason why we don't combine the two into a single file? I guess using only one of them makes no sense, or does it?

[pinussilvestrus]

it emits a fake element.out event

+1 going for executing drag.out instead, should follow to the same result.

it hooks into low-level DOM events to get the job done

This aims to ensure the additional out event is NOT thrown when it was already caught. But yeah true, I'd go for tracking the element.out instead of using the low-level DOM listener.

Is there a good reason why we don't combine the two into a single file?

I think there is not. I wanted to definitely separate the two fixes since they solve different things. Will go for having them in the same module but separate them there instead.

[nikku]

Why do you expect an additional out event after hover was fired? And if it was thrown, would it hurt?

[pinussilvestrus]

Not after the hover, but we have the following happy path in terms of events when we hover from one element to another:

hover Element A -> out Element A -> hover Element B

The bug in camunda/camunda-modeler#1383 is caused by the fact the out event between the two hover's is not fired in some occasions. The fix aims to simply add this missing out if (and only if) it is not fired.

Why so (and does it hurt us if we have multiple out events)? Without the check, we run into side effects

which look like bpmn-io/bpmn-js#1123, but it is happening all the time after adding a connection between two elements (as shown in the screencast). We discussed it with @philippfromme yesterday to add the check to ensure, we will not run into this kind of side effects when triggering unnecessary out operations.

I'll update the PR to the things mentioned above

[nikku]

Cool. Thanks for the clarification.

[pinussilvestrus]

One additional finding

it emits a fake element.out event

I tried to go for the drag.out way instead of emitting the element.out event. But at the point we are want to do the out operation for the previousHovered (Element A), the drag context is still set to Element B. This way it removes Element B from the drag context, which leads to side effects (in fact failing test cases).

In fact, waiting for the next drag.hover does not follow to the exact event-order as described from me above. To really achieve this, we would have hook into before a second drag.hover is fired, but after the mouse leaves Element A. This would follow to a similar solution like the current HoverFix. So we have two options

• NOT using drag.out but emitting element.out for Element A, leave the rest
• Changing the behavior to hook into before a second drag.hover is fired

@nikku what do you think? I'd personally go for the first one. Benefit: not using expensive operations in additional places ...

[pinussilvestrus]

Updated the PR to my preferred version.

[nikku]

I'll need to look into the issue.

If you have specific requirements regarding the event order, does it make sense to bake these into the test cases? Right now the coverage you provided is very light. I'd assume we verify the actual order and occurrence of events, from hover to out:

expect(emittedEvents).to.eql([
  [ 'drag.hover', ... ],
  [ 'drag.out', ... ]
]);

[pinussilvestrus]

Would totally go for additional test coverage, also for the existing HoverFixSpec. Originally planned to take the existing test case to the Hour of Code, since it does not test the overall functionality (despite the fact it's hard to simulate a MouseEvent, which would be needed in this case).

Really like the idea of testing the event order, since this would really ensure the expected behavior. Feel free to add additional thoughts regarding the solutions provided above if you had a deeper look. We can also chat about it next week.

[nikku]

I'm unsure about whether the things you've mentioned are issues I overlook or whether I came up with a solution that just works.

Could you checkout 87f2b21 and see if it suffers from the same limitations you mentioned?

Please add additional test coverage, regardless of the implementation we are going for. It should not depend on it, anyway. Hint: Do not do dragging.setOptions({ manual: true }) and you can nicely script the drag interaction by simply emitting element.hover / element.out events.

[pinussilvestrus]

The problem I described does not come out of your implementation because it just solved them 👍 Great approach! It ensures the things I mentioned (cf. out event for correct element) in a good manner I think ❤️

To complete this PR I will

• transfer your changes to this
• add additional test coverage

Thanks for your time!

[pinussilvestrus]

Ready to review now!

After finishing, I was not able to reproduce bpmn-io/bpmn-js#1123 again. Could be just luck or we really tackled this problem here as well. We will have to double-check it then.

Also tried to improve the existing test in HoverSpec but it revealed as being very hard to mock proper mouse events for this case. Let's postpone this for a future Hour of Code.