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

Jump to bottom

Bug with .focus and text nodes in 3.4 #4397

Closed
ambischof opened this issue May 14, 2019 · 10 comments · Fixed by #4558
Closed

Bug with .focus and text nodes in 3.4 #4397

ambischof opened this issue May 14, 2019 · 10 comments · Fixed by #4558
Assignees
@mgol
Labels
Bug Event Has Pull Request
Milestone
3.5.0

Comments

@ambischof
Copy link

@ambischof ambischof commented May 14, 2019 •
edited

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: gitana/alpaca#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/
@ambischof ambischof mentioned this issue May 14, 2019
Open
@timmywil
Copy link
Member

@timmywil timmywil commented May 14, 2019

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.

@timmywil timmywil added this to the 3.4.2 milestone May 14, 2019
@dmethvin
Copy link
Member

@dmethvin dmethvin commented May 14, 2019

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.

@jabrah jabrah mentioned this issue Jun 11, 2019
Closed
@dgsmith2
Copy link

@dgsmith2 dgsmith2 commented Jul 17, 2019

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

@seanvree
Copy link

@seanvree seanvree commented Nov 30, 2019

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
Copy link
Member

@dmethvin dmethvin commented Nov 30, 2019

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
Copy link

@seanvree seanvree commented Nov 30, 2019

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.

gitana/alpaca#705 (comment)

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Nov 30, 2019

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
Copy link
Member

@mgol mgol commented Dec 2, 2019

@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 mgol self-assigned this Dec 3, 2019
mgol added a commit to mgol/jquery that referenced this issue Dec 3, 2019
@mgol
Event: Only attach events to objects that accept data - for real
e1ebcc8 
There was a check in jQuery.event.add that made it a noop for objects that
don't accept data like text or comment nodes.

Fixes jquerygh-4397
@mgol mgol mentioned this issue Dec 3, 2019
Merged
3 tasks
@mgol
Copy link
Member

@mgol mgol commented Dec 3, 2019

PR: #4558

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

mgol added a commit to mgol/jquery that referenced this issue Dec 3, 2019
@mgol
Event: Only attach events to objects that accept data - for real
1b71d70 
There was a check in jQuery.event.add that was supposed to make it a noop
for objects that don't accept data like text or comment nodes. The problem was
the check was incorrect: it assumed `dataPriv.get( elem )` returns a falsy
value for an `elem` that doesn't accept data but that's not the case - we get
an empty object then. The check was changed to use `acceptData` directly.

Fixes jquerygh-4397
mgol added a commit to mgol/jquery that referenced this issue Dec 9, 2019
@mgol
Event: Only attach events to objects that accept data - for real
749385a 
There was a check in jQuery.event.add that was supposed to make it a noop
for objects that don't accept data like text or comment nodes. The problem was
the check was incorrect: it assumed `dataPriv.get( elem )` returns a falsy
value for an `elem` that doesn't accept data but that's not the case - we get
an empty object then. The check was changed to use `acceptData` directly.

Fixes jquerygh-4397
@mgol mgol closed this as completed in #4558 Dec 9, 2019
mgol added a commit that referenced this issue Dec 9, 2019
@mgol
Event: Only attach events to objects that accept data - for real
d5c505e 
There was a check in jQuery.event.add that was supposed to make it a noop
for objects that don't accept data like text or comment nodes. The problem was
the check was incorrect: it assumed `dataPriv.get( elem )` returns a falsy
value for an `elem` that doesn't accept data but that's not the case - we get
an empty object then. The check was changed to use `acceptData` directly.

Fixes gh-4397
Closes gh-4558
mgol added a commit that referenced this issue Dec 9, 2019
@mgol
Event: Only attach events to objects that accept data - for real
f36f6ab 
There was a check in jQuery.event.add that was supposed to make it a noop
for objects that don't accept data like text or comment nodes. The problem was
the check was incorrect: it assumed `dataPriv.get( elem )` returns a falsy
value for an `elem` that doesn't accept data but that's not the case - we get
an empty object then. The check was changed to use `acceptData` directly.

(cherry picked from d5c505e)

Fixes gh-4397
Closes gh-4558
@mgol
Copy link
Member

@mgol mgol commented Dec 9, 2019

Landed on master at d5c505e and on 3.x-stable at f36f6ab.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mgol mgol

Labels
Bug Event Has Pull Request
Milestone
3.5.0
Development

Successfully merging a pull request may close this issue.

Event: Only attach events to objects that accept data - for real mgol/jquery
6 participants
@dmethvin @timmywil @mgol @dgsmith2 @ambischof @seanvree