Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(jqLite): use .children to fetch child elements but fallback to .childNodes for SVGs in IE #8075

Closed
wants to merge 1 commit into from

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Jul 4, 2014

e35abc9 broke jqLiteDealoc on SVG elements in IE because .children is not supported on SVGs in IE (https://developer.mozilla.org/en-US/docs/Web/API/ParentNode.children#Browser_compatibility).

This change adds an SVG test and fixes it by doing .children || .childNodes. I also added .children in a few more places in addition to where e35abc9 originally did. Although if the .childNodes fallback is always required for IE, is it still worth using .children?

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8075)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@jbedard jbedard added cla: yes and removed cla: no labels Jul 4, 2014
@jbedard
Copy link
Contributor Author

jbedard commented Jul 10, 2014

@IgorMinar & @rodyhaddad since e35abc9 introduced the issue.

@jbedard
Copy link
Contributor Author

jbedard commented Jul 10, 2014

Another option is to use .getElementsByTagName('*') instead of .children/.childNodes, that way no recursion is necessary. For single nodes .getElementsByTagName('*') is slightly slower, but for all other cases when the element has children it seems faster: http://jsperf.com/gebtn-vs-children

rodyhaddad added a commit to rodyhaddad/angular.js that referenced this pull request Jul 10, 2014
SVG elements in IE don't have a `.children` but only `.childNodes` so it broke.
We started using `.children` for perf in e35abc9.

This also acts as a perf improvements, since
`getElementsByTagName` is faster than traversing the tree.

Related angular#8075
@rodyhaddad
Copy link
Contributor

Thanks for making us aware of that!

Looking at jQuery's code, they use .getElementsByTagName when doing .remove. And as your jsperf shows, it's more performant.
So I think we're gonna go in that path for jqLite.

As for single nodes being slower, we can just add a el.childNodes.length check, which makes it a lot faster (see http://jsperf.com/gebtn-vs-children/2)

I'll make a PR for that, to hopefully get it in before next release

@rodyhaddad
Copy link
Contributor

See #8136

@rodyhaddad rodyhaddad closed this Jul 10, 2014
ckknight pushed a commit to ckknight/angular.js that referenced this pull request Jul 16, 2014
SVG elements in IE don't have a `.children` but only `.childNodes` so it broke.
We started using `.children` for perf in e35abc9.

This also acts as a perf improvements, since
`getElementsByTagName` is faster than traversing the tree.

Related angular#8075
jbedard referenced this pull request in IgorMinar/angular.js Aug 12, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants