Permalink
Browse files

Add a hook for removing contenteditable in IE6/7 and remove the unnec…

…essary jQuery.attrFix. Fixes #10429.
  • Loading branch information...
1 parent e5b16e3 commit ce8d9c0ca59a8f03e119c80ed29c7dbc65efdd85 @timmywil timmywil committed Oct 6, 2011
Showing with 31 additions and 32 deletions.
  1. +20 −23 src/attributes.js
  2. +11 −9 test/unit/attributes.js
View
@@ -291,11 +291,6 @@ jQuery.extend({
offset: true
},
- attrFix: {
- // Always normalize to ensure hook usage
- tabindex: "tabIndex"
- },
-
attr: function( elem, name, value, pass ) {
var nType = elem.nodeType;
@@ -318,20 +313,8 @@ jQuery.extend({
// Normalize the name if needed
if ( notxml ) {
- name = jQuery.attrFix[ name ] || name;
-
- hooks = jQuery.attrHooks[ name ];
-
- if ( !hooks ) {
- // Use boolHook for boolean attributes
- if ( rboolean.test( name ) ) {
- hooks = boolHook;
-
- // Use nodeHook if available( IE6/7 )
- } else if ( nodeHook ) {
- hooks = nodeHook;
- }
- }
+ name = name.toLowerCase();
+ hooks = jQuery.attrHooks[ name ] || (rboolean.test( name ) ? boolHook : nodeHook);
}
if ( value !== undefined ) {
@@ -371,8 +354,7 @@ jQuery.extend({
l = attrNames.length;
for ( ; i < l; i++ ) {
- name = attrNames[ i ];
- name = jQuery.attrFix[ name ] || name;
+ name = attrNames[ i ].toLowerCase();
// See #9699 for explanation of this approach (setting first, then removal)
jQuery.attr( elem, name, "" );
@@ -493,8 +475,8 @@ jQuery.extend({
}
});
-// Add the tabindex propHook to attrHooks for back-compat
-jQuery.attrHooks.tabIndex = jQuery.propHooks.tabIndex;
+// Add the tabIndex propHook to attrHooks for back-compat (different case is intentional)
+jQuery.attrHooks.tabindex = jQuery.propHooks.tabIndex;
// Hook for boolean attributes
boolHook = {
@@ -556,6 +538,9 @@ if ( !jQuery.support.getSetAttribute ) {
}
};
+ // Apply the nodeHook to tabindex
+ jQuery.attrHooks.tabindex.set = nodeHook.set;
+
// Set width and height to auto instead of 0 on empty string( Bug #8150 )
// This is for removals
jQuery.each([ "width", "height" ], function( i, name ) {
@@ -568,6 +553,18 @@ if ( !jQuery.support.getSetAttribute ) {
}
});
});
+
+ // Set contenteditable to false on removals(#10429)
+ // Setting to empty string throws an error as an invalid value
+ jQuery.attrHooks.contenteditable = {
+ get: nodeHook.get,
+ set: function( elem, value, name ) {
+ if ( value === "" ) {
+ value = "false";
+ }
+ nodeHook.set( elem, value, name );
+ }
+ };
}
View
@@ -4,8 +4,8 @@ var bareObj = function(value) { return value; };
var functionReturningObj = function(value) { return (function() { return value; }); };
-test("jQuery.attrFix/jQuery.propFix integrity test", function() {
- expect(2);
+test("jQuery.propFix integrity test", function() {
+ expect(1);
// This must be maintained and equal jQuery.attrFix when appropriate
// Ensure that accidental or erroneous property
@@ -26,11 +26,6 @@ test("jQuery.attrFix/jQuery.propFix integrity test", function() {
contenteditable: "contentEditable"
};
- var propsShouldBe = {
- tabindex: "tabIndex"
- };
-
- deepEqual(propsShouldBe, jQuery.attrFix, "jQuery.attrFix passes integrity check");
deepEqual(props, jQuery.propFix, "jQuery.propFix passes integrity check");
});
@@ -456,7 +451,7 @@ test("attr('tabindex', value)", function() {
});
test("removeAttr(String)", function() {
- expect(8);
+ expect(9);
equal( jQuery("#mark").removeAttr( "class" )[0].className, "", "remove class" );
equal( jQuery("#form").removeAttr("id").attr("id"), undefined, "Remove id" );
equal( jQuery("#foo").attr("style", "position:absolute;").removeAttr("style").attr("style"), undefined, "Check removing style attribute" );
@@ -468,6 +463,13 @@ test("removeAttr(String)", function() {
equal( document.getElementById("check1").checked, false, "removeAttr sets boolean properties to false" );
jQuery("#text1").prop("readOnly", true).removeAttr("readonly");
equal( document.getElementById("text1").readOnly, false, "removeAttr sets boolean properties to false" );
+
+ try {
+ jQuery("#first").attr("contenteditable", "true").removeAttr("contenteditable");
+ ok( true, "Removing contenteditable does not throw an error.");
@staabm

staabm Oct 7, 2011

Contributor

Shouldn't you use pass() and fail() here... ?

@timmywil

timmywil Oct 7, 2011

Owner

I don't know of any pass/fail functions in QUnit. There is 'raises', but that's not really what I need here. I could have done this without the try/catch, but that is a toss-up to me because I'd like to have other tests continue even if this fails.

@staabm

staabm Oct 7, 2011

Contributor

you are right... thought there were such methods, but they don't exist...

other tests in this suite use a boolean flag to indicate whether an exception was thrown and therefore use only 1 "ok()" call to signal the state of the test. but thats just a little thing i think.

+ } catch(e) {
+ ok( false, "Removing contenteditable threw an error (#10429)" );
+ }
});
test("prop(String, Object)", function() {
@@ -882,7 +884,7 @@ var testAddClass = function(valueObj) {
equals( div.attr("class"), "foo bar baz", "Make sure there isn't too much trimming." );
div.removeClass();
- div.addClass( valueObj("foo") ).addClass( valueObj("foo") )
+ div.addClass( valueObj("foo") ).addClass( valueObj("foo") );
equal( div.attr("class"), "foo", "Do not add the same class twice in separate calls." );
div.addClass( valueObj("fo") );

0 comments on commit ce8d9c0

Please sign in to comment.