Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#11698 closed bug (wontfix)

$/jQuery doesn't set id when creating element with props

Reported by: tj@… Owned by:
Priority: low Milestone: 1.8
Component: core Version: 1.7.2
Keywords: Cc:
Blocked by: Blocking:

Description

When creating elements via jQuery(html, props), the id key doesn't set the ID of the element. This is very surprising.

Test case | source

jQuery(function($) {

  $("<div>Click me</div>", {
    id: "foo"
  }).appendTo(document.body).click(function() {
    $("<p>").text("My id is " + this.id).appendTo(document.body);
  });
       
});

The result is My id is, rather than My id is foo as expected.

Change History (17)

comment:1 Changed 7 years ago by tj@…

Ah, it's not just id, it's the properties in general, and the reason is that the string doesn't just have an element tag name on its own, but more general HTML text: http://jsbin.com/exikis | http://jsbin.com/exikis/edit

This is not clear from the documentation, and seems wrong. If just a single element results (or arguably even just a single top-level element), it seems like the properties should be applied to it. That or the documentation should call it out.

comment:2 Changed 7 years ago by gibson042

This is documented, but I can see a case for always setting attributes on jQuery( html, props )—with a warning against specifying multiple elements in the html.

comment:3 Changed 7 years ago by tj@…

@gibson: I don't see anything in the documentation saying that the props will be ignored if the HTML string is more than just a self-contained element tag, what am I reading past? (It's been known to happen.) I see things talking about how the elements are created, but that doesn't mean that the props will (or won't) be ignored.

But yes, it would be great to have the props apply to the top-level returned element(s).

comment:4 Changed 7 years ago by dmethvin

As of jQuery 1.4, the second argument to jQuery() can accept a map consisting of a superset of the properties that can be passed to the .attr() method. Furthermore, any event type can be passed in, and the following jQuery methods can be called: val, css, html, text, data, width, height, or offset. The name "class" must be quoted in the map since it is a JavaScript reserved word, and "className" cannot be used since it is not the correct attribute name.

[Edited, got the incomplete quote] I'd just prefer to deprecate the $(html, props) form, it's difficult to explain and requires special-case code inside jQuery.

Last edited 7 years ago by dmethvin (previous) (diff)

comment:5 Changed 7 years ago by scottgonzalez

Does that mean .attr( props, true ) would go away too? This is really useful and we use it in jQuery UI.

comment:6 Changed 7 years ago by dmethvin

@scott.gonzalez no, that would be the preferred way to do it.

comment:7 Changed 7 years ago by T.J. Crowder <tj@…>

@dmethvin: I don't understand what you're saying with that quote...? It doesn't say anything about props being ignored in one case but not in another.

FWIW, anything that reduces the outrageous overloading of $(), I'm in favor of. :-) I very rarely use this form and would be happy to use attr instead. But I see it used a lot, I suspect a fair bit of backlash if it were deprecated.

comment:8 in reply to:  4 Changed 7 years ago by jaubourg

Replying to dmethvin:

As of jQuery 1.4, the second argument to jQuery() can accept a map consisting of a superset of the properties that can be passed to the .attr() method. Furthermore, any event type can be passed in, and the following jQuery methods can be called: val, css, html, text, data, width, height, or offset. The name "class" must be quoted in the map since it is a JavaScript reserved word, and "className" cannot be used since it is not the correct attribute name.

[Edited, got the incomplete quote] I'd just prefer to deprecate the $(html, props) form, it's difficult to explain and requires special-case code inside jQuery.

I'm not with you on this one, Dave. I think $( html, props ) is probably the neatest way to create elements in jQuery. It makes for very readable code. $.attr( object, true ) not only adds noise but is also quite esoteric in comparison.

comment:9 in reply to:  3 Changed 7 years ago by gibson042

Replying to tj@…:

@gibson: I don't see anything in the documentation saying that the props will be ignored if the HTML string is more than just a self-contained element tag, what am I reading past? (It's been known to happen.) I see things talking about how the elements are created, but that doesn't mean that the props will (or won't) be ignored.

Emphasis mine:

jQuery( html, props )

html A string defining a single, standalone, HTML element (e.g. <div/> or <div></div>).

props A map of attributes, events, and methods to call on the newly-created element.

comment:10 Changed 7 years ago by Rick Waldron

From http://api.jquery.com/jQuery/#jQuery2

A string defining a single, standalone, HTML element (e.g. <div/> or <div></div>).

But I'm not sure this precludes <div>foo</div>, if so the docs need to be clearer.

I got blitzed by @ gibson042

comment:11 Changed 7 years ago by Rick Waldron

Turns out there is only one supporting test for this signature. I also think that T.J.'s assumption makes sense. It's still one element.

comment:12 Changed 7 years ago by Rick Waldron

Component: unfiledcore
Milestone: None1.8
Owner: set to Rick Waldron
Priority: undecidedlow
Status: newassigned

comment:13 Changed 7 years ago by dmethvin

@tj, sorry I was just confirming that it seemed like someone could be lulled into thinking it should work.

@jaubourg, you're right I misunderstood scott's comment. My brain saw the props and thought .prop. The pass arg to jQuery.attr is not even documented!

@rwaldron, yeah the confusion is because the case where it applies props is only when we recognize the simple single-element string, not something like $('<div id="abc">hello</div>", { "class": "def" });

The docs there are wrong in several ways, which is why I prefer deprecation as the easiest way out:

It's not true that "any event type" is accepted, it's only a short list that you would find only in the code. Just as with the single-elementness, this list can be documented but it makes the feature even more confusing and esoteric.

If it's not one of the recognized event names or methods in jQuery.attrFn we assume it's an attribute. However, since 1.6 we've been trying to draw the line between proper use of attributes and properties. This code always goes through .attr() and therefore would need updating to use .prop() in certain known cases once the "yer doin it rong but we'll save you" attribute-setter shims are removed. Or, we can never remove the shims.

This isn't even about brevity, we have a perfectly good chaining syntax that is not ambiguous, just about as brief as $(html, props) and doesn't require memorizing a long paragraph of special cases.

As far as deprecation goes, again it doesn't mean we *must* take it out. It means we know it has drawbacks, we can't easily fix them, and don't advise you use it in new code.

comment:14 Changed 7 years ago by gibson042

@rwaldron, the relevant fork uses document.createElement instead of jQuery.buildFragment when rsingleTag = /^<(\w+)\s*\/?>(?:<\/\1>)?$/ matches. It doesn't make sense to allow only text content and the regex is useless if we allow complex content, so we should either leave this alone or set attributes whenever the second argument is a plain object.

comment:15 Changed 7 years ago by Rick Waldron

Owner: Rick Waldron deleted
Status: assignedopen

I just poked around a bit and my first thoughts about the conditional matching that would likely be required to make this work is not worth it.

@gibson042 blitzed me... again!

Last edited 7 years ago by Rick Waldron (previous) (diff)

comment:16 Changed 7 years ago by dmethvin

Resolution: wontfix
Status: openclosed

I still feel like $(html, props) is a minefield. Even the abbreviation is deceptive since props are either event handlers or *attributes*; properties are only handled correctly due to the grace of special case attrHooks we want to remove. So let's not pick at this, we'll make it bleed.

comment:17 Changed 6 years ago by bobince@…

This also affects elements with attributes, ie:

$('<div class="a">', {id: 'b'})

fails.

If this behaviour isn't going to be fixed, let's deprecate the feature entirely - at the moment it is a surprising trap.

Note: See TracTickets for help on using tickets.