Node stops obeying widthConstraint after hover / blur

[nikivanov]

I've created a small jsfiddle that shows the issue:

https://jsfiddle.net/re61t898/13/

Initially, the graph is rendered correctly. However, once you hover over the second node and then blur it, the node will lose its width constraint. Curiously enough, if you hover over it again, it will get it back, but lose it again once you blur it.

[jessiesanford]

Also experiencing this problem. No idea how to remedy the issue...

[moppius]

I'm also running into this. Gonna take a look at the code and see if I can find the root cause...

[A-Fredette]

I'm running into this as well. Hopefully we can find a solution. @moppius did you find anything in the library?

[moppius]

I did a bit of debugging and it looks like the issue is a difference between the results returned from measureText in the overMaxWidth method in LabelSplitter.js line 443, depending on whether the node is in a hover state or not.

The measurements returned for the same text string return much lower values when the node is not in hover state (only after the first hover, though - so it must be initialized correctly). This leads the LabelSplitter to think that the text is much narrower than it really is when deciding how to split up the line, so it puts line breaks in the wrong place.

Context Problem

The root of the problem is that the context's font value is not being set correctly before the label is measured.
In the case where the width is correct in the supplied JSFiddle, this.ctx.font is "25px tahoma", and then when the node is no longer hovered, this.ctx.font is "10px sans-serif", which explains why the measurement is entirely different.

I'll keep looking to see if I can find out where the context is changing for this.

Update

A bit closer to the problem, it looks like when the mouse is over the node, the shape class's resize method is called first for _drawEdges (and ctx.font is incorrect for the label at this point), but then resize is called again from the Node's own draw method called from _drawNodes (and ctx.font is correct in this second call).
However, when the mouse is not hovering over the node, only the _drawEdges method makes a call to the node shape's resize method, with the same incorrect context, and resize is never called again, leading to the shape node having the incorrect size.

Update 2

I'm starting to get a headache now, but I think the problem is in the logic of NodeBase's needsRefresh method. Every time the mouse hovers over the node, needsRefresh returns true because the state of the node has changed (hover is true, previously was false). This means that the first call to resize with the incorrect context (from _drawEdges) is overwritten by the next call to resize with the correct context (from _drawNodes).
However, when the mouse moves off the node, needsRefresh returns false for some reason, even though the state is changing from hover=true to hover=false. This means that the first incorrect resize call is never overwritten because the _drawNodes call decides that the node doesn't need to be refreshed.

Update 3 - workaround/hack fix

I've managed to "fix" it by making the Label's differentState method always return true.

I suppose the problem might be that the Node's hover value is being reset too soon?

However, I feel like the real issue might be that any Node's resize method is being called from the _drawEdges method at all?
This seems an odd place for it to be called, since that code is drawing edges, it shouldn't be changing Node properties!

Update 4 - suspicious things

OK, a better "fix" for this issue (although probably introduces other issues elsewhere) is to remove all the calls to this.resize(ctx) from the distanceToBorder method NodeBase and every Shape class that overrides it.
Generally I don't like that the distanceToBorder method (which sounds like it should just be calculating something) is actually resizing the node at the same time.
This is problematic because the resize method calls needsRefresh from NodeBase, which sets the Node's internal refreshNeeded flag to false, so after the first resize call (with the wrong context!), the Node thinks that it doesn't need to be refreshed any more, so the next call to resize (with the correct context!) won't do anything!

Deleted commit

I committed a fix yesterday but it was not a good one. See newer comments for more recent fix.

[moppius]

Hmm, this seems to be fundamentally complicated by the architecture of drawing Edges before drawing Nodes.
Because the Node sizes haven't been evaluated yet for the current frame (since that happens in _drawNodes, which is called after _drawEdges in order to make Nodes draw on top of Edges), it means that in order to place arrows and other Node-size-dependent features on Edges, Node sizes needs to be calculated before Edges are drawn.

I can see a few ways of fixing this:

1. Leave architecture unchanged, fix up individual calls to resize so it doesn't invalidate future resize calls. I've already started doing this, but it "feels" wrong...
2. Do an overall Node resize calculation before edges are drawn, and remove all calls to resize from distanceToBorder. This likely introduces some extra performance overhead due to updating (but not drawing) all Nodes before drawing all Edges.
3. Actually draw Nodes first, before Edges, and change the Edge-drawing methods to only link to the attachment points at the edges of Nodes instead of drawing all the way to the Node center (as far as I can see, this is the only reason why Edges are drawn first - so the tangle of wires meeting at the center point is hidden behind the node itself)

I don't want this to become a massive re-architecture project just to fix one bug, but I will investigate each of these options more to see what makes the most sense.

[moppius]

OK, I've got a slightly less invasive fix for now, that fixes all the cases I can see.

I reverted my previous change since it was changing behaviour in other places, and instead I just set the font size and face in the process call of the LabelSplitter, as this is where the problem lies. When the LabelSplitter class is initialized, it has the wrong context, and the context is never set with the correct font until after the label string has already been split!

So my new change just ensures that ctx.font is at least mostly correct before the line is split, although it doesn't take into account any modifiers for the font since we don't have easy access to them at this point (although I'm sure that's fixable with a bit more work).

So yeah, this new commit should fix the issue in all known cases if anyone wants to try it out! 😅

[A-Fredette]

thanks for working on that @moppius, i'm going to try to implement your fix. i'll let you know if i have anything to add to it!

[A-Fredette]

seems to be working great, thanks again @moppius

[moppius]

This looks like a duplicate of #3872 .