Hareeshchandera / jsplumb

Automatically exported from code.google.com/p/jsplumb
0 stars 0 forks source link

'drawEndpoints: false' causes an error with connection creation (no offsetParent) #114

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What steps will reproduce the problem?
1. Attempt to use 'jsPlumb.connect' with 'drawEndpoints: false'

What version of the product are you using? On what browser and OS?

1.3.1 on Firefox 5.

Please provide any additional information below.

See http://jsfiddle.net/hT7zC/ . In this case, 'element.canvas' doesn't have an 
offsetParent (since it's hidden; see 
https://developer.mozilla.org/en/DOM/element.offsetParent).

I'm not really sure how to fix this; the only way to be able to get the offset 
may be to briefly show the canvas?

Original issue reported on code.google.com by paul.uit...@gmail.com on 27 Jul 2011 at 3:16

GoogleCodeExporter commented 8 years ago
Btw, I've added an explicit offsetParent in the example at 
http://jsfiddle.net/hT7zC/6/ , to show that just fixing the check to include 
(element.canvas && element.canvas.offsetParent) doesn't cut it ;). The 
connector will end up being positioned relative to the body in that case..

Original comment by paul.uit...@gmail.com on 27 Jul 2011 at 7:16

GoogleCodeExporter commented 8 years ago
this is related to issues 108, 110 and 116...all the result of a change i made 
in 1.3.0 to start appending canvases to their offsetParent (prior to 1.3.0, 
jsPlumb added everything to the body).

one suggestion i thought of was to try the 'Blank' endpoint:

http://jsfiddle.net/sporritt/Pcqub/

but because it doesn't paint anything it doesn't take offsetParent into account 
and so the connector ends up in the wrong place - another bug i need to fix.

Original comment by simon.po...@gmail.com on 2 Aug 2011 at 12:35

GoogleCodeExporter commented 8 years ago
can you tell me one thing about your usage: are you intending to have the 
endpoints appear at some stage, or do you set this because you never want to 
see endpoints? i have a feeling that people will either show/hide a whole 
connection (ie. endpoints and connector), but not really hide the endpoints 
alone. it was for that reason that i created the Blank endpoint, but which had 
a positioning bug too.

i have now fixed it so that it now appends a 1x1 transparent div to the 
container, which means that the offsetParent is set correctly and connections 
are positioned properly:

http://jsfiddle.net/hT7zC/6/

(it links to a build of the 1.3.2 dev code on my server)

if you could let me know either way i'd appreciate it.

Original comment by simon.po...@gmail.com on 6 Aug 2011 at 8:31

GoogleCodeExporter commented 8 years ago
Thanks a lot! I'd be happy to explain!

What we're building (part of the application) contains a tree structure, with a 
dependency graph overlaid on top of this structure. A user can build trees 
himself by creating nodes, and organizing these by drawing parent/child 
connections between nodes (0..1 parent per node, 0..* children).

This also contains other functionality like showing/hiding part of a tree (both 
upwards and downwards), and allow the user to create multiple workspaces (tabs) 
containing different (or even parts of the same) tree.

The dependencies are created/calculated based on other data, and can span 
trees. To minimize the visual disturbance (and not give the impression they can 
be manipulated directly), we don't show endpoints for the dependencies. Which 
is where 'drawEndpoints' comes in.

If I wanted to show them at some stage, I'd use the 'setVisible' function of 
the endpoint (I am using that when hiding selected nodes in a tree, since 
'jsPlumb.hide(el)' doesn't appear to hide endpoints associated with 'el'?).

By the way, what's the recommended way to handle 0..* connections to one 
node/endpoint? Right now I'm just using one endpoint with 'maxConnections' set 
to a sensible maximum, but that has some quirks when you get more than one 
connection (especially in combination with draggable elements and 
'dynamicAnchors').

Original comment by paul.uit...@gmail.com on 6 Aug 2011 at 5:50

GoogleCodeExporter commented 8 years ago
thanks for the info.  when you say "if i wanted to show them at some stage", do 
you mean that happens as part of the normal flow of the app, or that you're 
keeping your options open on the UI?  the answer to this has quite a large 
bearing on whether or not it's worth putting more effort into the drawEndpoints 
option!

as i mentioned a few comments ago, the JS error was caused by the same thing 
causing a few other issues, which was that some canvas's offsetParent was null 
when jsPlumb was not expecting it to be. turns out that i had incorrectly 
assumed there would always be an offsetParent, but in reality if an element is 
not visible on screen then its offsetParent is not set.  jsplumb used to attach 
everything it created to the document body so it did not have to mess around 
with offsetParents, but now that it adds things to the parent of the elements 
that endpoints have been attached to (see the documentation for the reasoning 
behind this), it has to adjust things whenever an offsetParent is in play.

for the other bugs reported this incorrect assumption manifested itself when 
someone had a container div that was not yet visible but inside which they were 
calling jsplumb to do things; not a massive problem, it turns out, as you can 
just tell jsplumb to repaint everything whenever you hide/show elements in the 
UI.

but the drawEndpoints case is different. since the endpoint is invisible, it 
does not have an offsetParent, and this means that when jsplumb computes the 
anchor positions for each end of a connection, it will not be able to take into 
account the fact that the origin of the element to which it is appending 
canvases is probably not at 0,0. i don't know, off the top of my head, how much 
work would be involved in making the offsetParent code a little smarter.

jsPlumb.hide is really a relic from the early days of jsPlumb now, and for many 
releases it's just been left in without much thought as to its usefulness.  you 
are right in thinking that it does not hide endpoints, only connections.  but 
i'm a fan of the API adapting to fit requirements, so this method could be 
updated a little if it seemed like a good idea (of course we may still have 
that whole offsetParent issue to deal with if the method offers a way to hide 
endpoints)

lastly, the maxConnections approach you have taken is pretty much what i would 
suggest. i'm interested in the "quirks" you mentioned, though - could you 
elaborate?  i don't suppose one of them is what was raised in issue 109, was it?

Original comment by simon.po...@gmail.com on 7 Aug 2011 at 12:46

GoogleCodeExporter commented 8 years ago
Reading it back, I see that the context of "If I wanted to show them at some 
stage" is a bit vague; I meant that I wouldn't use 'drawEndpoints=false' in 
that case, but use 'setVisible(true/false)'. I'd only use 'drawEndpoints' if I 
never ever wanted to see endpoints for a certain connection.

Originally, I simply tried to use 'drawEndpoints' since it was mentioned in the 
docs, and I needed just that. It might be a bit clearer semantically than using 
'endpoint: "blank"'; but if there's no other (performance) benefit in using 
'drawEndpoints' (since it still does need to create and draw endpoints, albeit 
secretly), it may be better to drop it and support just the second type.

Regarding using offsetParent: yeah, I agree this makes for a nicer 'out of the 
box' experience, although the 'container' option worked fine for me before as 
well; glad to have it back as a backup.

Regarding jsPlumb.show/hide: I don't know how representative my use is, but I 
find myself performing the following scenarios most often. On hide, do 
'jsPlumb.hide' to hide the connections, then hide each of the element's 
endpoints:

    jsPlumb.hide( this.el );
    for each endpoint: endpoint.setVisible( false, false, true );

On show, set each endpoint to visible, then check for each connection if the 
opposite element/endpoint is actually visible, and show the connection if it is:

    for each endpoint: endpoint.setVisible( true, true, true );
    for each connection: targetEndpoint.isVisible() && sourceEndpoint.isVisible() && connection.setVisible( true );

It seems to me these are fairly typical scenarios (when hiding an element, also 
hide it's endpoints and connections; when showing an element also show it's 
endpoints plus any connections for which the opposite endpoint is visible as 
well). I don't think these would cause offsetParent issues, since connections 
aren't shown if either of their endpoints is hidden. If you'd want to support 
other variations in which connections can be shown while one of their endpoints 
is hidden, you would get  offsetParent trouble though ;).

I'll open a separate issue (with test-case) for the maxConnections + dynamic 
anchors; it's not the same as 109. Basically, it looks as if just the 'first' 
connection controls the placement of the endpoint on one of the dynamicAnchors; 
and the other connections don't update to move to the new location of the 
endpoint if it moves to another anchor.

Original comment by paul.uit...@gmail.com on 7 Aug 2011 at 9:33

GoogleCodeExporter commented 8 years ago
ok, so here's what i think i will do:

- add a second option to jsPlumb.hide, a boolean, that instructs jsPlumb to 
hide all the Endpoints for the element as well as its connections. under the 
hood it will do as you have in your example: endpoint.setVisible(false, false, 
true);

- add a second option to jsPlumb.show, a boolean, that instructs jsPlumb to 
show Endpoints as well as connections.  in order to keep backwards 
compatibility, if you call jsPlumb.show with only one argument (the element), 
then it will behave exactly as it does now.  but if you include the second 
argument i will run the logic that you have outlined: show each connection if 
the opposite element/endpoint is visible.

as for 'drawEndpoints'...not sure really.  my feeling is that i should remove 
it altogether, since Blank endpoints do the same thing, and any usage of it 
clearly risks that offsetParent bug appearing.

i'll do this stuff for version 1.3.2.  if you could raise that other issue it'd 
be great: i will try to get that in to 1.3.2 also.

Original comment by simon.po...@gmail.com on 8 Aug 2011 at 3:41

GoogleCodeExporter commented 8 years ago
Yeah, that would be a great addition I think!

I agree the 'blank' endpoint is probably the way to go, since it looks like 
'drawEndpoints=false' cannot really be implemented like it's name suggests.

Original comment by paul.uit...@gmail.com on 9 Aug 2011 at 8:27

GoogleCodeExporter commented 8 years ago
yep, unless you're using the document body as the container, 
drawEndpoints:false is always going to be a little risky.

i'll make those changes.  could you maybe raise that other issue you mentioned? 
i'd like to get 1.3.2 out the door in the next few days...

Original comment by simon.po...@gmail.com on 9 Aug 2011 at 8:41

GoogleCodeExporter commented 8 years ago
Yep, working on a test case.

Original comment by paul.uit...@gmail.com on 9 Aug 2011 at 8:46

GoogleCodeExporter commented 8 years ago
ok i've made these changes to hide and show now. drawEndpoints is still in, but 
i've updated both the api docs and the main documentation to explain the perils.

i'm gonna mark this one fixed, because, well, something got fixed at least!

Original comment by simon.po...@gmail.com on 9 Aug 2011 at 11:59

GoogleCodeExporter commented 8 years ago
as discussed - marking fixed.  but i am aware that the original problem did not 
end up getting fixed.  it got talked about a lot, and it got documented, but it 
did not, strictly speaking, get fixed ;)

Original comment by simon.po...@gmail.com on 11 Aug 2011 at 12:04

GoogleCodeExporter commented 8 years ago

Original comment by simon.po...@gmail.com on 11 Aug 2011 at 3:43