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.

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

Resize node with enabled GridSnapper results in out of synch mouse pointer #400

Closed
ivy-lli opened this issue Sep 24, 2021 · 6 comments
Closed
Assignees
Labels
bug Something isn't working client

Comments

@ivy-lli
Copy link

ivy-lli commented Sep 24, 2021

Hi all,
I just recognized that if you try to resize a node and you have the GridSnapper enabled the behavior is really strange because the mouse pointer will get out of sync with the resize handle.

With GridSnapper:
resize-bug-with-gridsnapper

Without GridSnapper:
resize-bug-without-gridsnapper

Sadly you the move cursor is not really visible here in the gif files but you can see the final cursor positions.

Reproduce:

  • Enable the resizeFeature on the workflow-glsp examples TaskNode:
    • Open the glsp-client/examples/workflow-glsp/src/di.config.ts and remove the modelHintModule
    • Open the glsp-client/examples/workflow-glsp/src/model.ts and add the resizeFeature to the TaskNode DEFAULT_FEATURES
  • Start the glsp-workflow standalone demo

To disable the GridSnapper, I just removed the binding of it inside the di.config.ts file.

Tested in Chrome and Firefox.

Kind regards

Lukas

@tortmayr tortmayr added bug Something isn't working client labels Sep 24, 2021
@ivy-lli
Copy link
Author

ivy-lli commented Sep 24, 2021

I think it has something to do with the DragAwareMouseListener. Because in the ChangeBoundsListener#mouseMove method is always the element which is currently hovered as target. This would result in wrong calculations of the GridSnapper if this one uses e.g the element.bounds to check the snapping. You can also see this if you move your cursor very slowly while resizing the element. If you do so the behaviour seems to be correct.

But this is only a guess 🙂

@planger
Copy link
Member

planger commented Sep 24, 2021

It may be helpful for debugging to disable the CSS rule that hides the mouse pointer. I think I remember that @martin-fleck-at already mentioned an issue in this matter, which was the reason why the mouse pointer is set to none in the first place, but I don't know the details anymore.

@planger
Copy link
Member

planger commented Sep 24, 2021

Now when I try to remember a bit more, I think this was a conceptual issue with the snapper.
If the user moves the mouse three pixels up, the snapper prevents the resize handle to move up, because it didn't snap yet. If -- after moving up three pixels -- the user moves the mouse down, we start counting from 0 before the snap happens, instead of starting to count from -3. Otherwise the user would have to move ~ 13 pixels before the snap happens, which felt weird at the time this was written. But I might be wrong.

@martin-fleck-at
Copy link
Contributor

@ivy-lli Hi, you are absolutely right in that this is a bit of an usual behavior. For reasons that Philip mentioned already we initially chose another approach and decided to hide the mouse pointer to avoid confusion. However, the more common solution seems to be to properly consider the snapped position and ensure that the mouse pointer and the resize handle are in sync. I opened a PR to fix that with eclipse-glsp/glsp-client#144:

resize-snap

The nice thing is that now we can also show proper cursor feedback for moving and resizing.

@martin-fleck-at
Copy link
Contributor

@ivy-lli I'm closing this task for now since the PR has been merged. If you have any follow-up issues please let us know. Thank you!

@ivy-lli
Copy link
Author

ivy-lli commented Oct 8, 2021

Hi @martin-fleck-at,
Thanks for your fix, looks great! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working client
Projects
None yet
Development

No branches or pull requests

4 participants