patrick-steele-idem / morphdom

Fast and lightweight DOM diffing/patching (no virtual DOM needed)
MIT License
3.21k stars 129 forks source link

Removed xlink:href atribute in svg on v1.4.1 #63

Closed reenko closed 8 years ago

reenko commented 8 years ago

On version 1.4.1 I have a problem with patching svg. In DOM:

<svg class="icon icon--pin">
    <use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#icon-pin" class="js-icon"></use>
</svg>

Patching remove xlink:href attribute.

I create test:

    it('should transform svg', function() {
        var el1 = document.createElement('div');
        el1.innerHTML = '<svg xmlns="http://www.w3.org/2000/svg" class="icon icon--search"><use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#icon-search" class="js-icon"></use></svg>';

        var el2 = document.createElement('div');
        el2.innerHTML = '<svg xmlns="http://www.w3.org/2000/svg" class="icon icon--search"><use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#icon-search" class="js-icon"></use></svg>';

        morphdom(el1, el2);
        expect(el1.innerHTML).to.equal('<svg xmlns="http://www.w3.org/2000/svg" class="icon icon--search"><use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#icon-search" class="js-icon"></use></svg>');
    });

Failed:

-<svg xmlns="http://www.w3.org/2000/svg" class="icon icon--search"><use class="js-icon"></use></svg>
+<svg xmlns="http://www.w3.org/2000/svg" class="icon icon--search"><use xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="#icon-search" class="js-icon"></use></svg>
shock01 commented 8 years ago

https://developer.mozilla.org/en-US/docs/Web/API/Node/localName localName is actually mentioned as obsolete. No alternatives are offered at the moment. It's only obsolete on DOMNode and not on DOMAttr and DOMElement. DOM Level 4 moved the property to Element and Attr

Basically if you set the namespaces correct the nodeName/name property will be presented without the namespace prefix (not fully qualified). So something must be going wrong in the implementation of morphdom regarding setting the attributes. I suspect the attributes are set like: xlink:href causing the attribute to not be correctly namespaced and non-functional

shock01 commented 8 years ago

The following code is also incorrect

if (fromValue !== attrValue) {
            if (attrNamespaceURI) {
                fromNode.setAttributeNS(attrNamespaceURI, attrName, attrValue);
            } else {
                fromNode.setAttribute(attrName, attrValue);
            }
        }

First it should be checked if the attribute has the same name and namespace. And after that it should verify attribute values.

Compare:

<div  xmlns:a="1" xmlns:b="2" >
<div a:test="2" b:test="1"></div>
</div>

Morph with:

<div xmlns:a="2" xmlns:b="1">
<div a:test="2" b:test="1"></div>
</div>

What will happen if we only compare nodeName and not the namespaceURI??

I think it will update the wrong attribute. However it's an edge case

Expected:

<div a:test="1" b:test="2"></div>
Actual:
<div a:test="2" b:test="1"></div>
patrick-steele-idem commented 8 years ago

I looked into this issue and found using attr.localName (if available) as the best solution.

New version published with fix: morphdom@1.4.2

Thanks for reporting the problem! Please let me know if you still see issues.

reenko commented 8 years ago

Thank you for fixing this. It's work for me! I tested on MacOS(Safari/Chrome/FF), Ubuntu(Chrome/FF), Windows (IE11) and mobile devices(iOS/BlackBerry).