Fixes #2247. #2250

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
Owner

ibolmo commented Jan 24, 2012

This fixes a nasty regression that custom attributes set by HTML text
(e.g. innerHTML) would previously be considered expando and therefore
thought to have been considered a fake attribute. Therefore, we didn't
return the value.

This fix relies now on outerHTML to check for the existence of the
attribute. Keep in mind this also fixes the previous bug of returning
custom functions since any new el.attribute = are not shown in
outerHTML.

PASSED: IE6-9.

@ibolmo ibolmo Fixes #2247.
This fixes a nasty regression that custom attributes set by HTML text
(e.g. innerHTML) would previously be considered `expando` and therefore
thought to have been considered a fake attribute. Therefore, we didn't
return the value.

This fix relies now on outerHTML to check for the existence of the
attribute. Keep in mind this also fixes the previous bug of returning
custom functions since any new `el.attribute =` are not shown in
outerHTML.

PASSED: IE6-9.
d0208f9
Owner

ibolmo commented Jan 24, 2012

Fixes #2247.

@arian this seems to be performant. At least a single hit, then cached for any further lookups.

@ibolmo ibolmo and 1 other commented on an outdated diff Jan 24, 2012

Source/Element/Element.js
@@ -607,6 +607,9 @@ Element.implement({
} else {
if (value == null){
this.removeAttribute(name);
+ /* <ltIE9> */
+ if (pollutesGetAttribute) delete attributeWhiteList[name];
@ibolmo

ibolmo Jan 24, 2012

Owner

I sneaked this in. Not likely case, but I thought it's necessary to be symmetrical. If we add to the whitelist, we then remove from the white list if we remove the property.

@arian

arian Jan 24, 2012

Owner

euhm, now it removes it for the first element with this attribute, however there can be more elements with this attribute. So if you do .removeProperty('data-foo') other elements that try to get/set this property won't work correctly, right?

@ibolmo

ibolmo Jan 24, 2012

Owner

hrm.. good point .. perhaps attributeWhiteList should be per element. Perhaps in storage.

@arian arian and 1 other commented on an outdated diff Jan 24, 2012

Source/Element/Element.js
@@ -627,8 +630,9 @@ Element.implement({
if (getter) return getter(this);
/* <ltIE9> */
if (pollutesGetAttribute && !attributeWhiteList[name]){
- var attr = this.getAttributeNode(name);
- if (!attr || attr.expando) return null;
+ var outer = this.outerHTML, i = outer.indexOf(name);
+ if ((i < 0) || (i > outer.indexOf('><'))) return null;
@arian

arian Jan 24, 2012

Owner

outer.indexOf('><'): what if the outerHTML is <a href="#">text</a>, then there isn't a >< in that string...

@ibolmo

ibolmo Jan 24, 2012

Owner

I'm going to add that the coverage, and you made me also realize that i could save a couple of ops if I still check for expando. I'll modify this afternoon.

@ibolmo ibolmo attributeWhiteList now per Element, fixed for unnested html, a little
optimization for valid attributes.

PASSED: IE6-9
26e8c54
Owner

ibolmo commented Jan 26, 2012

@arian have another go pls :D

Owner

arian commented Jan 27, 2012

What if we'd have a proper hasAttribute method?

As you probably know these are the results in oldIE:

var div = makeElement('<div data-custom="a">hello</a>');

div.somerandomproperty = 1;

// the comments are the actual outcome.

var attr = div.getAttributeNode('data-custom');
expect(attr.specified).toEqual(true); // true
expect(attr.expando).toEqual(undefined); // true

div.randomproperty = 1;

attr = div.getAttributeNode('somerandomproperty');
expect(attr).toEqual(undefined); // DOMNode
expect(attr && attr.specified).toEqual(undefined); // true
expect(attr && attr.expando).toEqual(undefined); // true

After some research the only solution I found is using outerHTML indeed.

Owner

ibolmo commented Jan 27, 2012

That's a great idea @arian and I'll add a github issue for enhancing the MooTools API for 1.5.

I tried to augment the Slick.hasAttribute but there's a couple of problems.

  1. Slick doesn't need this fix. It's very -- very -- unlikely that anyone would check attribute with value of a function (the original problem).
  2. We don't use setProperty or removeProperty from Slick. Therefore it's very hard to whitelist dynamic properties.

So my initial idea of adding this code to Slick is out of the question. I think we'll have to wait for 1.5 to use a cleaner hasAttribute lookup and then return the property if we should

Owner

ibolmo commented Jan 27, 2012

@arian when you're ready give me the 👍 or 👎

Owner

arian commented Jan 27, 2012

div = new Element('div', {html: '<div class="><" data-custom="other"></div>'}).getFirst();
expect(div.get('data-custom')).toEqual('other');

fails.

@ibolmo ibolmo Adding coverage for even more edge cases.
I enjoy a good stumping. Keep 'em coming @arian. :D
79f2635

arian closed this Jan 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment