fix(tooltip): clean up focus handling

[smbea]

Closes #272
Related to camunda/camunda-modeler#3756

[pinussilvestrus]

Do you plan to have this fix in the current release already? I'm asking because I already released the popup editor via a minor version, so bumping the properties panel would mean we introduce this everywhere.

Not a problem, just wanted to make sure we are aware of this 👍

[smbea]

Do you plan to have this fix in the current release already? I'm asking because I already released the popup editor via a minor version, so bumping the properties panel would mean we introduce this everywhere.

Not a problem, just wanted to make sure we are aware of this 👍

Would this change anything, since it hans't been released with form-js? I don't think this is blocking, so we can postpone releasing

[barmac]

Thanks for a quick fix. How would I test this? Could you share what the root cause of the problem is?

[smbea]

Thanks for a quick fix. How would I test this?

You can test it by liking this branch in bpmn-js-properties-panel. I recommend trying it out with the timer props because the tooltip has more content.

Could you share what the root cause of the problem is?

Like I mentioned above, we added a preventDefault on mouseDown to the container div to ensure group headers tooltips wouldn't stay always visible after clicking on them to open. This made it so that when trying to select the content, the default selection behaviour didn't happen. So I stopped propagation of the events within the tooltip content div. Happy to explain further via call if my explanation was confusing

[barmac]

So if I get it correctly, we want to keep the interaction within tooltip. Is there any need for prevent default in that case?

[barmac]

Also, it would be cool to have automatic tests for both renderPortal and direct render case. Thus, we can easily verify the correct behavior, and more important catch it when it breaks again.

[smbea]

So if I get it correctly, we want to keep the interaction within tooltip. Is there any need for prevent default in that case?

The prevent default prevents the trigger element (group header) to become focused via mouse, thus keeping the tooltip open. We only want that to happen via keyboard navigation.

Also, it would be cool to have automatic tests for both renderPortal and direct render case. Thus, we can easily verify the correct behavior, and more important catch it when it breaks again.

I can work on adding test cases for createPortal

[smbea]

There is actually a test for rendering via portal

I haven't been able to add a test case that reproduces the content selectable or the focus issue I mentioned was fixed with prevent default.

[smbea]

When trying to fix #272 and camunda/camunda-modeler#3756, I ended up breaking one while fixing the other. This is because preventing default actions in some places (like focus on mouseDown) also ends up preventing wanted actions (like selecting context).

In order to not end up with super confusing code trying to gracefully handle these events, I decided to clean up how we open/persist tooltips based on focus - which is what causes all the issues.

I also added some test cases which should help enforce this, but it's hard to test these things for every case so feel free to give it a good user test.

[marstamm]

I can confirm that this fixes the selection issue in a browser environment. Good job cleaning up the opening handling.

As expected, in the Camunda Modeler, selection is still blocked for the whole modeler container