fixed namespaced attributes not updating #125

AndyOGo · 2018-04-18T18:48:09Z

fixes #124

Using SVG sprites with <use xlink:href="" /> will not update the icon.

The reason is that getAttributeNS and setAttributeNS DOM APIs are inconsistent in it's arguments.
E.g.: lets say 'foo:bar' ('foo' being the namespace and 'bar' the local attribute name):

getAttributeNS only needs the localName, i.e. getAttributeNS('https://www.w3.org/1999/foo', 'bar');

getAttributeNS(namespace, localName)
https://www.w3.org/TR/DOM-Level-2-Core/glossary.html#dt-localname
A local name is the local part of a qualified name. This is called the local part in Namespaces in XML [Namespaces].
BUT setAttributeNS expects it's fully qualified name, i.e. setAttributeNS('https://www.w3.org/1999/foo', 'foo:bar');

setAttributeNS(namespace, qualifiedName, value)
https://www.w3.org/TR/DOM-Level-2-Core/glossary.html#dt-qualifiedname
A qualified name is the name of an element or attribute defined as the concatenation of a local name (as defined in this specification), optionally preceded by a namespace prefix and colon character. See Qualified Names in Namespaces in XML [Namespaces].

AndyOGo · 2018-04-19T17:49:47Z

@patrick-steele-idem
May I asked you to look into this as soon as you have some free time?
I'm happy to get your feedback:)

AutoSponge · 2018-04-20T12:33:16Z

@AndyOGo is it possible to demonstrate this with a test (one that fails under current version but passes under proposed)?

I know @patrick-steele-idem changed jobs recently, so he may not have the time to look into this.

AndyOGo · 2018-04-20T12:53:01Z

@AutoSponge
Thank you for your answer.

Well, the thing is that, I faced this issue in Firefox at a customer of mine for axa-ch-webhub-cloud/pattern-library#411.
Though we are using nanomorph at above project, I figured since they borrowed a few bits from morphdom, may the same issue exists here. And indeed morphdom's code is identical.

Here are the copies from my screencasts (but baked by nanomorph):

Latest Chrome:

Latest Firefox:

May I ask, aren't the links I provided form the official specs making the point clear enough?
For me it's obvious that the official API specification is inconsistent and there is the real bug, but I can't change that. We could only change the used API calls 😊

And I supplied the same Bugfix to nanomorph

AndyOGo · 2018-04-20T12:55:56Z

PS. The reason this works in Chrome, is that SVG 2 drop <use>'s xlink:href in favour of href...

AndyOGo · 2018-04-20T13:11:22Z

PPS: I also updated the related MDN article here:
https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttributeNS

AutoSponge

This PR needs a test which can be run cross-browser to ensure it will not fail existing functionality. Fixing firefox but breaking IE is not an option.

This also serves to explain to other developers what's expected for a given mutation.

AutoSponge · 2018-04-20T16:36:24Z

src/morphAttrs.js

@@ -16,9 +16,15 @@ export default function morphAttrs(fromNode, toNode) {
        attrValue = attr.value;

        if (attrNamespaceURI) {
-            attrName = attr.localName || attrName;


We use || attrName because IE doesn't seem to support .localName see

Thanks for your feedback, very appreciated.
About which version of IE are we speaking? (IE9 supports localName, see).
Microsoft officially stopped to support old IE versions until version 11

My point is that the setAttributeNS API is called with the wrong argument and I provided the official W3C specifications. May I ask you to study them yourself, look up all the technical definitions in the official DOM API glossary and then decide for yourself if I'm right or wrong.

AndyOGo · 2018-04-20T17:59:19Z

src/morphAttrs.js

+            // ref: https://www.w3.org/TR/DOM-Level-2-Core/glossary.html#dt-localname
+            // ref: https://dom.spec.whatwg.org/#dom-element-setattributens
+            // ref: https://www.w3.org/TR/DOM-Level-2-Core/glossary.html#dt-qualifiedname
+            fromValue = fromNode.getAttributeNS(attrNamespaceURI, attrLocalName || attrName);


@AutoSponge
I just inlined the attrName fallback just for getAttributeNS.

@AndyOGo are you saying these two blocks are different?

current

attrName = attr.localName || attrName; fromValue = fromNode.getAttributeNS(attrNamespaceURI, attrName);

this PR

var attrLocalName = attr.localName; fromValue = fromNode.getAttributeNS(attrNamespaceURI, attrLocalName || attrName);

Because I'm not seeing it. I'm still confused as to exactly what you think this fixes.

No with my last commit I restored the original getAttributeNS function call with a fallback to attrName if attr.localName is not defined.

But the line below using setAttributeNS has changed to never use attr.localName.

AndyOGo · 2018-04-20T19:28:56Z

src/morphAttrs.js

+            // ref: https://www.w3.org/TR/DOM-Level-2-Core/glossary.html#dt-localname
+            // ref: https://dom.spec.whatwg.org/#dom-element-setattributens
+            // ref: https://www.w3.org/TR/DOM-Level-2-Core/glossary.html#dt-qualifiedname
+            fromValue = fromNode.getAttributeNS(attrNamespaceURI, attrLocalName || attrName);
            if (fromValue !== attrValue) {
                fromNode.setAttributeNS(attrNamespaceURI, attrName, attrValue);


@AutoSponge
Since I don't overwrite attrName with attr.localName || attrName anymore, setAttributeNS will always only use the full qualified attribute name, including it's namespace.

AndyOGo · 2018-04-20T19:56:57Z

src/morphAttrs.js

            if (fromValue !== attrValue) {
+                // but setAttributeNS requires the fully qualified name


I also moved my complementary comments to the related API calls

AndyOGo · 2018-04-23T12:33:05Z

@AutoSponge
I really want to suggest you read and understand the execution flow of morphAttrs function at runtime. My contribution is definitely NOT a ~~noop~~.

Before the local variable attrName was conditionally overwritten.
By my contribution this variable is never overwritten, causing proper argument passing to setAttributeNS below.

And please stop trolling and giving me bad reputation, I develop frontend projects since decades, have studied all ECMA script specs starting with version 1 fully by myself.

AutoSponge · 2018-04-23T12:52:01Z

xlink:href is deprecated: https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/xlink:href

Do not use. Without a test case to demonstrate how this affects morphdom, I'm keeping it closed. I've already blocked you but I will report if you continue this harassment and waste of my time.

AndyOGo · 2018-04-23T13:04:11Z

Agreed xlink:href is deprecated and I understand this point.

I'm sorry if you interpret my comments as harassment, they don't meant to be.

My main idea is to help and to suggest an spec-conformant implementation regarding the usage of setAttributeNS. From my viewpoint it's really a general and very detailed bug in morphdom. So I made also sure to provide all official documentation regarding this issue.

Unfortunately cooperation with you is not a perfect match and therefore I can't help morphdom getting better.

AndyOGo added 2 commits April 18, 2018 20:48

fixed namespaced attributes not updating

d5996f4

fixed missing semis

915b90c

AutoSponge requested changes Apr 20, 2018

View reviewed changes

just in case local name is not available

a26daa3

AndyOGo commented Apr 20, 2018

View reviewed changes

movend comments to related api call

3c8e5fa

AndyOGo commented Apr 20, 2018

View reviewed changes

AutoSponge mentioned this pull request Apr 23, 2018

fixed updating namespaced attributes is broken choojs/nanomorph#102

Open

AutoSponge closed this Apr 23, 2018

Repository owner deleted a comment from LucaMele Apr 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed namespaced attributes not updating #125

fixed namespaced attributes not updating #125

AndyOGo commented Apr 18, 2018 •

edited

AndyOGo commented Apr 19, 2018

AutoSponge commented Apr 20, 2018

AndyOGo commented Apr 20, 2018

AndyOGo commented Apr 20, 2018

AndyOGo commented Apr 20, 2018

AutoSponge left a comment

AutoSponge Apr 20, 2018

AndyOGo Apr 20, 2018

AndyOGo Apr 20, 2018

AutoSponge Apr 20, 2018

AndyOGo Apr 20, 2018 •

edited

AndyOGo Apr 20, 2018

AndyOGo Apr 20, 2018

AndyOGo commented Apr 23, 2018

AutoSponge commented Apr 23, 2018

AndyOGo commented Apr 23, 2018

		if (fromValue !== attrValue) {
		// but setAttributeNS requires the fully qualified name

fixed namespaced attributes not updating #125

fixed namespaced attributes not updating #125

Conversation

AndyOGo commented Apr 18, 2018 • edited

AndyOGo commented Apr 19, 2018

AutoSponge commented Apr 20, 2018

AndyOGo commented Apr 20, 2018

AndyOGo commented Apr 20, 2018

AndyOGo commented Apr 20, 2018

AutoSponge left a comment

Choose a reason for hiding this comment

AutoSponge Apr 20, 2018

Choose a reason for hiding this comment

AndyOGo Apr 20, 2018

Choose a reason for hiding this comment

AndyOGo Apr 20, 2018

Choose a reason for hiding this comment

AutoSponge Apr 20, 2018

Choose a reason for hiding this comment

AndyOGo Apr 20, 2018 • edited

Choose a reason for hiding this comment

AndyOGo Apr 20, 2018

Choose a reason for hiding this comment

AndyOGo Apr 20, 2018

Choose a reason for hiding this comment

AndyOGo commented Apr 23, 2018

AutoSponge commented Apr 23, 2018

AndyOGo commented Apr 23, 2018

AndyOGo commented Apr 18, 2018 •

edited

AndyOGo Apr 20, 2018 •

edited