Jquery: Bug with .focus and text nodes in 3.4

[ambischof]

Description

There's a new breaking change in 3.4 when you attempt to call .focus(fn) on a selector containing a text node.

Consider the following:

$('<span>foo</span>hi<span>bar</span>').focus(function() {})

in v3.3.1 this is all fine, but in v3.4.0 and v3.4.1 you get
Uncaught RangeError: Maximum call stack size exceeded

Of note, no one would do this on purpose, as it doesn't make sense, and even this example is a bit contrived (actual case I encountered at least had form elements), but given various libraries out of my control, it managed to happen for me.

This is the case in question: https://github.com/gitana/alpaca/issues/705

This is clearly outside the boundaries of normal usage, however, is it possible for the .focus(fn) could have some checks to not fail quite so critically?

According to the jQuery function docs it _sounds_ like text nodes should generally be tolerated?

When passing HTML to jQuery(), note that text nodes are not treated as DOM elements. With the exception of a few methods (such as .content()), they are generally ignored or removed. E.g:

Link to test case

In this jsfiddle with v3.4.1, it produces an error
https://jsfiddle.net/anneb574/r2pambcz/1/

but this jsfiddle with v3.3.1 it doesn't
https://jsfiddle.net/anneb574/r2pambcz/3/

[timmywil]

What a well-structured issue. Test cases and everything. Thanks!

We can look into whether the call stack error is avoidable. focus did see some significant changes in 3.4.

[dmethvin]

This is like a holy trinity of nightmare scenarios. There aren't a lot of ways to get a text node into a set, but $() and .contents() are two. If you call any filtering method after that the text nodes would be thrown out.

The event system does not want or expect the target of an event to be a text node. It's not supposed to happen, and for the few cases where browsers erroneously did it in the past we fixed the issue when the browser created it. That fix wouldn't work in this case anyway 1) because those text nodes are orphans and event.target would be null which also is not a respectable value and 2) manually triggered events go through a different and less realistic path.

I'll also point out that triggering focus on three nodes in succession as done in this test case is pretty strange. When the whole thing is done only one will have focus, I assume it's the last one but the async focus behavior of IE may do something different.

All that said, it's worth examining the new 3.4 code to see what is causing the error. Blowing out the stack is never a fun outcome.

[dgsmith2]

FWIW, I encounter this error in regards to blur on an element that contains an HTML comment.

[seanvree]

Gents,

Any idea when this might be resolved? I thought I saw somewhere a fix was scheduled for 3.4.2, but I see the milestone is 3.5.

Thanks

[dmethvin]

If you don't care about the text or comment nodes, just add .filter("*") before the call to .focus() or .blur() to throw them out. If you have some other case (e.g., it's not your code and you can't fix it) then you can make a pull request with the fix.

[seanvree]

If you don't care about the text or comment nodes, just add .filter("*") before the call to .focus() or .blur() to throw them out. If you have some other case (e.g., it's not your code and you can't fix it) then you can make a pull request with the fix.

@dmethvin - It's not my code. jQ is required for a library I'm using in a PHP webapp. I wouldn't know where to start as I'm a PHP guy.

https://github.com/gitana/alpaca/issues/705#issuecomment-533105458

[dmethvin]

Normal Github etiquette is to either move the ball forward with a fix of some kind or, if you have no ability to do that, simply vote on the original ticket. Conversations like this notify thousands of people who are watching the repo, so it's rude to them to make "how soon will this be fixed" posts.

[mgol]

@dmethvin

I'll also point out that triggering focus on three nodes in succession as done in this test case is pretty strange.

It's not triggering focus on three nodes but attaching a focus handler on them, using the deprecated event shortcut form. on & trigger are way more readable!

[mgol]

PR: #4558

This issue has uncovered a bug that has been in the code for a long time!

[mgol]

Landed on master at d5c505e35d8c74ce8e9d99731a1a7eab0e0d911c and on 3.x-stable at f36f6abbb3e9e4d7d3729272ffb10b4c2c382919.