Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
vthemelis opened this issue Feb 25, 2023 · 11 comments
Closed

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

vthemelis opened this issue Feb 25, 2023 · 11 comments
Labels
bug Something isn't working

Comments

@vthemelis
Copy link
Contributor

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)
@vthemelis vthemelis added the bug Something isn't working label Feb 25, 2023
@vthemelis
Copy link
Contributor Author

CC: @mbektas as the author of #14

@vthemelis vthemelis mentioned this issue Feb 25, 2023
Merged
@jjrv
Copy link
Contributor

jjrv commented Mar 22, 2023

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.

@vthemelis
Copy link
Contributor Author

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

@jjrv
Copy link
Contributor

jjrv commented Mar 22, 2023 •
edited
Loading

Another clue is that if I comment out this line:

lumino/packages/datagrid/src/basicmousehandler.ts

Line 247 in d577cc9

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
Copy link
Contributor

jjrv commented Mar 22, 2023

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
Copy link
Contributor

jjrv commented Mar 22, 2023 •
edited
Loading

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
Copy link
Contributor

jjrv commented Mar 22, 2023 •
edited
Loading

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.

@vthemelis
Copy link
Contributor Author

vthemelis commented Mar 22, 2023 •
edited
Loading

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
Copy link
Contributor

jjrv commented Mar 22, 2023 •
edited
Loading

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 added a commit to jjrv/lumino that referenced this issue Mar 22, 2023
@jjrv
Fix local pointer events with Drag.overrideCursor active (jupyterlab#547
dd9a5fb 
)

The .lm-cursor-backdrop div was preventing dblclick events from reaching
datagrid, breaking cell edit functionality.
@jjrv jjrv mentioned this issue Mar 22, 2023
Merged
@jjrv
Copy link
Contributor

jjrv commented Mar 22, 2023

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

jjrv added a commit to jjrv/lumino that referenced this issue Mar 22, 2023
@jjrv
Allow double clicks with Drag.overrideCursor active (jupyterlab#547)
265df92 
The .lm-cursor-backdrop div was preventing dblclick events from reaching datagrid, breaking cell edit functionality.
jjrv added a commit to jjrv/lumino that referenced this issue Apr 7, 2023
@jjrv
Allow double clicks with Drag.overrideCursor active (jupyterlab#547)
da7f424 
The .lm-cursor-backdrop div was preventing dblclick events from reaching datagrid, breaking cell edit functionality.
krassowski pushed a commit that referenced this issue May 11, 2023
@jjrv
Allow double clicks with Drag.overrideCursor active (#547) (#564)
613dc9d 
The .lm-cursor-backdrop div was preventing dblclick events from reaching datagrid, breaking cell edit functionality.
@vthemelis
Copy link
Contributor Author

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

Thanks @jjrv ! 🎉

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jjrv @vthemelis