Skip to content

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

Merged
krassowski merged 1 commit into
jupyterlab:mainfrom
jjrv:fix-547
May 11, 2023
Merged

Fix dblclick events with Drag.overrideCursor active (#547)#564
krassowski merged 1 commit into
jupyterlab:mainfrom
jjrv:fix-547

Conversation

@jjrv

@jjrv jjrv commented Mar 22, 2023

Copy link
Copy Markdown
Contributor

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

@welcome

welcome Bot commented Mar 22, 2023

Copy link
Copy Markdown

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.
welcome
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

jjrv commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

@jjrv

jjrv commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

@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

jjrv commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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

jjrv commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

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 jjrv changed the title Fix local pointer events with Drag.overrideCursor active (#547) (BROKEN PR) Fix local pointer events with Drag.overrideCursor active (#547) Mar 22, 2023
@jjrv jjrv changed the title (BROKEN PR) Fix local pointer events with Drag.overrideCursor active (#547) Fix local pointer events with Drag.overrideCursor active (#547) Mar 22, 2023
@jjrv

jjrv commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

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.

@jjrv jjrv changed the title Fix local pointer events with Drag.overrideCursor active (#547) Fix dblclick events with Drag.overrideCursor active (#547) Mar 22, 2023
@fcollonval fcollonval added the bug Something isn't working label Apr 7, 2023
@fcollonval fcollonval requested a review from krassowski April 7, 2023 07:12

@krassowski krassowski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/dragdrop/src/index.ts
The .lm-cursor-backdrop div was preventing dblclick events from reaching datagrid, breaking cell edit functionality.
@vthemelis

vthemelis commented May 9, 2023

Copy link
Copy Markdown
Contributor

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 krassowski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jjrv!

@krassowski krassowski merged commit 613dc9d into jupyterlab:main May 11, 2023
@welcome

welcome Bot commented May 11, 2023

Copy link
Copy Markdown

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants