AngularJS's ng-disabled doesn't remove "disabled" attribute when jquery is loaded

[ghost]

I'm submitting a ...

• bug report
• feature request
• other

Current behavior:
ng-disable creates disabled="false"/ng-disabled="true" attributes for elements when jquery was loaded before angularjs. This was observed only for XHTML

Expected / new behavior:
disabled attribute is removed when ng-disabled evaluates to false

Minimal reproduction of the problem with instructions:
http://next.plnkr.co/edit/1GxYGSKho4H0nDAD
(tested in chromium - should be the same behavior in any XHTML aware browser)

AngularJS version: 1.7.5 (or whatever your server currently ships)

Browser: all? (tested in chrome 70.0.3538.110 | Firefox 60.3.0esr)

This was already submitted to angularJS but they say it is the fault of jquery: angular/angular.js#16778

[jbedard]

Here's a jQuery only version: http://next.plnkr.co/edit/QlL18WpsOlkofGJ0

[ghost]

I don't need a jQuery only version. I am talking about a bug which breaks angularJS.

[mgol]

@CharlemagneLasse A jQuery-only version is needed for us to confirm there's an issue lying on the jQuery side.

An even simpler test case: https://next.plnkr.co/edit/MFIiCjoCv4k3sOPA

The issue lies in https://github.com/jquery/jquery/blob/3.3.1/src/attributes/attr.js#L44 where XML nodes are explicitly excluded from attrHooks and hence from the special boolean attributes treatment. The commit that introduced it (641ad80) dates back to 2011! It was committed by @timmywil. The jQuery issue it fixes is https://bugs.jquery.com/ticket/9568.

@timmywil It's not clear to me from that old ticket description why it's more important in XML documents to be able to set boolean attribute values to any value than in regular HTML documents. Do you remember the reasons?

From what I understand the original issue wanted to set those attrs on a custom node where they don't have special meaning. That could be achieved by changing boolean attrs logic to account for the nodeName of an element on which it's applied, although that would add quite a bit of bytes to the gzipped size.

Any change we do related to such old code will need to be treated as a breaking change and so will need to wait for 4.0.

One related issue from the past: #2946.

@jbedard I think AngularJS could be changed to invoke removeAttr(attrName) instead of .attr(attrName, false) for boolean attrs to avoid the issue.

[dmethvin]

We document that .attr(name, null) removes the attribute the same way .removeAttr(name) does. The value is supposed to be a string, number, or null but stringifying false to "false" doesn't seem crazy and would be the desired result with ARIA attributes, for example where that is the string you want!

Here's the way I remember it. The .attr(name, false) case dates back to the attroperty days, it was the equivalent of .prop(name, false) because oldIE often needed it that way. When Timmy did the rewrite for 1.6 he put in a hook to salvage at least some of that behavior because existing jQuery code was misusing .attr() to set properties like disabled. Since oldIE didn't have XHTML semantics it should be safe to not support that case since old code wasn't expecting it.

[timmywil]

This was 7 years ago so bear with me if I misremember some details. I'll attempt to explain.

The boolean hook is there both for back-compat-lots of code still out there with .attr(<boolean>, false) that would not work as intended-and compliance with the standard.

If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

Not even the strings "true" and "false" are technically allowed.

The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.

The point is simply to write valid DOM despite common, predictable misuse of the API.

An attribute in an XML document that happens to have the same name as a boolean attribute, such as disabled does not have any special meaning. The boolean attribute behavior only applies to HTML documents (and even then limited to certain nodeNames as @mgol pointed out).

The original bug wasn't really related to XML exactly, but it did make me think of this distinction when the user tried to access what looked like boolean attributes on a custom tag. As @mgol said, the disabled attribute on a custom tag has no special meaning or behavior. In the old boolean hook, we used to check the corresponding property to construct a valid return value. In this case, the property of course returned undefined, just as it would in XML, and so it always returned undefined for the attribute value. At some point, we started sharing a lot more attribute code with Sizzle and instead checked the attribute using .getAttribute. If it returns any value at all, we assume the boolean property is true and move on-this is what actually fixed the original bug.

The boolean setter hook remains in place to remove boolean attributes whenever someone tries to set false, which as @dmethvin pointed out is an artifact of the old attroperties and far too common and technically incorrect. However, Angular should have been using .removeAttr all along. .attr("disabled", false) really makes no sense, semantically. If we were completely aligned with the spec, what we should have done is set the value "false", which would both be invalid and work as if the value "disabled" or literally any string value were set instead, because browsers are lenient.

As for what changes to make, I'm not sure. It's my understanding that boolean attributes also have no special meaning or behavior in a pure XML document (which is different than XML in HTML), even on tags that happen to be named INPUT, hence the different path for XML documents. However, XHTML presents a challenge if we cannot distinguish between XHTML (which is just XML syntax for HTML) and XML (which would display as raw XML if you tried to render it in a browser without defining the method in which to render it). We support retrieving and setting attributes in both cases.

[timmywil]

Sizzle has a PR that should address this. XHTML will follow the HTML path and all should work as expected, unless anything we haven't foreseen comes up, which sometimes happens.

[mgol]

Sizzle PR: jquery/sizzle#436

[mgol]

I added this to the 3.4.0 milestone as all we need is merging that Sizzle PR, publishing it & updating the Sizzle dependency in jQuery.