attr: Document attr(key, null) to remove attribute

[Krinkle]

Our unit tests cover it explicitly, and the code implements it explicitly:

        if ( value !== undefined ) {

            if ( value === null ) {
                jQuery.removeAttr( elem, name );

From the API perspective I'd also say this makes a lot of sense as it allows roundtripping between the getter returning null and the setter receiving null.

[AurelioDeRosa]

Hi @Krinkle. Maybe there's something I haven't checked but it seems to me that jQuery returns undefined, not null.

Two examples (taken from the attr() method):

// don't get/set attributes on text, comment and attribute nodes
if ( !elem || nType === 3 || nType === 8 || nType === 2 ) {
   return;
}

and

// Non-existent attributes return null, we normalize to undefined
return ret == null ?
   undefined :
   ret;

[kswedberg]

@Krinkle: is there a benefit to using attr(key, null) instead of removeAttr(key). Is the point of documenting attr(key, null) to recommend it or to warn about it?

[Krinkle]

@AurelioDeRosa I know we return undefined now, but I think it used to return null in an older version. And of course the native getAttribute always returns null.

@kswedberg The main benefit is that it allows roundtripping of values. I'd recommend we do this with both undefined and null (right now null removes the attribute, and passing undefined is a weird no-op that neither sets a casted string "undefined" nor removes the attribute).

// Restore foo
var val = foo.attr(key);
foo.attr(key, 'changed');
foo.attr(key, val);

// Sync bar with foo
var val = foo.attr(key);
bar.attr(key, val);

I've run into this a various times in the past few years when a widget overrides an attribute and temporarily and stores the result of calling attr(key) in a property and later restores the attribute. One ends up having to distinguish whether it wasn't there to begin with and potentially remove the attribute afterwards instead of merely restoring its value.

[gibson042]

Setting undefined is a no-op because of #5571, and I suspect that .attr returns undefined for consistency with .prop (the pre-1.6 curse of attroperties). It'd definitely be nice for the API to allow round-tripping, but I'm not sure how much would break if we changed jQuery.attr to return null when no attribute is found (and obviously, it's not an option for .prop). Would you like to open a ticket for discussion on http://bugs.jquery.com/?

[Krinkle]

Setting undefined is a no-op because of #5571.

I'm aware of 5571. This shouldn't affect the original issue though.

I thought we fixed that problem by using arguments.length instead of undefined so that passing a second argument (even if its undefined) will not go in the getter path, but go to the setter path instead (and thus naturally preserve chaining as one would expect from a setter).

However it seems that this is no longer the case (or never was?). Instead passing undefined makes it fall through the cracks. From reading the source code this doesn't seem intended/explicit. Though the test cases do cover it explicitly.

equal( typeof jQuery("#name").attr( "maxlength", undefined ), "object", ".attr('attribute', undefined) is chainable (#5571)" );
equal( jQuery("#name").attr( "maxlength", undefined ).attr("maxlength"), "5", ".attr('attribute', undefined) does not change value (#5571)" );
equal( jQuery("#name").attr( "nonexisting", undefined ).attr("nonexisting"), undefined, ".attr('attribute', undefined) does not create attribute (#5571)" );

It currently uses arguments.length with access() to determine the ultimate return value (whether or not chainable), but this information is not used when interacting with the callback (jQuery.attr). That still uses undefined to determine getter/setter. In the case of passing value=undefined there are 2 arguments, so chainable is true. But neither getter nor setter is invoked. That looks like an oversight.

Passing any value as second argument (say, undefined) should imho either result in either a getter (like old jQuery, not chainable, not desirable), or result in setting value "undefined" (cast to string), or some other setter-like behaviour (e.g. removing the attribute, like we do with null).

Right now it triggers setter-like behaviour (chainable return), but the value is silently rejected and nothing is either get, set or removed.

Attributes can only contain string values (no other primitives like numbers or booleans and no non-primitive values). If we're rejecting it on being an invalid value, should we reject other invalid values?

The native setAttribute, for example, does behave this way (any value passed is set as a string).

> var x = document.createElement('div');
> x.setAttribute('a', undefined);
<div a="undefined"></div>

While inconsistent with the DOM, at least we are being consistent internally. All effected access-based methods (attr(), val(), width(), etc.) ignore the undefined value.

$('<input>').val(undefined).val() === ""

I see now that it was intentionally implemented as a no-op, your idea in fact (@gibson042):

(bug 5571 comment #8)
I would like to define a rule that providing any value argument places .attr etc. into a "set" mode with consistent (chainable) return values and overrideable default behavior to no-op if value is undefined. (.css does this explicitly right now).

Perhaps it's a weird quirk not worth changing that we just need to document?

[dmethvin]

I like the idea of round-tripping as well, but our previous decisions here seem to have us boxed in if we want to maintain strict compat. Having not yet checked the impact of such a decision, I would be okay with having a breaking change so that .attr("name", undefined) removed the attribute rather than being a no-op. I think the main reason for the special handling of undefined was the chainability issue, and we haven't altered that if we implement this behavior.

[gibson042]

There are at least two options for (slightly) breaking backcompat in favor of .attr round-tripping:

• .attr( name ) returns null instead of undefined for nonexistent attributes, in contrast to .prop( name ) and .data( name ). .attr( name, undefined ) remains a chainable no-op.
  • But I don't recall a getter/setter other than .attr that actually removes data; most require a second .remove* method
• .attr( name, undefined ) removes the attribute, in contrast to all other getter/setters and almost regressing on http://bugs.jquery.com/ticket/5571 (in that we'll get bug reports about $el.attr( name, externalObject.undefinedProperty ) clearing attributes instead of leaving them alone)

[jugglinmike]

The discussion around the behavior of attr with undefined, while important, is unrelated to the intent of this issue. I think it should be safe to document "null unsets the attribute" even without consensus on undefined; is that right?

[Krinkle]

@jugglinmike Yep. My main request here is to document (and thereby acknowledge) that null is supported.

Right now, passing null can appear fragile. Peers sometimes point out when I use this idiom in code – sometimes expressing a (very understandable) preference for messy conditional blocks that switch between attr() and removeAttr() based on null. I don't want my code to be responsible for this switch. It inevitably leads to inconsistency when people forget to do so and makes code harder to read. For separation of concerns, this should be (and already is) in jQuery. I just want to make sure I can rely on this behaviour (without risking a silently breaking change), and to have API documentation that I can easily reference as proof of sorts.

However, this isn't very valuable unless we change attr(key) to return null instead of undefined, or change attr(key, val) to support undefined in addition to null for attribute removal.

[Krinkle]

I still don't see this reflected on https://api.jquery.com/attr/ ?

[kswedberg]

@Krinkle, yeah... #890
sigh.