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

Fix offset when creating node with mouse #7639

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Feb 20, 2024

The border around the viewports caused a slight offset when creating a node (probably, other interactions were also impacted). Debugging showed that the mouse move listeners used getBoundingClientRect (which contains the border) to make the clicked position relative. However, the remaining code assumes that the border has to be ignored when interacting with the div. For example, makeRectRelativeToCanvas subtracts the border after calling getBoundingClientRect. I think, it makes sense to ignore the border altogether. However, doing this in input.ts would mean to dynamically read the border and subtract it there.
I figured the easiest way was to move the border simply somewhere else so that the targeted viewport divs don't have any border.

@daniel-wer I suggest to test this first thoroughly. If you are fine with this, I'd remove OUTER_CSS_BORDER completely, since it shouldn't have any relevance afterwards (hopefully).

By the way, a smell of the current border was also that you could create nodes on the border. With this PR, this is not possible anymore.

URL of deployed dev instance (used for testing):

Steps to test:

  • test that the mouse position is respected precisely when
    • creating, moving, selecting nodes (remember that node positions are integer; so zoom out a bit to get precise positions)
    • brushing
    • moving with alt + mouse move
    • something else?

TODOs:

  • fix brush preview
  • remove OUTER_CSS_BORDER

Issues:


(Please delete unneeded items, merge only when none are left open)

@philippotto philippotto self-assigned this Feb 20, 2024
@daniel-wer
Copy link
Member

The first thing I noticed comparing before and after is that the scale bar got a little wider with this PR:

Before
image

After
image

When setting nodes at the edges of the scalebar and measuring the distance I get the sense that the new one is too wide and the old one is correct. I'll test more thoroughly tomorrow.

@philippotto
Copy link
Member Author

When setting nodes at the edges of the scalebar and measuring the distance I get the sense that the new one is too wide and the old one is correct.

Good catch! I confirm this bug. However, I don't understand it. I removed the CSS border completely and there is still an offset. I suspect that there is another hardcoded border somewhere, but I don't know where 🤔 Other than that, I don't have an explanation right now. I'll look into this again tomorrow, too.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nice find and fix! I'm glad that we can get rid of this prone-to-error piece of code 🎉 I didn't notice any issues during testing.

@philippotto
Copy link
Member Author

Nice find and fix! I'm glad that we can get rid of this prone-to-error piece of code 🎉 I didn't notice any issues during testing.

Great, thanks for testing! I noticed that there is still a slight change of the scalebar width (compared to master), but it's only 1px and it's hard to say whether the old or the new width is correct. Since the new code is way simpler, I'd like to assume that it's now correct =)

@philippotto philippotto merged commit c32bc1b into master Feb 21, 2024
2 checks passed
@philippotto philippotto deleted the fix-node-creation-offset branch February 21, 2024 13:55
This was referenced Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants