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

mgol · 2018-12-01T16:45:29Z

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 · 2018-12-01T16:49:28Z

This touches $compile so a security review is needed.

petebacondarwin · 2018-12-02T10:15:52Z

src/ng/compile.js

+              // in XHTML, call `removeAttr` in such cases instead.
+              // See https://github.com/jquery/jquery/issues/4249
+              lowercasedName = lowercase(attrName);
+              isBooleanAttr = BOOLEAN_ATTR[lowercasedName];


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

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.)

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

@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 · 2018-12-02T11:38:08Z

src/ng/compile.js

+              // in XHTML, call `removeAttr` in such cases instead.
+              // See https://github.com/jquery/jquery/issues/4249
+              lowercasedName = lowercase(attrName);
+              isBooleanAttr = BOOLEAN_ATTR[lowercasedName];


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 · 2018-12-03T09:49:42Z

@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 :-)

src/ng/compile.js

gkalpak · 2018-12-03T12:06:28Z

[...] 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 😛)}

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 angular#16778

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 Closes #16779

mgol added type: bug component: $compile frequency: low severity: broken expected use labels Dec 1, 2018

mgol added this to the 1.7.x milestone Dec 1, 2018

mgol self-assigned this Dec 1, 2018

googlebot added the cla: yes label Dec 1, 2018

mgol mentioned this pull request Dec 1, 2018

ng-disabled doesn't remove "disabled" attribute when jquery is loaded #16778

Closed

3 tasks

petebacondarwin suggested changes Dec 2, 2018

View reviewed changes

gkalpak reviewed Dec 2, 2018

View reviewed changes

mgol force-pushed the xhtml-boolean-attrs branch from 832222e to 504ad88 Compare December 3, 2018 10:00

mgol changed the title ~~fix(compile): properly handle the false value for boolean attrs with jQuery~~ fix(compile): properly handle false value for boolean attrs with jQuery Dec 3, 2018

mgol force-pushed the xhtml-boolean-attrs branch from 504ad88 to 9979ebf Compare December 3, 2018 10:02

petebacondarwin approved these changes Dec 3, 2018

View reviewed changes

src/ng/compile.js Outdated Show resolved Hide resolved

gkalpak approved these changes Dec 3, 2018

View reviewed changes

mgol force-pushed the xhtml-boolean-attrs branch from 9979ebf to fe705a9 Compare December 3, 2018 15:41

mgol merged commit 09f013a into angular:master Dec 6, 2018

mgol deleted the xhtml-boolean-attrs branch December 6, 2018 09:07

git-ram-pal mentioned this pull request May 27, 2019

jQuery migrate 3.0.1 shows warnings with angularjs 1.7.6-1.7.8 for removeAttr #16877

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mgol commented Dec 1, 2018

mgol commented Dec 1, 2018

petebacondarwin Dec 2, 2018

gkalpak Dec 2, 2018

mgol Dec 3, 2018

mgol Dec 3, 2018

gkalpak left a comment

gkalpak Dec 2, 2018

mgol commented Dec 3, 2018 •

edited

petebacondarwin left a comment

gkalpak commented Dec 3, 2018

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

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

Conversation

mgol commented Dec 1, 2018

mgol commented Dec 1, 2018

petebacondarwin Dec 2, 2018

Choose a reason for hiding this comment

gkalpak Dec 2, 2018

Choose a reason for hiding this comment

mgol Dec 3, 2018

Choose a reason for hiding this comment

mgol Dec 3, 2018

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Dec 2, 2018

Choose a reason for hiding this comment

mgol commented Dec 3, 2018 • edited

petebacondarwin left a comment

Choose a reason for hiding this comment

gkalpak commented Dec 3, 2018

mgol commented Dec 3, 2018 •

edited