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

#2060: Don't break the selection if the user's cursor goes beyond the canvas #2071

Merged
merged 2 commits into from
Jan 10, 2023
Merged

#2060: Don't break the selection if the user's cursor goes beyond the canvas #2071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24e78e2
#2060 — Don't break the selection if the user's cursor goes beyond th…
Jan 9, 2023
838bf08
#2060 - fix redundant actions on mouse events when not over canvas
Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions packages/ketcher-react/src/script/editor/Editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ function isMouseRight(event) {
}

function resetSelectionOnCanvasClick(editor: Editor, eventName: string) {
if (eventName === 'mouseup') {
if (eventName === 'mouseup' && editor.selection()) {
editor.selection(null)
}
}
Expand All @@ -616,17 +616,18 @@ function updateLastCursorPosition(editor: Editor, event) {
function domEventSetup(editor: Editor, clientArea) {
// TODO: addEventListener('resize', ...);
;[
'click',
'dblclick',
'mousedown',
'mousemove',
'mouseup',
'mouseleave',
'mouseover'
].forEach((eventName) => {
{ target: clientArea, eventName: 'click' },
{ target: clientArea, eventName: 'dblclick' },
{ target: clientArea, eventName: 'mousedown' },
{ target: document, eventName: 'mousemove' },
{ target: document, eventName: 'mouseup' },
{ target: document, eventName: 'mouseleave' },
Copy link
Collaborator

@yuleicul yuleicul Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi 😃 seems it causes a small bug: pasted structure is here to stay when the mouse is moving outside the canvas.

Steps to Reproduce

  1. Cut or copy a structure
  2. Ctrl+v to paste it on the canvas
  3. Move the mouse to the toolbar

Actual behavior
The pasted structure is still here
ketcher-paste-bug-07

Expected behavior
The pasted structure disappears. The following GIF is created on v2.7.0 RC test remote version.
ketcher-paste-bug-08

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we have explicit requirements to remove it when over a toolbar?
Why should we use controversial logic for different tools? We do not break selection when going over toolbars, why should we break copy-paste action?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right. Actually, sometimes I'm a bit confused about some controversial logic. Thanks for your fast reply :)

Copy link
Collaborator

@yuleicul yuleicul Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

And please check out this one. The hand tool is still following the cursor when moving outside the canvas.

ketcher-paste-bug-09

I'm not sure if it's caused by that line of code, but I guess it's because the mouseleave event is not triggered. The following code in ketcher-react/src/script/ui/views/components/StructEditor/StructEditor.jsx may help.

case 'leave':
  this.editorRef.current.classList.remove(classes.enableCursor)
  this.setState({
    enableCursor: false
  })
  break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that does look weird. Thanks for noticing.
I think it deserves its own issue on the board. I'll look into it later.

{ target: clientArea, eventName: 'mouseover' }
].forEach(({ target, eventName }) => {
editor.event[eventName] = new DOMSubscription()
const subs = editor.event[eventName]
clientArea.addEventListener(eventName, subs.dispatch.bind(subs))

target.addEventListener(eventName, subs.dispatch.bind(subs))

subs.add((event) => {
updateLastCursorPosition(editor, event)
Expand All @@ -644,7 +645,11 @@ function domEventSetup(editor: Editor, clientArea) {
}
const EditorTool = editor.tool()
editor.lastEvent = event
if (EditorTool && eventName in EditorTool) {
if (
EditorTool &&
eventName in EditorTool &&
clientArea.contains(event.target)
) {
EditorTool[eventName](event)
return true
}
Expand Down
6 changes: 5 additions & 1 deletion packages/ketcher-react/src/script/editor/tool/select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,11 @@ class SelectTool {
// TODO it catches more events than needed, to be re-factored
this.selectElementsOnCanvas(newSelected, editor, event)
} else if (this.#lassoHelper.fragment) {
if (!event.shiftKey) editor.selection(null)
if (
!event.shiftKey &&
this.editor.render.clientArea.contains(event.target)
)
editor.selection(null)
}
editor.event.message.dispatch({
info: false
Expand Down