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

closest: remove exception for pos selectors #2796

Closed
kingjiv opened this Issue Jan 4, 2016 · 14 comments

Comments

Projects
None yet
4 participants
@kingjiv

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 4, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@kingjiv

kingjiv 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.)

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 4, 2016

Member

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.

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.

@kingjiv

This comment has been minimized.

Show comment
Hide comment
@kingjiv

kingjiv 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.

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 4, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 4, 2016

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.

Member

gibson042 commented Jan 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 4, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 4, 2016

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.

Member

gibson042 commented Jan 4, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kingjiv

kingjiv 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.

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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 5, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 11, 2016

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.

Member

timmywil commented Jan 11, 2016

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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 11, 2016

Member

Nevermind, that's a different signature.

Member

timmywil commented Jan 11, 2016

Nevermind, that's a different signature.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 11, 2016

Member

Per meeting, closing this in favor of docs clarification.

Member

timmywil commented Jan 11, 2016

Per meeting, closing this in favor of docs clarification.

@timmywil timmywil closed this Jan 11, 2016

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 11, 2016

Member

Well, aside from removing the pos exception.

Member

timmywil commented Jan 11, 2016

Well, aside from removing the pos exception.

@timmywil timmywil reopened this Jan 11, 2016

@timmywil timmywil changed the title from closest not using the set's context when a context parameter is not passed to closest: remove exception for pos selectors Jan 11, 2016

gibson042 added a commit to gibson042/jquery that referenced this issue Jan 12, 2016

@timmywil timmywil added this to the 3.0.0 milestone Jan 13, 2016

@timmywil timmywil added the Traversing label Jan 13, 2016

@timmywil timmywil closed this in a268f52 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.