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

JQLite.find() fails on when collection contains text nodes #4120

Closed
hsablonniere opened this issue Sep 23, 2013 · 10 comments
Closed

JQLite.find() fails on when collection contains text nodes #4120

hsablonniere opened this issue Sep 23, 2013 · 10 comments

Comments

@hsablonniere
Copy link
Contributor

I created a directive. I use the transclude function passed by compile to clone one element of the original DOM into my template DOM.

compile: function (cElt, cAttrs, transclude) {
  return function ($scope, lElt, lAttrs) {
    // some code
    transclude($scope, function (clone) {
      var thead = clone.find('thead');
      lElt.append(thead);
    });
    // some code
  };
}

clone is a JQLite array-like object and when you call find(tagName) on it, it calls find(tagName) on each element and group the results. The problem is that clone can contain text nodes and behind the scenes, find(tagName) uses getElementsByTagName which doesn't work on text nodes.

@petebacondarwin
Copy link
Member

@hsablonniere - So if your transcluded contents contain top level text nodes then the find() fails with an error?
For example...

<my-directive>
  Some text
  <div>Some element</div>
  Some other text
</my-directive>

What happens if you use jQuery instead of jqLite?

@hsablonniere
Copy link
Contributor Author

Exactly. It's even more tricky because line feed and carriage return create text nodes. So it's invisible.

I guess it would work with jQuery since it does a querySelectorAll but I can't add it as a dependency in my project :-(

Maybe I'm not using this stuff correctly but if it's not the case there's two possible fixes :

  • Change find so it does not fail while trying to getElementsByTagName on a text node (
    find: function(element, selector) {
    )
  • Change de fact that cloneLinkingFn is called with nodes that can be text nodes.

What do you think ?

@petebacondarwin
Copy link
Member

I guess that find() failing on text nodes is really a bug that should be fixed. In the meantime you can work around this problem by iterating over the items in the array-like object and checking for non-text nodes before calling find().

@hsablonniere
Copy link
Contributor Author

Yeah thanks very much. I actually needed to find a node by tag name amongst the items of the JQLite array and I've done it by hand using vanilla DOM.

Do you want me to work on a PR ? Either modify the JQLite.find() or the cloneLinkingFn argument...

@petebacondarwin
Copy link
Member

@hsablonniere - I think the fix would be on jqLite#find. Make sure if you submit a PR you have provided relevant unit tests, signed the CLA and checked the commit message guidelines.

@hsablonniere
Copy link
Contributor Author

Ok thx.

@caitp
Copy link
Contributor

caitp commented Oct 13, 2013

I'm not totally sure I understand what the "bug" is --- I've spent some time trying to build a plnkr to reproduce the issue, and I'm not actually seeing text nodes being forgotten to be cloned.

Text nodes are not able to be branch nodes, they are always leafs, and so it's not like there can be content nested within them which is forgotten.

I'm not saying that this isn't a bug, but it would be good to provide a plnkr so that it's more clear what is actually happening, and so that it's clear what the bug really is. Just a thought.

@hsablonniere
Copy link
Contributor Author

You're absolutely right. I'll provide one this weekend ;-)

@hsablonniere
Copy link
Contributor Author

Here's a very silly example demonstrating the bug :

With jQuery : http://plnkr.co/edit/WYO9DJ
With jQLite : http://plnkr.co/edit/xaBPYa

Error in console :

TypeError: Object #<Text> has no method 'getElementsByTagName'

@hsablonniere
Copy link
Contributor Author

The bug is, in that calling .find() on the clone argument of a transclude function fails because this jQLite list can contain text nodes and .find() jQLite implementation doesn't try to avoid it.

fmoggia pushed a commit to fmoggia/angular.js that referenced this issue Oct 25, 2013
find check if the element is a text node.
If element is a text node return an empty array.
Otherwise invoke getElementsByTagName on the element and return the result

Closes angular#4120
@ghost ghost assigned tbosch Dec 3, 2013
@tbosch tbosch closed this as completed in 1169b54 Dec 3, 2013
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
When a jqLite collection contains text nodes, find() does not work :-(

This fix ignores all nodes than can't do getElementsByTagName()

It seems a little bit faster than testing nodeType : http://jsperf.com/nodetype-vs-duck-typing

Closes angular#4120
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
When a jqLite collection contains text nodes, find() does not work :-(

This fix ignores all nodes than can't do getElementsByTagName()

It seems a little bit faster than testing nodeType : http://jsperf.com/nodetype-vs-duck-typing

Closes angular#4120
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants