Skip to main content

Bug Tracker

Side navigation

#13265 closed bug (fixed)

Opened January 19, 2013 01:03AM UTC

Closed February 13, 2013 02:30AM UTC

Last modified May 24, 2013 01:43PM UTC

parent method fails with text nodes in IE10

Reported by: markrendle Owned by: dmethvin
Priority: high Milestone: 1.10/2.0
Component: traversing Version: 2.0b1
Keywords: Cc: orkel
Blocked by: Blocking:
Description

In earlier versions of jQuery, the parent method looked like this:

parent: function (elem) {

var parent = elem.parentNode;

return parent && parent.nodeType !== 11 ? parent : null;

}

In 2.0.0b1, it looks like this:

parent: function (elem) {

return elem.parentElement;

}

In IE10, it seems text nodes are not elements, and do not have the parentElement property, so parent() returns undefined.

Attachments (0)
Change History (16)

Changed January 26, 2013 08:20PM UTC by dmethvin comment:1

cc: → markelog
owner: → markrendle
status: newpending

Do you have a test case for this? Did you encounter it in real code? I can kind of see how it might happen if you used something like .contents() although we generally don't provide full support for text nodes in a jQuery set.

Changed January 27, 2013 01:27AM UTC by gibson042 comment:2

component: unfiledtraversing
priority: undecidedhigh
status: pendingopen

http://jsfiddle.net/ZH8em/

I'm inclined to make the same argument here as elsewhere; that text nodes should generally be considered valid input for traversing methods.

Changed January 27, 2013 06:29PM UTC by gibson042 comment:3

It turns out that satisfying my position would require reverting much of https://github.com/jquery/jquery/commit/65bdfbf07f911d5014e32ab126ef0c568c22bf49 (even Chrome doesn't define *ElementSibling on text nodes). Looks like it's judgment time for official support/non-support.

Changed January 27, 2013 06:51PM UTC by markrendle comment:4

I did encounter this issue in real code; it happens in AngularJS's compile phase, which seems to include the newlines between divs as text nodes with value "\\n". It works fine with jQuery 1.9 and earlier, and in Chrome and Firefox with 2.0.

Changed January 27, 2013 07:23PM UTC by dmethvin comment:5

Man that's just sad. I guess we'll have to back out a lot of those 2.0 optimizations because our users can't easily avoid having text nodes in the set and it doesn't make sense to fork the logic.

Changed January 28, 2013 03:22AM UTC by markelog comment:6

Let's look at documentation for jQuery#parent

Get the parent of each element in the current set of matched elements, optionally filtered by a selector.

and

Given a jQuery object that represents a set of DOM elements, the .parent() method allows us to search through the parents of these elements...

If you look at documentation for others traversal methods you can find the same quotes. Text nodes, comments, document fragments, etc; aren't elements nodes.

even Chrome doesn't define *ElementSibling on text nodes

This is logical, how can, say,

nextElementSibling
property, on text node, can return element node, if text node is not an element?

How can it be "next element" if current node is not an element? Same logic applies to jQuery traversal methods – if, say,

jQuery#nextAll
, return only element nodes, current node should be an element, because we search the next elements relative to the current element.

Otherwise

jQuery#nextAll
called on text node should return next text nodes, but not an element nodes.

This is why, say,

parentElement
prop on element, doesn't return document fragment node (if it's immediate parent),
jQuery#parent
, doesn't return it too.

What is bad, that behavior in 2.0 differ from 1.9, but differ or not, jQuery does not, generally, support them, nor data module, nor events and etc; are not fully (or entirely) support them too, same goes for other node types,

i think we don't have tests for them, for that reason, 1.9 traversal methods called on text node situation is just a side effect of

jQuery#contents
.

Given by limited use cases, and hit on size (although not big) and perfomance, i say we should stick to W3C specification and our documentation.

That being said, we might make an exception for

jQuery#parent
, this is more or less in line with the existing logic, although contradicts our documentation, but i would not hurry with such decision, we might just wait for other examples, if it really widespreaded, others tickets should pop up.

Changed January 28, 2013 04:27AM UTC by gibson042 comment:7

We've always suffered a backhanded treatment of text nodes, but methods like .contents, parseHTML, and even jQuery itself are explicit about putting them in collections. I took the position last year that we should stop carving out exceptions piecemeal for things like .has and .parent, and accept them as valid ''input'' (though generally not output) for ''traversing'' methods (specifically excluding other modules). As it turns out, we were already pretty solid on this point, but unfortunately it wasn't secured by unit tests—which is how we got here.

But either way, I think the time has come for a clear stance, preferably expressible in a single line of copypasta.

Changed January 28, 2013 05:37AM UTC by markelog comment:8

Traversal module is basically representation of Element Traversal API, it always has been, documentation is explicit about what input and output should be, just like W3C-specification about definition of what element is, it's pretty clear stance i would say.

There are some exceptions, but i think not in this case, for text node, it's logical that it might have parent element. But it's not logical to ask for previous element from a text node if user does not have current element node, but only text node, this is not semantically correct.

If we excluding other modules and we are and we should, leaving only traversal module for that equation it would look as exception to me.

Changed January 28, 2013 05:40PM UTC by dmethvin comment:9

@markrendle, Can you point to the code that was failing? We're trying to determine whether just ignoring the text nodes during traversal would give us the correct results.

Changed January 28, 2013 05:50PM UTC by markrendle comment:10

https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L313

What happens (in my app, at least) is, the call on line 316 to jqLite (which is jQuery if it's present) passes a <div>, a text node "\\n" and another <div>. The return is [element, undefined, element], which then causes an error on line 321, "no property nodeType on undefined".

It's possible that splicing the text node out of the array, either at Angular's end or in the jQuery function, would fix the issue; I haven't tried it though.

Changed January 28, 2013 06:37PM UTC by dmethvin comment:11

milestone: None1.9.1

Thanks Mark! This is funny, their code is basically faced with the same dilemma we have about text nodes and their workaround is what breaks:

https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L318

This could easily be rewritten to avoid the traversal method, and is depending on undocumented behavior as it is right now.

During a meeting discussion we talked about changing the behavior of 1.9.1 traversal methods so I'll mark this for action there.

Changed February 01, 2013 03:52PM UTC by dmethvin comment:12

owner: markrendledmethvin
status: openassigned

I'll touch base with the Angular guys and convince them to rearrange that code slightly, so that they start with an element and append the text node, which is the kosher way to do it.

Changed February 13, 2013 02:30AM UTC by Richard Gibson comment:13

resolution: → fixed
status: assignedclosed

Fix #13265 #13332: Allow .parent/.closest with text nodes. Close gh-1146.

Changeset: 5e29ff7e59a0c97bae02b7512b40486df0f44022

Changed February 13, 2013 02:39AM UTC by dmethvin comment:14

milestone: 1.9.11.9.2

Changed February 13, 2013 03:09AM UTC by Richard Gibson comment:15

Fix #13265 #13332: traversing methods with text nodes. Close gh-1145.

Changeset: b734666f4d2e9a92b8ebb99db5b05bd4c82e71f2

Changed May 24, 2013 01:43PM UTC by dmethvin comment:16

milestone: 1.9.21.10/2.0

Bulk update to milestone 1.10/2.0