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

fixed updating namespaced attributes is broken #102

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AndyOGo
Copy link

@AndyOGo AndyOGo commented Apr 18, 2018

fixes #101

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):

@AndyOGo
Copy link
Author

AndyOGo commented Apr 18, 2018

All tests pass:

1..567

tests 567

pass 567

ok

@AndyOGo
Copy link
Author

AndyOGo commented Apr 19, 2018

@yoshuawuyts
@kristoferjoseph
@bcomnes
May I asked you to look into this as soon as you have some free time?
I'm happy to get your feedback:)

@AutoSponge
Copy link

This same code was submitted to morphdom but is a noop. All it does is rename a variable. I suggest close.

@AndyOGo
Copy link
Author

AndyOGo commented Apr 23, 2018

That previous comment is incorrect.

@AutoSponge does not understand the flow of program execution during runtime and he is trolling my reputation.

@LucaMele
Copy link

LucaMele commented Apr 23, 2018

@AutoSponge how can this only be a variable name change? The magic is on line 24 and 29 as attrName contains the original value instead of the conditional one removed at line 19.

bildschirmfoto 2018-04-23 um 14 31 10

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Apr 23, 2018

I'm not having any objections to this patch. Interested in hearing more from @AutoSponge though, I value his experience from prior work on morphdom.

@AndyOGo I wouldn't stress about reputation here; this patch is pretty great. If we can keep this on-topic (without any vigorous up/downvoting) I'm sure we can figure out the right approach for this patch. I'm convinced everyone is here with the best intentions.

Also would it perhaps be possible to add a test to catch any future regressions for this? Only if it's not too much effort. Thanks!

edit: also thanks for your first contribution; it's very thorough - that's real nice 😁

@AndyOGo
Copy link
Author

AndyOGo commented Apr 23, 2018

@yoshuawuyts
Thanks for your quick and very friendly answer.

Btw the main contributor of morphdom is @patrick-steele-idem

@goto-bus-stop
Copy link
Member

Ah, I see what its doing now. It looked like a noop to me first too but the difference is that attrName is unaltered (still the full path) in the set branch with this patch. @AndyOGo if you could add a test we can merge this. Because the API behaviour is so unintuitive it would be easy to accidentally break this in the future without a test

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.

SVG use does not repaint in firefox upon second update
5 participants