Inconsistent behavior overriding jquery.attr in svgcanvas

[GoogleCodeExporter]

I ran into a problem today with the way svg-edit monkey patched jquery.attr. It 
turns out that, at least in jquery 1.7.1, the correct return value for attr in 
the case that 'this.length' evaluates to false is undefined and not 'this'. I 
cant tell if this was an issue introduced by upgrading to 1.7.1 but I can tell 
you that at least one plugin I was using failed to work correctly because of 
this issue.

Generally speaking it seems very dangerous to override the behavior of core 
jQuery methods like attr because they are used by every plugin everywhere and 
issues can be very hard to track down due to the general trust put in jQuery 
core functionality to 'just work'.

I think there are three possible solutions to this problem, one of which I have 
attached a patch for:

1) Instead of using jQuery's attr method can we switch to using something like 
svgAttr which can delegate to jQuery when necessary but will only be invoked in 
the cases it is needed instead of overriding the core behavior.

2) Following on the above suggestion , roll up the svg support to create a 
jQuery plugin that can plug-in to the proper jQuery mechanism and can collect 
all svg related fixes into a single place.

3) In the short term we should proxy any request to jQuery that the overriding 
method isn't prepared to handle. This will keep us in sync with jQuery as long 
as the API stays the same. See attached for the patch.

Original issue reported on code.google.com by adamben...@gmail.com on 9 Mar 2012 at 9:23

Attachments:

• 0001-fixed-inconsistent-handling-of-empty-selector-in-jqu.patch

[GoogleCodeExporter]

I don't understand why SE try this hackish method in the first place. It 
doesn't even try to be consistent with proxied method (eg. return Number 
instead of string).

jQuery does not support namespaced .attr() yet, and I do believe all plugins 
should use native *.setAttributeNS / *.getAttributeNS.

Original comment by asyazwan on 10 Mar 2012 at 9:45

[GoogleCodeExporter]

I don't have time to look into the hisotry of this and figure out what this 
does, unfortunately.

asyazwan - can you clarify whether you think this patch should be applied?  Is 
there a better way to fix this so that svg-edit is consistent and doesn't 
override jquery behavior?

Original comment by codedr...@gmail.com on 17 Mar 2012 at 5:51

[GoogleCodeExporter]

Patch applied in r2066.

Jeff - I think a re-implementation of this will require thorough testing to 
make sure it doesn't break anything. As such at the moment it's probably best 
to leave it be. From what I can see the only custom feature this function uses 
is passing an array of attributes. It is a bad idea in the first place to 
override jQuery core function. Should've just created a utility function or 
something...

Original comment by asyazwan on 22 Mar 2012 at 7:49

• Changed state: Fixed

[GoogleCodeExporter]

Has anyone looked at this library: http://keith-wood.name/svg.html? I ran into 
another problem with jquery and svg today that this library looks like it would 
help with. Perhaps it is a more complete implementation of of the functionality 
we need.  

Original comment by adamben...@gmail.com on 26 Mar 2012 at 11:07

[GoogleCodeExporter]

What is the point? It's a big nice SVG solution but it boils down to basic 
drawing, which I think we have already implemented. Most of our problems come 
from non-basic drawing and interopability (import, export (which is something 
even the plugin don't get right)).

If you're talking about how it handles .attr() and friends, he's also 
overriding jQ's core which is something I'm highly against (although he's doing 
it more cleanly).

Take this code for example:
/* Support removing attributes on SVG nodes. */
$.fn.removeAttr = function(origRemoveAttr) {
    return function(name) {
        return this.each(function() {
            if ($.svg.isSVGElem(this)) {
                (this[name] && this[name].baseVal ? this[name].baseVal.value = '' :
                    this.setAttribute(name, ''));
            }
            else {
                origRemoveAttr.apply($(this), [name]);
            }
        });
    };
}($.fn.removeAttr);

He's using default jquery for everything non-svg, no attempt to return custom 
values that might cause confusion like SE did. Although I still think we should 
stop overriding jquery functions and use utility function with proper use of 
*AttributeNS() as the spec suggests.

Let's bring this discussion to the groups if you want to further it: 
https://groups.google.com/forum/?hl=en&fromgroups#!forum/svg-edit

Thanks.

Original comment by asyazwan on 27 Mar 2012 at 2:35