Fix #2100. Element.erase caused unexpected values for the attributes #2140

Merged
merged 2 commits into from Nov 29, 2011

2 participants

@ibolmo
MooTools member

Added a feature detect if the browser sets 'null' instead of null in the
attribute.

Added a Element.Property for class.

TESTED: Opera 11, Safari 5, Firefox 3-5, 8, 10, Chrome latest, IE6-9.

@ibolmo ibolmo Fix #2000. Element.erase class and other properties later Element.get…
… the unexpected results.

Added a feature detect if the browser sets 'null' instead of null in the
attribute.

Added a `Element.Property` for class.

TESTED: Opera 11, Safari 5, Firefox 3-5, 8, 10, Chrome latest, IE6-9.
da72cc6
@ibolmo
MooTools member

Fixes #2100.

@cpojer cpojer and 1 other commented on an outdated diff Nov 28, 2011
Source/Element/Element.js
@@ -863,6 +868,20 @@ Element.Properties.style = {
};
+if ('className' in document.documentElement){
@cpojer
MooTools member
cpojer added a line comment Nov 28, 2011

When would this be false?

@ibolmo
MooTools member
ibolmo added a line comment Nov 28, 2011

Yeah I thought it strange too, but as you noticed in L557, we still check against it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cpojer cpojer and 1 other commented on an outdated diff Nov 28, 2011
Source/Element/Element.js
@@ -863,6 +868,20 @@ Element.Properties.style = {
};
+if ('className' in document.documentElement){
+ Element.Properties['class'] = {
@cpojer
MooTools member
cpojer added a line comment Nov 28, 2011

This means you can get rid of this:

https://github.com/ibolmo/mootools-core/blob/da72cc67db4c11364e1430936d540222e3746774/Source/Element/Element.js#L557-559

However, isn't this unrelated to the fix for erase?

@ibolmo
MooTools member
ibolmo added a line comment Nov 28, 2011

Yep, I didn't want to disturb the code in the first commit.

See: http://jsfiddle.net/yVMfJ/1/

For failing case of erase('class');

@ibolmo
MooTools member
ibolmo added a line comment Nov 28, 2011

Also take a look (and comment) on this new issue: #2141

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ibolmo
MooTools member
@ibolmo ibolmo Simpler fix for #2100.
Removed unnecessary Element.Properties.class and opted to use
propertySetter and getter. Adding for condition to set the value to ''
instead of null (IE would set to 'null').

Refactored setProperty. Now much simpler. Checks for propertySetter and
assumes it'll take responsibility in cleaning up the value (and to set
it). Still removeAttribute if no setter, and value is null. Passes all
specs, and therefore, it's backwards compatible.

PASSED: IE6-9, FFx 3-5, 8, 10, Chrome latest, Safari 5, Opera 11.
9726eff
@ibolmo
MooTools member

Naice. Looks like this fixes #2108 as well

@cpojer cpojer merged commit edb0324 into mootools:master Nov 29, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment