Align jqLite's attr method with jQuery #15181

mgol · 2016-09-23T11:50:18Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactors & breaking API changes.

What is the current behavior? (You can also link to an open issue here)
Boolean attribute getters take properties into account and setters modify them. All falsy values for boolean attribute setters remove the attribute and set the property to false. The null value in a setter (for all attributes, not only boolean ones) sets the attribute value to the string "null" instead of removing it.

What is the new behavior (if this is a feature change)?
Boolean attribute getters/setters don't touch properties. The null value in a setter (for all attributes, not only boolean ones) removes the attribute. The false value removes it only for boolean attributes. Other defined values for boolean attributes set the value to the lowercased attribute name.

Does this PR introduce a breaking change?
Yes.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
~~Docs have been added / updated (for bug fixes / features)~~ - only in a form of commit messages with their BREAKING CHANGES sections; is anything else needed?

Other information:

Narretz · 2016-09-26T17:48:15Z

There are a few failures when running the tests with jquery 2.2, which is probably expected. How do we handle this?

mgol · 2016-09-28T12:22:46Z

@Narretz I fixed the test: I'm just skipping that one in jQuery 2.x as this was a jQuery 3.0 change.

BTW, most other changes here are aligning even with older jQuery versions, this is not just about 3.0 changes.

mgol · 2016-09-29T15:55:33Z

Tests pass, PR ready for review.

gkalpak

I have left a couple of (mostly minor) comments.

One thing that I believe is not pointed out explicitly enough in the BC notices is the fact that boolean attribute getters do not rely on the value of the corresponding property (as was previously the case).

From the commit message:

Previously, all boolean attributes were reflected into properties in a setter and from a property in a getter, even on elements that don't treat those attributes in a special way. Now it's only happening for the elements where browsers do the conversion automatically.

This sounds to me like boolean attribute getters will still take the property into account, but only on browsers that do the conversion automatically (which is not the case).

gkalpak · 2016-09-30T08:06:58Z

test/jqLiteSpec.js

@@ -635,6 +635,56 @@ describe('jqLite', function() {
      expect(select.attr('multiple')).toBe('multiple');
    });

+    it('should set respective properties when getting/setting boolean attributes', function() {


As far as I understand, it is actually the browser that does that, not jqLite/.attr().
We should either remove the test (if we don't consider it to be jqLite's job), or make sure jqLite always sets the properties (instead of relying on the browsers to do it).

Also, getting seems not related here.

IMO it often does make sense to test things like that; in a way that we should have had a test like that in 1.5.x and it should remain passing after we remove our own reflection. In this way we'd know if a buggy browser broke it and we need to patch it ourselves.

But maybe here we wouldn't do the patching either way so I'd be fine either way.

I was under the impression that we wouldn't do the patching even if a browser/version failed to do the right thing. If the plan is to do the patching in that case, then I would rather be on the afe side and set the properties anyway. (But if jQuery doesn't do it, I think it is better for jqLite not to do it either.)

I don't feel strongly about keeping the test around, but a different description that makes it clear we expect the browser to do it would probably make things a little easier for our future selves, if something breaks 😃

OK, I removed the test.

gkalpak · 2016-09-30T08:09:43Z

test/jqLiteSpec.js

+      expect(div.attr('multiple')).toBe('multiple');
+    });
+
+    it('should not set properties when settting respective boolean attributes', function() {


Too much t in setttings 😃

gkalpak · 2016-09-30T08:12:53Z

src/jqLite.js

@@ -638,33 +638,32 @@ forEach({
  },

  attr: function(element, name, value) {
+    var ret;


_{Nit: I prefer declaring vars in the block in which they are used.}

Then we have a conflict as I prefer them where they're really declared by the browsers. :P

I prefer small functions so that there is not much difference between the two approaches :-P

gkalpak · 2016-09-30T08:13:58Z

src/jqLite.js

+      // setter
+
+      if (value === null || (value === false && isBooleanAttr)) {
+        element.removeAttribute(name);


Previously, for boolean attributes, we used the lowercasedName. Does it make a difference?

It shouldn't. Also, jQuery removes name, not lowercasedName.

gkalpak · 2016-09-30T08:18:17Z

src/jqLite.js

+    } else {
+      // getter
+
+      ret = element.getAttribute(name);


Is there any chance getAttribute(name) could return undefined (on any browser)?

I don't think so but if that happened then what do you propose? jQuery only checks for null to convert it to undefined, if a browser returned undefined we'd already have what we want.

This line was using a != null check, so I assumed undefined is a possible return value. If that is the case, then we shouldn't set ret = lowercasedName when ret === undefined, but afaict getAttribute() shouldn't ever return undefined (in which case we are fine).

gkalpak · 2016-09-30T08:20:06Z

src/jqLite.js

      }
-    } else if (isDefined(value)) {
-      element.setAttribute(name, value);
-    } else if (element.getAttribute) {


Previously, there was this element.getAttribute check, which is now removed.
Was it necessary for IE<9 browsers only? It would be useful to have a comment (e.g. in the PR) explaining why it is not necessary any more.

Good catch. It's needed for window doesn't have getAttribute. In such cases jQuery falls back from attr to prop; should we do the same here?

It was still failing in a setter which jQuery doesn't so I fixed that as well. I decided to not fall back to prop for now, though, but just return as this feature is not very well tested & documented in jQuery.

gkalpak · 2016-09-30T08:20:59Z

src/jqLite.js

-    } else if (element.getAttribute) {
-      // the extra argument "2" is to get the right thing for a.href in IE, see jQuery code
-      // some elements (e.g. Document) don't have get attribute, so return undefined
-      var ret = element.getAttribute(name, 2);


Same as above: It would be useful to have a comment (e.g. in the PR) explaining why this is not necessary any more (assuming it isn't 😛).

mgol · 2016-10-05T10:40:10Z

src/jqLite.js

@@ -56,7 +56,7 @@
 * - [`after()`](http://api.jquery.com/after/)
 * - [`append()`](http://api.jquery.com/append/)
 * - [`attr()`](http://api.jquery.com/attr/) - Does not support functions as parameters
- * - [`bind()`](http://api.jquery.com/bind/) - Does not support namespaces, selectors or eventData
+ * - (_deprecated_, use `on()`) [`bind()`](http://api.jquery.com/bind/) - Does not support namespaces, selectors or eventData


Is that a good way to mark methods as deprecated? Thoughts?

That's how it looks like:

Perhaps put the deprecated after the method name?

bind() (deprecated, use on()) - ...

Sounds good. It looked better in the code but in the screenshot my order looks a bit off, I agree.

We should alse:

Document deprecations with the release in which they will be removed (usually the next minor release).

Place an issue in the appropriate milestone to remind us to do so.

gkalpak · 2016-10-05T12:46:27Z

src/jqLite.js

@@ -56,7 +56,7 @@
 * - [`after()`](http://api.jquery.com/after/)
 * - [`append()`](http://api.jquery.com/append/)
 * - [`attr()`](http://api.jquery.com/attr/) - Does not support functions as parameters
- * - [`bind()`](http://api.jquery.com/bind/) - Does not support namespaces, selectors or eventData
+ * - (_deprecated_, use `on()`) [`bind()`](http://api.jquery.com/bind/) - Does not support namespaces, selectors or eventData


We should alse:

Document deprecations with the release in which they will be removed (usually the next minor release).

Place an issue in the appropriate milestone to remind us to do so.

gkalpak · 2016-10-05T12:49:28Z

test/jqLiteSpec.js

@@ -2256,4 +2334,19 @@ describe('jqLite', function() {
      expect(onLoadCallback).toHaveBeenCalledOnce();
    });
  });
+
+
+  describe('bind()/unbind()', function() {


_{Super-nit: Other descriptions don't have parenthesis 😁}

Don't worry about it

Already changed locally! :D

petebacondarwin · 2016-10-05T13:26:10Z

LGTM

gkalpak

LGTM
_{(Don't forget the issue for removing `bind/unbind' 😃)}

gkalpak · 2016-10-05T14:50:59Z

(Also, feel free to tweak the commit message as per #15181 (review) 😃.)

petebacondarwin · 2016-10-05T20:57:43Z

@mgol - feel free to tweak as suggested by @gkalpak and then merge. Thanks for your work on this.

mgol · 2016-10-06T10:01:05Z

Removal issue: #15221.

…butes jQuery supports removing multiple attributes in one go, jqLite doesn't. This is now documented.

This is done automatically by browsers in cases where it's needed; the workaround was only needed for IE<9. The new behavior means boolean attributes will not be reflected on elements where browsers don't reflect them. This change aligns jqLite with jQuery 3. Fixes angular#14126 BREAKING CHANGE: Previously, all boolean attributes were reflected into properties in a setter and from a property in a getter, even on elements that don't treat those attributes in a special way. Now Angular doesn't do it by itself but relies on browsers to know when to reflect the property. Note that this browser-level conversions differs between browsers; if you need to change dynamic state of an element you should modify the property, not the attribute. See https://jquery.com/upgrade-guide/1.9/#attr-versus-prop- for a more detailed description about a related change in jQuery 1.9. To migrate the code follow the example below: Before: CSS: input[checked="checked"] { ... } JS: elem1.attr('checked', 'checked'); elem2.attr('checked', false); After: CSS: input:checked { ... } JS: elem1.prop('checked', true); elem2.prop('checked', false);

This change aligns jqLite with jQuery. Also, the extra `2` second parameter to `setAttribute` has been removed; it was only needed for IE<9 and latest jQuery doesn't pass it either. Ref angular#15126 BREAKING CHANGE: Invoking `elem.attr(attributeName, null)` would set the `attributeName` atribute value to a string `"null"`, now it removes the attribute instead. To migrate the code follow the example below: Before: elem.attr(attributeName, null); After: elem.attr(attributeName, "null");

This change aligns jqLite with jQuery. Ref angular#15126 BREAKING CHANGE: Before, using the `attr` method with an empty string as a value would remove the boolean attribute. Now it sets it to its lowercase name as was happening for every non-empty string so far. The only two values that remove the boolean attribute are now null & false, just like in jQuery. To migrate the code follow the example below: Before: elem.attr(booleanAttrName, ''); After: elem.attr(booleanAttrName, false); or: elem.attr(booleanAttrName, null);

The attr method was refactored to be divided into setter & getter parts and to handle boolean attributes in each one of them separately instead of dividing into boolean & non-boolean ones and then handling setter & getter in both of them. This is because handling boolean & non-boolean attributes has common parts; in particular handling of the `null` value or using getAttribute to get the value in the getter.

The bind/unbind aliases to on/off were being assinged in every iteration of the function assigning traversal methods to the prototype. Now it happens only once.

The on/off aliases have been available since Angular 1.2. bind/unbind have been deprecated in jQuery 3.0 so we're following suit in jqLite.

jQuery falls back to prop here but this feature is not very well tested & documented so let's just skip it here. Closes angular#15181

mgol added component: jqLite needs: breaking change labels Sep 23, 2016

mgol added this to the 1.6.0-rc.1 milestone Sep 23, 2016

mgol self-assigned this Sep 23, 2016

googlebot added the cla: yes label Sep 23, 2016

mgol force-pushed the jquery branch 2 times, most recently from 6eeb9fa to bd0d5a5 Compare September 28, 2016 12:21

gkalpak reviewed Sep 30, 2016

View reviewed changes

mgol force-pushed the jquery branch from bd0d5a5 to d831abb Compare October 5, 2016 10:35

mgol commented Oct 5, 2016

View reviewed changes

gkalpak reviewed Oct 5, 2016

View reviewed changes

mgol force-pushed the jquery branch from d831abb to 5bc1e0d Compare October 5, 2016 14:06

gkalpak approved these changes Oct 5, 2016

View reviewed changes

mgol added 10 commits October 6, 2016 12:15

docs(jqLite): Document that removeAttr doesn't support multiple attri…

1f16c79

…butes jQuery supports removing multiple attributes in one go, jqLite doesn't. This is now documented.

chore(jqLite): fix a typo in a test name

095cacb

perf(jqLite): move bind/unbind definitions out of the loop

304c765

The bind/unbind aliases to on/off were being assinged in every iteration of the function assigning traversal methods to the prototype. Now it happens only once.

tests(jqLite): add basic tests for the bind/unbind aliases

bb3cfd3

refactor(jqLite): deprecate bind/unbind

d5b7803

The on/off aliases have been available since Angular 1.2. bind/unbind have been deprecated in jQuery 3.0 so we're following suit in jqLite.

feat(jqLite): don't throw for elements with missing getAttribute

4e6c14d

jQuery falls back to prop here but this feature is not very well tested & documented so let's just skip it here. Closes angular#15181

mgol force-pushed the jquery branch from 5bc1e0d to 4e6c14d Compare October 6, 2016 10:15

mgol merged commit 4e6c14d into angular:master Oct 6, 2016

mgol deleted the jquery branch October 6, 2016 10:48

Align jqLite's attr method with jQuery #15181

Align jqLite's attr method with jQuery #15181

Conversation

mgol commented Sep 23, 2016

Narretz commented Sep 26, 2016

mgol commented Sep 28, 2016

mgol commented Sep 29, 2016

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 5, 2016

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak commented Oct 5, 2016

petebacondarwin commented Oct 5, 2016

mgol commented Oct 6, 2016

gkalpak Sep 30, 2016 •

edited

Loading

gkalpak Sep 30, 2016 •

edited

Loading