fix(compile): properly handle false value for boolean attrs with jQuery

[mgol]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix

What is the current behavior? (You can also link to an open issue here)

ng-disabled="disabled" always disables the element in XHTML mode when jQuery is used, even when the disabled binding is set to false.

What is the new behavior (if this is a feature change)?

Yes, but only in XML mode (a.k.a. XHTML).

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Fix/Feature: Docs have been added/updated N/A
• Fix/Feature: Tests have been added; existing tests pass

Other information:

jQuery skips special boolean attrs treatment in XML nodes for historical reasons
and hence AngularJS cannot freely call .attr(attrName, false) with such attributes. To avoid issues in XHTML, call removeAttr` in such cases instead.

Ref jquery/jquery#4249
Fixes #16778

[mgol]

This touches $compile so a security review is needed.

[petebacondarwin]

Can we not just use booleanKey here rather than recomputing this stuff?

[gkalpak]

Using booleanKey has the difference that it takes the nodeName into account. On the other hand, the current approach stays more similar to jqLite's implementation of attr(). (Not sure which option is best.)

[mgol]

Using booleanKey may be a good idea, let me try it.

[mgol]

@petebacondarwin PR updated.

[gkalpak]

Not sure how I feel about "fixing" this at this point. There will still be a difference between jqLite and jQuery on XML documents (such as XHTML) (e.g. for truthy values or possibly how values are handled for non-BOOLEAN_ELEMENTS).

Will there be a difference for non-XML documents?
(I can't think of any, but am I missing something?)

[gkalpak]

Using booleanKey has the difference that it takes the nodeName into account. On the other hand, the current approach stays more similar to jqLite's implementation of attr(). (Not sure which option is best.)

[mgol]

@gkalpak

Not sure how I feel about "fixing" this at this point. There will still be a difference between jqLite and jQuery on XML documents (such as XHTML) (e.g. for truthy values or possibly how values are handled for non-BOOLEAN_ELEMENTS).

That's true but it's not about attr in jqLite vs jQuery, it's about ng-disabled etc. being broken in XML documents when jQuery is loaded. It may suggest to the user the problem is in jQuery when, in fact, the issue is in AngularJS as it uses jQuery API incorrectly. See https://api.jquery.com/attr/, there's nothing in there that allows passing a boolean value parameter. You can also see it disallowed by the TypeScript definitions at @types/jquery#1095-1906.

Will there be a difference for non-XML documents?

There are some attrHooks related to radio elements in jQuery so maybe but there shouldn't be anything major, I think (I spent some time on re-aligning jQuery & jqLite for AngularJS 1.6). To be sure I'd have to spend more time on it, though.

[petebacondarwin]

I'm happy with the code now, except for the redundant variables; I'll leave @mgol and @gkalpak to battle out the semantic question :-)

[gkalpak]

[...] There will still be a difference between jqLite and jQuery on XML documents (such as XHTML) (e.g. for truthy values [...]

That's true but it's not about attr in jqLite vs jQuery, it's about ng-disabled etc. being broken in XML documents when jQuery is loaded.

I am not talking about .attr() either. There will be differences (in XHTML) with and without jQuery when ngDisabled evaluates to true for example (the disabled attribute will be set to "true" and "disabled" respectively).

But maybe that is not a big deal for truthy values.

I certainly feel more confident now that BOOLEAN_ELEMENTS are at least taken into account in both cases.

In any case, since this will only affect XML-based documents (which shouldn't be that wide-spread any more), I am fine shipping this.
_{(And if it breaks someone's XHTML app, I won't tell you "I told you so", but you will know I told you so 😛)}