Draggable doesn't clean up inline style

[monokee]

Hey guys,

I feel like calling kill() on a Draggable instance should probably clean up after itself and reset the DOM element it was invoked on to it's pre-initialization state.

Currently it leaves 'user-select: text' on the inline style attribute of each element, overwriting any default user-select settings that may have been previously defined (inline or css).

 

See line 903 in Draggable.js

_setSelectable = function(elements, selectable) {
	var i = elements.length,
	e;
	while (--i > -1) {
		e = elements[i];
		e.ondragstart = e.onselectstart = selectable ? null : _emptyFunc;
		_setStyle(e, "userSelect", (selectable ? "text" : "none"));
	}
}

 

I can of course manually clean up the style in my implementations (which is what I currently do) but I think it would be much cleaner if Draggable would keep an internal reference to any style properties it is going to modify and reset the element to that reference after the Draggable instance is killed.

 

Sorry for being nitpicky. Keep up the great work!

Jonathan

[GreenSock]

I totally see what you mean, Jonathan. In some ways, it makes perfect sense. Here's the thing, though: it wouldn't seem intuitive (to me at least) if we cleaned up all inline styles because that would likely, for example, suddenly move the element to a completely different position (transform). And it seems a little odd to only reset parts of the inline styles. See what I mean? Some users may complain "wait, why does Draggable.kill() remove some inline styles but not others?"

 

I definitely appreciate the suggestion. I'll keep it in mind moving forward. And if many others echo this request, it'll help show me that it's a pain point worthy of risking the unintuitive "partial inline cleanup" strategy. 

[monokee]

Thanks Jack! Makes perfect sense in that light, especially if that's how people have been using the kill() method. I was expecting it to completely reset everything but I see that that's tricky given the conversion to transform-based positioning etc.

 

Still, from a (read: my) design standpoint there should be some public method that completely resets any mutations on elements not directly created/defined by a controller like Draggable. In non-trivial applications the DOM element that can be dragged may also be controlled by other plugins and if they all leave properties behind on elements they don't directly own you're quickly in a situation where you have to clean up plugin internals that you shouldn't really have to care about.

 

Maybe a future version of kill() could simply accept a boolean (with default false) that indicates whether the element should be reset completely... My two cents.  

[OSUblake]

If I'm killing a draggable instance, that usually means I'm also removing the element, so this has never been a problem for me. The only thing I expect is that any references will be cleared so the object can be removed memory.

 

That's not to say you're wrong, but I honestly can't think of a single plugin that returns something back to it's previous state when you kill it.

 

[monokee]

1 hour ago, OSUblake said:

If I'm killing a draggable instance, that usually means I'm also removing the element

If the sole purpose of the element is to be permanently draggable this makes perfect sense. What I'm saying is that dragability shouldn't have to be its sole purpose and probably isn't in any highly interactive UI.

If I'm calling draggable.kill() I want to kill the the draggable functionality, not the element it controlls.

 

Quote

I honestly can't think of a single plugin that returns something back to it's previous state when you kill it.

Do you think that it's good practice for plugins to leave ghost properties on objects or elements after their business is officially over?

It quickly becomes a debugging nightmare in large scale applications where every plugin does that (and I agree with you that not many plugins do that - which is why I'm here to annoy you guys as plugin authors!).

 

I'm not saying change the default kill behaviour. I get Jacks points about inline positioning and people having warmed up to it's current behaviour. I'm asking, specifically for Draggable to not add 'user-select: text' on my element after I kill it. It's not what I want, it's not what I asked for and it shouldn't be there if that's not how I set up my element in the first place. And a way to generally and completely.. well.. kill it... without ghosts... I don't know.

I guess I'm asking for a BURN method. That would be

 

[OSUblake]

OK, I see your point. I would also have to agree that Draggable should clean up userSelect on kill. It removes other stuff it adds like the cursor.

[monokee]

It also leaves a _dCache object on the parent element (dom node).