Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support namespaced attributes with default attr binding #27

Merged
merged 3 commits into from
Jun 1, 2017

Conversation

rampion
Copy link
Contributor

@rampion rampion commented Jun 1, 2017

When using knockout to create manage SVG or XML nodes, it is sometimes desirable to have it manage attributes from specific namespaces. For example, links in SVG documents use the xlink namespace. Currently, the attr binding doesn't support attribute namespaces, as can be seen in this fiddle. The hardcoded xlink:href works as a link, while the xlink:href attribute created by knockout doesn't, because it doesn't have the right namespace.

This could certainly be compensated for by creating a attr-ns binding just to handle namespaced attributes, however not only would this have a high degree of code repetition, but this breaks the similarity between hardcoded attribute and attributes defined by attr. The HTML parser is smart enough to figure out which attributes have namespaces, why shouldn't knockout do the same? The DOM provides the Node.lookupNamespaceURI(), Node.setAttributeNS() and Node.removeAttributeNS() methods just for this purpose.

This pull request uses these API functions to add namespace support to the default attr binding. It also adds a test to the spec to check that namespace support is working properly.

ref: http://stackoverflow.com/questions/18083723/how-can-i-get-knockout-js-to-set-the-namespaceuri-for-attributes

ref: knockout/knockout#1066

@brianmhunt brianmhunt merged commit 7255f89 into knockout:master Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants