Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Align jqLite's attr method with jQuery #15181

Merged
merged 10 commits into from
Oct 6, 2016
Merged

Align jqLite's attr method with jQuery #15181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1f16c79
docs(jqLite): Document that removeAttr doesn't support multiple attri…
mgol Sep 21, 2016
7ceb5f6
refactor(jqLite): Don't get/set props when getting/setting bool attrs
mgol Sep 21, 2016
4e36245
feat(jqLite): remove the attribute for .attr(attribute, null)
mgol Sep 23, 2016
3faf450
feat(jqLite): don't remove a boolean attribute for .attr(attrName, '')
mgol Sep 23, 2016
c8ba433
refactor(jqLite): refactor the attr method
mgol Sep 23, 2016
095cacb
chore(jqLite): fix a typo in a test name
mgol Sep 23, 2016
304c765
perf(jqLite): move bind/unbind definitions out of the loop
mgol Oct 5, 2016
bb3cfd3
tests(jqLite): add basic tests for the bind/unbind aliases
mgol Oct 5, 2016
d5b7803
refactor(jqLite): deprecate bind/unbind
mgol Oct 5, 2016
4e6c14d
feat(jqLite): don't throw for elements with missing getAttribute
mgol Oct 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 28 additions & 28 deletions src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
* - [`bind()`](http://api.jquery.com/bind/) (_deprecated_ - to be removed in 1.7.0, use [`on()`](http://api.jquery.com/on/)) - Does not support namespaces, selectors or eventData
* - [`children()`](http://api.jquery.com/children/) - Does not support selectors
* - [`clone()`](http://api.jquery.com/clone/)
* - [`contents()`](http://api.jquery.com/contents/)
Expand All @@ -78,14 +78,14 @@
* - [`prop()`](http://api.jquery.com/prop/)
* - [`ready()`](http://api.jquery.com/ready/)
* - [`remove()`](http://api.jquery.com/remove/)
* - [`removeAttr()`](http://api.jquery.com/removeAttr/)
* - [`removeAttr()`](http://api.jquery.com/removeAttr/) - Does not support multiple attributes
* - [`removeClass()`](http://api.jquery.com/removeClass/) - Does not support a function as first argument
* - [`removeData()`](http://api.jquery.com/removeData/)
* - [`replaceWith()`](http://api.jquery.com/replaceWith/)
* - [`text()`](http://api.jquery.com/text/)
* - [`toggleClass()`](http://api.jquery.com/toggleClass/) - Does not support a function as first argument
* - [`triggerHandler()`](http://api.jquery.com/triggerHandler/) - Passes a dummy event object to handlers
* - [`unbind()`](http://api.jquery.com/unbind/) - Does not support namespaces or event object as parameter
* - [`unbind()`](http://api.jquery.com/unbind/) (_deprecated_ - to be removed in 1.7.0, use [`off()`](http://api.jquery.com/off/)) - Does not support namespaces or event object as parameter
* - [`val()`](http://api.jquery.com/val/)
* - [`wrap()`](http://api.jquery.com/wrap/)
*
Expand Down Expand Up @@ -638,33 +638,33 @@ forEach({
},

attr: function(element, name, value) {
var ret;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var nodeType = element.nodeType;
if (nodeType === NODE_TYPE_TEXT || nodeType === NODE_TYPE_ATTRIBUTE || nodeType === NODE_TYPE_COMMENT) {
if (nodeType === NODE_TYPE_TEXT || nodeType === NODE_TYPE_ATTRIBUTE || nodeType === NODE_TYPE_COMMENT ||
!element.getAttribute) {
return;
}

var lowercasedName = lowercase(name);
if (BOOLEAN_ATTR[lowercasedName]) {
if (isDefined(value)) {
if (value) {
element[name] = true;
element.setAttribute(name, lowercasedName);
} else {
element[name] = false;
element.removeAttribute(lowercasedName);
}
var isBooleanAttr = BOOLEAN_ATTR[lowercasedName];

if (isDefined(value)) {
// setter

if (value === null || (value === false && isBooleanAttr)) {
element.removeAttribute(name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
return (element[name] ||
(element.attributes.getNamedItem(name) || noop).specified)
? lowercasedName
: undefined;
element.setAttribute(name, isBooleanAttr ? lowercasedName : value);
}
} else if (isDefined(value)) {
element.setAttribute(name, value);
} 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);
// normalize non-existing attributes to undefined (as jQuery)
} else {
// getter

ret = element.getAttribute(name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (isBooleanAttr && ret !== null) {
ret = lowercasedName;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Normalize non-existing attributes to undefined (as jQuery).
return ret === null ? undefined : ret;
}
},
Expand Down Expand Up @@ -1061,12 +1061,12 @@ forEach({
}
return isDefined(value) ? value : this;
};

// bind legacy bind/unbind to on/off
JQLite.prototype.bind = JQLite.prototype.on;
JQLite.prototype.unbind = JQLite.prototype.off;
});

// bind legacy bind/unbind to on/off
JQLite.prototype.bind = JQLite.prototype.on;
JQLite.prototype.unbind = JQLite.prototype.off;


// Provider for private $$jqLite service
/** @this */
Expand Down
95 changes: 94 additions & 1 deletion test/jqLiteSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ describe('jqLite', function() {


describe('attr', function() {
it('should read write and remove attr', function() {
it('should read, write and remove attr', function() {
var selector = jqLite([a, b]);

expect(selector.attr('prop', 'value')).toEqual(selector);
Expand Down Expand Up @@ -635,6 +635,43 @@ describe('jqLite', function() {
expect(select.attr('multiple')).toBe('multiple');
});

it('should not take properties into account when getting respective boolean attributes', function() {
// Use a div and not a select as the latter would itself reflect the multiple attribute
// to a property.
var div = jqLite('<div>');

div[0].multiple = true;
expect(div.attr('multiple')).toBe(undefined);

div.attr('multiple', 'multiple');
div[0].multiple = false;
expect(div.attr('multiple')).toBe('multiple');
});

it('should not set properties when setting respective boolean attributes', function() {
// jQuery 2.x has different behavior; skip the test.
if (isJQuery2x()) return;

// Use a div and not a select as the latter would itself reflect the multiple attribute
// to a property.
var div = jqLite('<div>');

// Check the initial state.
expect(div[0].multiple).toBe(undefined);

div.attr('multiple', 'multiple');
expect(div[0].multiple).toBe(undefined);

div.attr('multiple', '');
expect(div[0].multiple).toBe(undefined);

div.attr('multiple', false);
expect(div[0].multiple).toBe(undefined);

div.attr('multiple', null);
expect(div[0].multiple).toBe(undefined);
});

it('should normalize the case of boolean attributes', function() {
var input = jqLite('<input readonly>');
expect(input.attr('readonly')).toBe('readonly');
Expand Down Expand Up @@ -684,6 +721,47 @@ describe('jqLite', function() {
expect(comment.attr('some-attribute','somevalue')).toEqual(comment);
expect(comment.attr('some-attribute')).toBeUndefined();
});

it('should remove the attribute for a null value', function() {
var elm = jqLite('<div attribute="value">a</div>');
elm.attr('attribute', null);
expect(elm[0].hasAttribute('attribute')).toBe(false);
});

it('should not remove the attribute for an empty string as a value', function() {
var elm = jqLite('<div attribute="value">a</div>');
elm.attr('attribute', '');
expect(elm[0].getAttribute('attribute')).toBe('');
});

it('should remove the boolean attribute for a false value', function() {
var elm = jqLite('<select multiple>');
elm.attr('multiple', false);
expect(elm[0].hasAttribute('multiple')).toBe(false);
});

it('should remove the boolean attribute for a null value', function() {
var elm = jqLite('<select multiple>');
elm.attr('multiple', null);
expect(elm[0].hasAttribute('multiple')).toBe(false);
});

it('should not remove the boolean attribute for an empty string as a value', function() {
var elm = jqLite('<select multiple>');
elm.attr('multiple', '');
expect(elm[0].getAttribute('multiple')).toBe('multiple');
});

it('should not fail on elements without the getAttribute method', function() {
forEach([window, document], function(node) {
expect(function() {
var elem = jqLite(node);
elem.attr('foo');
elem.attr('bar', 'baz');
elem.attr('bar');
}).not.toThrow();
});
});
});


Expand Down Expand Up @@ -2256,4 +2334,19 @@ describe('jqLite', function() {
expect(onLoadCallback).toHaveBeenCalledOnce();
});
});


describe('bind/unbind', function() {
if (!_jqLiteMode) return;

it('should alias bind() to on()', function() {
var element = jqLite(a);
expect(element.bind).toBe(element.on);
});

it('should alias unbind() to off()', function() {
var element = jqLite(a);
expect(element.unbind).toBe(element.off);
});
});
});