Fix dblclick events with Drag.overrideCursor active (#547)

[jjrv]

The .lm-cursor-backdrop div introduced in #502 was preventing dblclick events from reaching datagrid, breaking cell edit functionality.

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[jjrv]

Ping @krassowski I don't think making the backdrop div transparent to pointer events would interfere with the performance reasons for which it was created?

[krassowski]

This is not immediately clear to me and will require manual benchmarking

[jjrv]

@krassowski Hmm! This sounds strange but I tested the Lumino datagrid example with only grid6 (the editable one) and 100000 divs and spans added to the document. Clicks on the datagrid are equally slow regardless of whether this fix is present or not.

However if I do this:

.lm-cursor-backdrop {
display:none;
}

All the clicks are handled instantly and also bug #547 is fixed. Opening and closing the cell editor a few times crashes the browser tab though, but that sounds like a different issue (it does that regardless of the styles I apply to .lm-cursor-backdrop).

[jjrv]

Here's a Gist to test it:
https://gist.github.com/jjrv/0be6215e0be34b6da1a998917bbaf518

Run npm install && npm start to test.

Uncomment display: none or comment pointer-events: none in index.html to compare.

[krassowski]

Great to hear! We ideally want to keep the cursor override but of course we can adjust the implementation. I am away from keyboard right now so cannot test but what I am worried about is whether cursor override works when pointer-events: none is set. The alternative solution would be to add event listener on the backdrop div and pass the click events down transparently.

[jjrv]

OK ignore that benchmark, the performance issues were because I added the test divs directly under body and the backdrop caused a layout slowdown when added as their sibling. That's an unrealistic situation in practice.

Also, the pointer-events: none does break the override. Back to the drawing board...

[jjrv]

New attempt! The prior functionality was already that the backdrop div is only correctly placed after the mouse moves with the button pressed:

      document.addEventListener('pointermove', alignBackdrop, {
        capture: true,
        passive: true
      });

So before that pointermove event, we might as well hide the div. Then double click events to the underlying element still get detected, and datagrid editing is fixed.

[krassowski]

Conceptually looks good.

We may still want to propagate click events through backdrop manually by creating synthetic events but this can be done in a separate PR.

[vthemelis]

Hi @krassowski and @jjrv,

Would it be possible to merge this now? It would be great if I could get the edit functionality to work robustly.

[krassowski]

Thank you @jjrv!

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️