jupyterlab/lumino

[DataGrid] double-clicking cells to edit them is flakey

vthemelis opened this issue · 11 comments

Description

Double-clicking editable cells is meant to actuate an editor to change their values (see #14).

From my experience with the example DataGrids (here), the dblclick event does not reliably fire and it's not always possible to see the editor.

I also noticed the same while working on #546 which also relies on dblclick events.

Reproduce

  1. Start the example DataGrid page according to the steps in the main README file.
  2. Double click on a few cells on the Editable Grid in the top left.
  3. You will notice that you cannot consistently trigger the edit mode.

You can always trigger the edit mode by selecting a cell and starting to type though.

Expected behavior

dblclick events should be consistently fired and trigger the editor.

Context

  • Operating System and version: macOS Monterey (12.0.1)
  • Browser and version: Chrome Version 110.0.5481.177 (Official Build) (arm64) and Safari Version 15.1 (17612.2.9.1.20)

CC: @mbektas as the author of #14

jjrv commented

I investigated this a bit. Double clicking to edit seems to work reliably if I do one of:

Without those and if I accidentally move the mouse a tiny bit between clicks, it doesn't just ignore that double click but future ones as well in that column and some others (not sure what the exact pattern is). It recovers if I single click various cells in different columns, again not sure exactly what kind of cells or how many times. It's sort of unpredictably flakey after the first imprecise double click.

To clarify, I don't even get a dblclick event once the flakiness starts. I don't just bail early from the handler.

jjrv commented

Another clue is that if I comment out this line:

model.select({ r1, c1, r2, c2, cursorRow, cursorColumn, clear });

A failed double click will select cells between (0,0) and the clicked cell. While that selection is visible, double clicking to edit is seemingly randomly broken. So this seems like a conflict between dragging the mouse to select cells vs double clicking to edit. It breaks if it thinks both happen simultaneously.

As you mentioned, once it's broken, handleEvent in datagrid.ts never even gets called with a dblclick event. If it does get called, the editor pops up reliably, so the flakiness is not in dblclick event handling.

jjrv commented

Actually it breaks if I make the tiniest drag movement. So after page load double clicking to edit by lifting the mouse is initially reliable but if I hold the mouse button pressed and move it by one pixel inside one cell, it becomes flakey in nearby cells. The dblclick event doesn't fire at all. It's all happening inside the same canvas element so I can't see what's blocking the event.

jjrv commented

OK, so when the button is pressed, a DOM node is inserted with the class lm-cursor-backdrop. Initially its style has no transform attribute and everything works. First time I move the mouse with the button pressed, it gains that attribute permanently and double clicking becomes unreliable. That div, if it has a CSS transform applied, is somehow messing with the mouse event.

Edit: If I add this in <head>:

        <style type="text/css">
.lm-cursor-backdrop { 
background-color: #ff00ff;
}
        </style>

I can see the enormous box that's messing with my attempts to edit cells 🤣

jjrv commented

And, of course, I can fix the bug from my own code with:

.lm-cursor-backdrop { 
pointer-events: none;
}

It's still sad that double clicks aren't detected if the mouse moves the tiniest bit during them, but at least it won't break everything if I ever fail to execute a perfect double click.

Interesting, looks like this div was only recently added in #502 by @krassowski .

When Drag.overrideCursor is active, local pointer events (=anything not listening on document) are no longer triggered. This largely does not change the behaviour when this method is used with the Drag class as it was already capturing (and preventing) many events that could interfere with drag experience.

Would you like to open a PR to fix it?

jjrv commented

Sure! Guess I'll just add the pointer-events fix to packages/dragdrop/style/index.css where that div is given all the other, in this case troublesome styles.

Edit: #564

jjrv commented

The rabbit hole continues in the PR and is deeper than expected.

This can be closed now that #564 has been merged.

Thanks @jjrv ! 🎉