Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Fix node hover when mouse moves from an edge #4027

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Gelio
Copy link

@Gelio Gelio commented Jul 5, 2018

When the mouse enters a node while hovering over an edge previously, the node hover event is not emitted.

Here are the screenshots of this unintended behavior. I have marked the mouse position with a black dot.
screenshot from 2018-07-05 17-00-12
screenshot from 2018-07-05 17-00-14

This PR fixes that issue. It has been also mentioned in #3907, which this PR also fixes.

@@ -653,7 +653,14 @@ class SelectionHandler {
}

if (object !== undefined) {
hoverChanged = hoverChanged || this.emitHoverEvent(event, pointer, object);
const hoveredEdgesCound = Object.keys(this.hoverObj.edges).length;
Copy link
Member

Choose a reason for hiding this comment

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

hoveredEdgesCound -> hoveredEdgesCount

Copy link
Author

@Gelio Gelio Jul 16, 2018

Choose a reason for hiding this comment

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

Good catch, thanks 👍 I have fixed the typo

@micahstubbs
Copy link

micahstubbs commented Sep 3, 2018

@yotamberk could you tag this one as network when you have a moment?

@micahstubbs
Copy link

@Gelio thanks for the sending this PR with the fix!

Curious, could you provide a new example that we can use to verify the fix, or point us to an existing example that shows the bug behavior (and would also show the fix)?

https://github.com/almende/vis/tree/master/examples/network

said another way, could you provide steps to test this PR?

@Gelio
Copy link
Author

Gelio commented Sep 4, 2018

@micahstubbs Sure thing

events/interactionEvents.html

  1. Open the console
  2. Set the filter to hoverNode (so there are no unnecessary logs)
  3. Hover over an edge
  4. Move the mouse towards a node (while hovering an edge)
  5. Enter the node, still hovering an edge

The hoverNode event will not be emitted. It is emitted when the mouse enters the node from empty canvas space, not from an edge

@micahstubbs
Copy link

micahstubbs commented Sep 4, 2018

hi @Gelio, thanks for the steps to test.

I'm not able to reproduce the bug behavior at events/interactionEvents.html

here's a gif of my repro session:
vis-pr-4027-before

I'm don't see the extra hoverNode event when I try these steps on vis.js before this PR locally either 🤔

Is there something that I'm missing?

@Gelio
Copy link
Author

Gelio commented Sep 4, 2018

@micahstubbs It is not the extra hoverNode event. It is the lack of it 😄 Notice that there is no hoverNode event when you enter the node while following the edge

@micahstubbs
Copy link

@Gelio acknowledged 👌

I notice this desired behavior even before I apply the changes in this PR. this makes me wonder if it is already fixed somehow 🤔

@Gelio
Copy link
Author

Gelio commented Sep 5, 2018

@micahstubbs Hmm 🤔 Just to be on the same page - you noticed that there is a hoverNode event emitted when moving the mouse from the edge to the node?

The gif shows a different behavior 🙁 The hoverNode event is missing

I have built the latest master branch and the bug is still there - no hoverNode event is emitted when entering the node while following the edge:

vis no hovernode

What branch did you test it on and noticed the possible bugfix 🙂?

@micahstubbs
Copy link

micahstubbs commented Sep 5, 2018

@Gelio thanks for patiently explaining - I think I was confused before and perhaps understand now 💡

does this sound correct to you?

  1. the desired behavior is to emit a hoverNode event anytime a node is hovered over. (this means if the node is entered from the background or from an edge)

  2. the bug behavior is that when a node is hovered over after the cursor was hovering an edge, the expected hoverNode event is missing.

if 1) and 2) are correct, then I do see the bug on latest master.

@Gelio
Copy link
Author

Gelio commented Sep 5, 2018

@micahstubbs No problem 😄 I am glad I can help

Yup, that sounds just about right. Both 1 & 2 are correct.

I have a custom popup (as a regular HTML component overlayed on the canvas) and it is shown upon hoverNode event. When the user hovers a node having been previously hovering an edge, the popup does not appear because of the lack of the hoverNode event. That is why I thought of it as a bug. Would you agree 🙂?

@micahstubbs
Copy link

ok, now that I understand the bug, let's take another look at the fix 😄

@micahstubbs
Copy link

ok, after I remove the linter/code style changes, this is the substantive change that remains:

screen shot 2018-09-05 at 10 27 13 am

@micahstubbs
Copy link

here's another look at the clean diff https://github.com/visjs-community/visjs-network/pull/29/files

screen shot 2018-09-05 at 10 32 40 am

@micahstubbs
Copy link

✅ looks good to me. I'm able to see the desired behavior after applying the changes in this PR.
when I hover over an edge, and then hover over a node, the hoverNode event is emitted. nice work @Gelio 👍

http://127.0.0.1:8080/examples/network/events/interactionEvents.html
vis-pr-4027-after

@Gelio
Copy link
Author

Gelio commented Sep 5, 2018

@micahstubbs Awesome! Thank you for testing my work so thoroughly 👍

@micahstubbs
Copy link

I've merged this in the community standalone network module project, and published a patch version with the fix up to npm 🎉

visjs-community/visjs-network#29
https://www.npmjs.com/package/visjs-network

@mojoaxel
Copy link
Member

💌 Thanks @Gelio for your contribution!
This pull-request has been merge into visjs/vis-network#22

@mojoaxel mojoaxel added Network visjs APPLIED this PR was merged into a repo of the visjs family labels Jul 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Network visjs APPLIED this PR was merged into a repo of the visjs family
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants