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 #1066

Merged
merged 2 commits into from Sep 29, 2017

Conversation

rampion
Copy link
Contributor

@rampion rampion commented Aug 7, 2013

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

Attribute namespaces can be defined by `xmlns:<i>prefix</i>="namespace-url"`
attributes, and allow all descendents of that element to define
`<i>prefix</i>:name="value"` attributes.
The DOM provides a way to lookup which namespace an attribute name belongs to
given the element the attribute will be bound to, Node.lookupNamespaceURI
(http://www.w3.org/TR/DOM-Level-3-Core/core.html#Node3-lookupNamespaceURI).

Given the namespace, it's simple to add/remove the desired attribute with the
correct namespace using the Node.setElementNS and Node.removeAttributeNS.
].join('');

ko.applyBindings(model, testNode);
var anchor = testNode.childNodes[0]/*svg*/.childNodes[0]/*g*/.childNodes[0]/*a*/;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I've heard this is a bit unclear:

 testNode.childNodes[0].  // the <svg> is the first child of the testNode
          childNodes[0].  // the <g> is the first child of the <svg>
          childNodes[0];  // the <a> is the first child of the <g>

I was trying to use the inline comments to make this more clear, but evidently that backfired :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks a bit like intermixing weird XPath syntax and JS, the missing syntax highlighting doesn't help either. :)

@mariusGundersen
Copy link
Contributor

I too need this feature. I'm in the exact same situation where I need xlink:href to work.

@troyschneringer
Copy link

+1

@krnlde
Copy link

krnlde commented Feb 18, 2016

It's been two, almost three years, since that feature was requested. What is happening? Why is this PR not merged yet? Big bump!

@brianmhunt
Copy link
Member

Submit a PR and I'll merge this against tko.binding.core/src/attr.js

@mbest mbest added this to the 3.5.0 milestone May 7, 2017
@rampion
Copy link
Contributor Author

rampion commented May 8, 2017

@brianmhunt I would love to, but I can't seem to run tko.binding.core's tests right now:

root@5b0632d0c3f6:~/tko.binding.core# gulp test --once
[00:53:48] Using gulpfile ~/tko.binding.core/gulpfile.js
[00:53:48] Starting 'test'...
08 05 2017 00:53:48.215:WARN [watcher]: Pattern "/root/tko.binding.core/spec/helpers/setup.js" does not match any file
.
08 05 2017 00:54:18.191:INFO [karma]: Karma v1.7.0 server started at http://0.0.0.0:7777/
08 05 2017 00:54:18.194:INFO [launcher]: Launching browser PhantomJS with unlimited concurrency
08 05 2017 00:54:18.227:INFO [launcher]: Starting browser PhantomJS
08 05 2017 00:54:18.690:INFO [PhantomJS 2.1.1 (Linux 0.0.0)]: Connected on socket IY4nCyPIs2VTHKBrAAAA with id 7442831
4
PhantomJS 2.1.1 (Linux 0.0.0)  🚨  Error: SyntaxError: Unexpected token 'const'
at http://localhost:7777/base/spec/attrBehaviors.js?48d2f00b2ac970646673804383217f610e500667:3919
PhantomJS 2.1.1 (Linux 0.0.0) ERROR
  SyntaxError: Unexpected token 'const'
  at spec/attrBehaviors.js:3919


 🏁  Run complete.
...

I'm not sure what in the tko.binding.core configuration chain is causing const to be emitted in the code, but AFAIK, PhantomJS doesn't support ES6, so this is breaking my ability to run tests.

Tell me how I can test my PR, and I'll gladly submit it.

@brianmhunt
Copy link
Member

Thanks @rampion – I just broke the testing – we're moving to a monorepo (at knockou/tko) so there are a few hitches, like this, as I nail tooling down.

You can either checkout from a few days ago (where testing should work) or give me a couple days and I'll have it working – it'll be in the monorepo at github.com:knockout/tko's packages/tko.binding.core directory.

Please ping me if I haven't gotten back by next week! Thanks @rampion

@krnlde
Copy link

krnlde commented Jun 10, 2017

Can I help somehow?

@mbest mbest merged commit 0b575f8 into knockout:master Sep 29, 2017
@mbest
Copy link
Member

mbest commented Oct 1, 2017

Turns out some browsers (Chrome) don't like it when you use setAttributeNS(null,.. instead of setAttribute.

mbest added a commit that referenced this pull request Oct 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants