Bug Tracker

Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#6696 closed bug (invalid)

Delegate does not work with child selector "> *"

Reported by: Phrogz Owned by:
Priority: undecided Milestone: 1.4.3
Component: event Version: 1.4.2
Keywords: delegate child selector live Cc:
Blocked by: Blocking:

Description

A) $("#foo").delegate( "> *", "click", ... ) Doesn't work B) $("#foo > *").live( "click", ... ) Works

Where "work" is defined as invoking the callback when clicking on a child element of #x. See http://phrogz.net/tmp/delegate_child.html for sample code. (Click on a row of the table or one of the paragraphs to see output in the console.)

Change History (12)

comment:1 Changed 9 years ago by Phrogz

Better formatting:

$("#foo").delegate( "> *", "click", ... ); // Does not work
$("#foo > *").live( "click", ... );        // Works

comment:2 Changed 9 years ago by SlexAxton

Priority: undecided
Resolution: invalid
Status: newclosed

This is because technically, '> *' is an invalid css selector. You can't start a selector with a child selector.

The correct way to write this as a delegate method would be:

// The element you select is where the event is bound
// The first argument just needs to be a (valid)matching selector for the 
//    type of element you wish to fire the event on
$('#foo').delegate('#foo > *', 'click', ...);

I understand there are some catches to make similar things work (via sizzle), but you should try to use valid selectors whenever possible for speed and compatibility purposes.

Thanks for the report!

Last edited 9 years ago by SlexAxton (previous) (diff)

comment:3 Changed 9 years ago by jitter

Why would this be an invalid selector just because it starts with a child selector?

There are enough other "functions/operations" in jQuery (e.g. find) where this works as expected. I here also would have assumed that the elements in the jQuery object are used implicitly as context and thus a child selector should surely be legal.

e.g. with find vs delegate. Why would the "same" syntax work for find but not for delegate.

test case shows similar usage.

comment:4 Changed 8 years ago by rpflorence@…

This is valid. The delegating element is the context of the selector.

Additionally, your element may not have an ID, and may have been dynamically created so your work-around doesn't work.

And finally, consistency. If it works that way in some instances, it should in others.

comment:5 Changed 8 years ago by SlexAxton

It's an invalid selector because it does not fall into the specification of valid css selectors.

It's difficult to implement because matchesSelector would be useless (since as ryan pointed out it may not even have an id).

We would then need to create some special parsing cases for this where _if_ it started with a set of special characters to bypass the selector engine, and then remove selectors, and then only run matches selector on the direct children of the root element. This would all be slow and gross. Possible, sure, but an instant anti-pattern for performance in my opinion.

I get why this is useful, but I don't know how to make it worthwhile from a code perspective. I would much prefer a fully qualified selector.

As to Ryan's "dynamically created element with an ID" - nothing is stopping you from giving it one. Then this could be fast by using the fully qualified selector and the native matchesSelector stuffs.

Happy to open back up if people still disagree. I am not the end of the line for the argument.

Last edited 8 years ago by SlexAxton (previous) (diff)

comment:6 Changed 8 years ago by ChrisNielsen

I'd like to express support for the eventual resolution of this issue.

As previous commenters have stated, there are several other methods in which an initial child selector works as expected. If it works this way in some places but not in all places, there is a needless burden on developers who must now remember an arbitrary rule (use it HERE but not THERE). This seems counterproductive to the spirit of jQuery.

Also, I note that the provided workaround is not intuitive. When using delegate, the selector is already executed within the parent context. The provided workaround only works because the initial ID selector breaks this convention--in my opinion, another bug! For example:

// This should delegate doSomething for all clicks on span tags that
// are descendants from #foo.
$('#foo').delegate('span', 'click', doSomething);

// But if you read this USING THE SAME CONVENTION, it sounds like it is
// trying to delegate doSomething for all clicks on all span tags that are
// direct children of any element with ID of "foo" that is also a descendant
// of #foo:
$('#foo').delegate('#foo > span', 'click', doSomething);

In other words, the workaround reads as though it is expecting this:

<div id="foo">
  <p id="foo">
    <span>...</span>
  </p>
</div>

Of course, this would be an invalid document. I would therefore argue that the workaround is actually an invalid (or at least nonsensical) CSS selector.

Let's take a look at the documentation for the ID selector:

If more than one element has been assigned the same ID, queries that use that ID will only select the first matched element in the DOM. This behavior should not be relied on, however; a document with more than one element using the same ID is invalid.

Ergo, the documentation suggests that we should not be relying on this workaround.

I therefore ask that this bug be reconsidered.

comment:7 Changed 8 years ago by dmethvin

$('#foo').delegate('#foo > span', 'click', doSomething);

Yep, that's the right way to do it. That may look funny because you have #foo in two places, but consider the case where the elements are selected through other means like a class or traversal methods. And the selector you are matching is absolutely correct. You want to match spans that are children of #foo.

So, on the pro side, it saves a few characters in the selector string. On the con side, it's not a standard W3C selector, it's slower, it's more code in core for an edge case, and it has simple alternatives available like attaching a class to the children or just using the current selector.

comment:8 Changed 8 years ago by ChrisNielsen

Sorry, after re-reading my post, I have detected an error in my reasoning:

The workaround is:

$('#foo').delegate('#foo > span', 'click', doSomething);

The signature here is:

$(parent).delegate(selector, eventType, handler);

I failed to realize that selector is always understood to be in the global context. Sorry for my misunderstanding: because handlers attached with delegate can only fire for events which both match selector AND are on the descendant-or-self axis of parent, I incorrectly assumed that selector must function in the context of parent. Indeed, I am surprised that this is not the case; would this not be faster?

Although I am now enlightened, I still believe it would be worthwhile to address this. Much like find, delegate can ONLY operate under a clearly defined context. I think there is a reasonable expectation that it should use its context in doing so.

I also note that the current definition breaks positional selectors, such as "span:first", which may target very different elements if considered in the global context versus a local context.

comment:9 Changed 8 years ago by dmethvin

Hierarchical selectors are best avoided for performance reasons. About two-thirds of all selectors I've seen in the wild are simple non-hierarchical selectors of the form tag#id.class. Adding a class to the children makes this problem go away and is fast.

Don't use positional selectors for delegation, they are not W3C standard and have confusing meaning in a matchesSelector situation.

If you have a patch that can address the issue in a back-compat way we can certainly look at it, with the understanding that we are not likely to use any solution that takes a lot of bytes to implement or makes the common cases slower. It's not on our radar.

comment:10 Changed 8 years ago by ryan.wheale@…

I would like to make a case for this being re-opened. Take my current situation where I have several un-ordered lists on a single page. Each list has a class of "accordion" and is marked up using your "standard" UL nav markup:

<ul class="accordion">
 <li>
  <a>Heading 1</a>
  <ul>...</ul>
 </li>
 <li>
  <a>Heading 1</a>
  <ul>...</ul>
 </li>
</ul>

And dumbed-down code to allow top level links to serve as accordion headings:

// This does not work
$('.accordion').delegate('> li > a', 'click', function() {
  $(this).next().toggle();
});

// This does work, but wastes resources
$('.accordion').find('> li > a').bind('click', function() {
  $(this).next().toggle();
});

I understand the reasoning behind the proposed workaround... but the argument that this is 'not a standard W3C selector' doesn't sit well when 1) the find() method works, and 2) jQuery is chock full of non-standard selectors. I have been doing jQuery development for years, and I updated an old plugin to use delegate only to find that my selectors didn't work as anticipated.

My proposed solution would be something like this... which should allow matchesSelector to always work.

// at the top of the "on" method (which now replaces delegate)
on: function( types, selector, data, fn, /*INTERNAL*/ one ) {
  [...]
  
  // Shortly before the "return" statement
  // Prepend the selector of "this" to the passed in selector
  if(selector !== undefined) {
    selector = this.selector + ' ' + selector;
  }
  return [...]
}

comment:11 Changed 8 years ago by dmethvin

We're making a strong effort going forward not only to avoid introducing new selector extensions, but to deprecate many existing ones. So the fact they exist today isn't an argument to use them more.

My proposed solution

Actually, this.selector is another thing that isn't reliable either, since it doesn't work correctly on many traversal methods e.g. $("a, button").find("span, b"). So that won't fly, and don't depend on .selector being correct.

I'm not stoked about adding a bunch of code to do this since it's a corner case and it can be accomplished with decent performance by manually doing event delegation. Directly bind an event to your container and then add something like this to the top of the handler:

if ( $(this).children("li").children("a").index(event.target) < 0 )
   return;

http://jsfiddle.net/uPVpH/1/

comment:12 Changed 7 years ago by syndicatedshannon

related: #11993

Note: See TracTickets for help on using tickets.