Skip to content

closest: remove exception for pos selectors #2796

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.

Sign up for GitHub

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
Closed
kingjiv opened this issue Jan 4, 2016 · 14 comments
Closed

closest: remove exception for pos selectors #2796

kingjiv opened this issue Jan 4, 2016 · 14 comments
Labels
Traversing
Milestone
3.0.0

Comments

@kingjiv
Copy link

kingjiv commented Jan 4, 2016

Further discussion here: http://stackoverflow.com/questions/34580013/jquery-closest-default-context

The documentation for closest states:

If no context is passed in then the context of the jQuery set will be used instead.

However, it seems that this is often not the case. You can see this illustrated here:

https://jsfiddle.net/pjbg4398/

When the context is passed explicitly, closest works as expected and only finds an element if it is within the context element. However, if the jquery set has a context and closest is called without explicitly passing a context, no context is used (contradicting the documentation). However, if the selector passed to closest contains non-css selectors (gt, eq, etc.), then the set's context is used.

I believe the fix should be to change the loop condition as follows:

for (cur = this[i]; cur && cur !== context; cur = cur.parentNode) {

should become:

for (cur = this[i]; cur && cur !== (context || this.context); cur = cur.parentNode) {

which would be in line with the non-css code which does a similar thing with context:

pos = rneedsContext.test(selectors) || typeof selectors !== "string" ? jQuery(selectors, context || this.context) : 0;
@dmethvin
Copy link
Member

dmethvin commented Jan 4, 2016

Hmmm. The .context property was deprecated as of 2.0 and will be removed in 3.0, see gh-1908, so it looks like the docs need to be changed in that respect

Even if the property was still present, Sizzle.matchesSelector doesn't take a context so any string selectors passed to .closest() with a context wouldn't be rooted at the context the way they should be which could cause incorrect results. The only place we use the context arg there is for positional selectors, I think it would be best to document that and leave the implementation as-is.

@kingjiv
Copy link
Author

kingjiv commented Jan 4, 2016

Deprecation aside, you wouldn't need to be able to pass context to matchesSelector for it to work as documented but rather just update the loop condition as mentioned. The documentation implies that this.context will be used in place of context if context is not passed. Since passing context works as expected, you could certainly do the same with this.context.

That said, I think most people would be confused by it actually working as documented. Since it generally doesn't work as documented (and hasn't for many releases--if ever), I would think it would be better to remove the "this.context" portion of the "pos" creation and just remove that line from the documentation.

As it is now, it is inconsistent as using ":first" in your selector will use the context but without it it won't, etc.

(Not sure if any of this matters in the end if 3.0 is in the works and there will be no further 2.x releases, not familiar with the current release schedule. Obviously if this.context is removed entirely it will need to be removed here anyway.)

@dmethvin
Copy link
Member

dmethvin commented Jan 4, 2016

Discussed in the meeting today. Since this.context doesn't exist anymore in 3.0 we need to update the docs to remove the mention; the bug won't be fixed because the property has disappeared. We also need to make clear that the context arg in .closest() is only used for positional selectors.

@dmethvin dmethvin mentioned this issue Jan 4, 2016
Closed
@kingjiv
Copy link
Author

kingjiv commented Jan 4, 2016

But currently the context parameter is used for all selectors. It is in the loop condition so the loop ends once the context element is reached, thereby not continuing any further up the dom.

The only possible issue I see with the context parameter is if the context is not an ancestor element then it is basically "not used" for non positional selectors.

@dmethvin
Copy link
Member

dmethvin commented Jan 4, 2016

Yeah, true, the docs need to be clear about what "used" means. I was thinking in terms of being the root of the tree from which selector strings are evaluated. I agree it is currently the point at which we stop trying to match, which is a different thing.

For example, http://jsbin.com/xuqeneqayi/1/edit?html,js,console

<body>
<div id=outer>
  <div id=inner>
    <div id=start>
    </div>
  </div>
</div>
</body>

$("#start").closest("div div div", $("#inner")) // returns #start which is wrong

@gibson042
Copy link
Member

The context parameter has two effects: a cap on upward traversal, and a root for positional selectors and non-string selectors (another latent bug, by the way). The first is of arguable value but (as noted above) fails for any element in the collection that does not descend from it, but the second seems utterly pointless ($el.closest( "div:first", context )? Really?) and isn't even tested (the closest we have is assertions that positional selectors without context yield nothing).

I'd prefer to drop the parameter completely, but at minimum I think the positional selector interaction should be removed.

@dmethvin
Copy link
Member

dmethvin commented Jan 4, 2016

I can see the cap on upward traversal being useful in the context of a component on the page that could be embedded in arbitrary markup. Still, in cases where I personally have wanted to use .closest() it was a programming error to not be inside whatever I expected to find; limiting the traversal just ensures that nothing happens which isn't always the best thing for debugging. 😸

$el.closest( "div:first", context )? Really?

Yeah, I tried to think of a reason for using .closest() with a positional selector, with or without a context, and I got nothing. It's possible that someone is using it but if we're going to break their code it seems like 3.0 would be as good a time as any.

If we can come to an agreement on this I can put together a PR. At minimum we need to remove this.context but removing the whole positional selector exception would be nice as well. That would leave context only for limiting upward traversal.

@gibson042
Copy link
Member

Yeah, I think we should just return empty whenever the selector is positional rather than try to get clever.

As for the second argument, it feels like an underpowered parentsUntil alias (e.g., $el.closest( selector, elHalt ) is just $el.parentsUntil( elHalt, selector ).first() and $els.closest(…) is similar but requires a wrapping $els.map), which I guess is okay because that method also ignores non-ancestor halting conditions.

@kingjiv
Copy link
Author

kingjiv commented Jan 5, 2016

I see the point your making with the nested div example but just want to point out that the context parameter here has to be a dom element. Passing a jquery object as in your example has no effect at all with non-positional selectors. I suppose you could consider that another bug, but the documentation does define it as a dom element so not really a bug.

@dmethvin
Copy link
Member

dmethvin commented Jan 5, 2016

I agree about the error in the example @kingjiv but even when corrected it's the same result. http://jsbin.com/ququlisabu/1/edit?html,js,console

@timmywil
Copy link
Member

So, I think we already deprecated the context argument. The docs even say that it was removed in 1.8, but it's still around. Perhaps all we need to do is actually remove it in 3.0.

@timmywil
Copy link
Member

Nevermind, that's a different signature.

@timmywil
Copy link
Member

Per meeting, closing this in favor of docs clarification.

@timmywil
Copy link
Member

Well, aside from removing the pos exception.

@timmywil timmywil reopened this Jan 11, 2016
@timmywil timmywil changed the title closest not using the set's context when a context parameter is not passed closest: remove exception for pos selectors Jan 11, 2016
gibson042 added a commit to gibson042/jquery that referenced this issue Jan 12, 2016
@gibson042
Traversing: Never let .closest() match positional selectors
c5144f8 
Fixes jquerygh-2796
@gibson042 gibson042 mentioned this issue Jan 12, 2016
Closed
@timmywil timmywil added this to the 3.0.0 milestone Jan 13, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Traversing
Milestone
3.0.0
Development

Successfully merging a pull request may close this issue.

Traversing: Never let .closest() match positional selectors gibson042/jquery
4 participants
@dmethvin @timmywil @gibson042 @kingjiv