Skip to content

Commit

Permalink
Attributes: Make .attr( name, false ) remove for all non-ARIA attrs
Browse files Browse the repository at this point in the history 
The HTML spec defines boolean attributes:
https://html.spec.whatwg.org/#boolean-attributes
that often correlate with boolean properties. If the attribute is missing, it
correlates with the `false` property value, if it's present - the `true`
property value. The only valid values are an empty string or the attribute name.

jQuery tried to be helpful here and treated boolean attributes in a special way
in the `.attr()` API:
1. For the getter, as long as the attribute was present, it was returning the
   attribute name lowercased, ignoring the value.
2. For the setter, it was removing the attribute when `false` was passed;
   otherwise, it was ignoring the passed value and set the attribute -
   interestingly, in jQuery `>=3` not lowercased anymore.

The problem is the spec occasionally converts boolean attributes into ones with
additional attribute values with special behavior - one such example is the new
`"until-found"` value for the `hidden` attribute. Our setter normalization
means passing those values is impossible with jQuery. Also, new boolean
attributes are introduced occasionally and jQuery cannot easily add them to the
list without incurring breaking changes.

This patch removes any special handling of boolean attributes - the getter
returns the value as-is and the setter sets the provided value.

To provide better backwards compatibility with the very frequent `false` value
provided to remove the attribute, this patch makes `false` trigger attribute
removal for ALL non-ARIA attributes. ARIA attributes are exempt from the rule
since many of them recognize `"false"` as a valid value with semantics different
than the attribute missing. To remove an ARIA attribute, use `.removeAttr()` or
pass `null` as the value to `.attr()` which doesn't have this exception.

Fixes gh-5388
Closes gh-5452

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
  • Loading branch information
@mgol @gibson042
mgol and gibson042 committed Mar 19, 2024
1 parent f80e78e commit 063831b
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 48 deletions.
38 changes: 8 additions & 30 deletions src/attributes/attr.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,14 @@ jQuery.extend( {
}

if ( value !== undefined ) {
if ( value === null ) {
if ( value === null ||

// For compat with previous handling of boolean attributes,
// remove when `false` passed. For ARIA attributes -
// many of which recognize a `"false"` value - continue to
// set the `"false"` value as jQuery <4 did.
( value === false && name.toLowerCase().indexOf( "aria-" ) !== 0 ) ) {

jQuery.removeAttr( elem, name );
return;
}
Expand Down Expand Up @@ -96,32 +103,3 @@ if ( isIE ) {
}
};
}

// HTML boolean attributes have special behavior:
// we consider the lowercase name to be the only valid value, so
// getting (if the attribute is present) normalizes to that, as does
// setting to any non-`false` value (and setting to `false` removes the attribute).
// See https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes
jQuery.each( (
"checked selected async autofocus autoplay controls defer disabled " +
"hidden ismap loop multiple open readonly required scoped"
).split( " " ), function( _i, name ) {
jQuery.attrHooks[ name ] = {
get: function( elem ) {
return elem.getAttribute( name ) != null ?
name.toLowerCase() :
null;
},

set: function( elem, value, name ) {
if ( value === false ) {

// Remove boolean attributes when set to false
jQuery.removeAttr( elem, name );
} else {
elem.setAttribute( name, name );
}
return name;
}
};
} );
86 changes: 72 additions & 14 deletions test/unit/attributes.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,12 +304,12 @@ QUnit.test( "attr(String, Object)", function( assert ) {
assert.equal( $input[ 0 ].selected, true, "Setting selected updates property (verified by native property)" );

$input = jQuery( "#check2" );
$input.prop( "checked", true ).prop( "checked", false ).attr( "checked", true );
$input.prop( "checked", true ).prop( "checked", false ).attr( "checked", "checked" );
assert.equal( $input.attr( "checked" ), "checked", "Set checked (verified by .attr)" );
$input.prop( "checked", false ).prop( "checked", true ).attr( "checked", false );
assert.equal( $input.attr( "checked" ), undefined, "Remove checked (verified by .attr)" );

$input = jQuery( "#text1" ).prop( "readOnly", true ).prop( "readOnly", false ).attr( "readonly", true );
$input = jQuery( "#text1" ).prop( "readOnly", true ).prop( "readOnly", false ).attr( "readonly", "readonly" );
assert.equal( $input.attr( "readonly" ), "readonly", "Set readonly (verified by .attr)" );
$input.prop( "readOnly", false ).prop( "readOnly", true ).attr( "readonly", false );
assert.equal( $input.attr( "readonly" ), undefined, "Remove readonly (verified by .attr)" );
Expand All @@ -318,7 +318,7 @@ QUnit.test( "attr(String, Object)", function( assert ) {
assert.equal( $input[ 0 ].checked, true, "Set checked property (verified by native property)" );
assert.equal( $input.prop( "checked" ), true, "Set checked property (verified by .prop)" );
assert.equal( $input.attr( "checked" ), undefined, "Setting checked property doesn't affect checked attribute" );
$input.attr( "checked", false ).attr( "checked", true ).prop( "checked", false );
$input.attr( "checked", false ).attr( "checked", "checked" ).prop( "checked", false );
assert.equal( $input[ 0 ].checked, false, "Clear checked property (verified by native property)" );
assert.equal( $input.prop( "checked" ), false, "Clear checked property (verified by .prop)" );
assert.equal( $input.attr( "checked" ), "checked", "Clearing checked property doesn't affect checked attribute" );
Expand All @@ -345,22 +345,22 @@ QUnit.test( "attr(String, Object)", function( assert ) {

// HTML5 boolean attributes
$text = jQuery( "#text1" ).attr( {
"autofocus": true,
"required": true
"autofocus": "autofocus",
"required": "required"
} );
assert.equal( $text.attr( "autofocus" ), "autofocus", "Reading autofocus attribute yields 'autofocus'" );
assert.equal( $text.attr( "autofocus", false ).attr( "autofocus" ), undefined, "Setting autofocus to false removes it" );
assert.equal( $text.attr( "required" ), "required", "Reading required attribute yields 'required'" );
assert.equal( $text.attr( "required", false ).attr( "required" ), undefined, "Setting required attribute to false removes it" );

$details = jQuery( "<details open></details>" ).appendTo( "#qunit-fixture" );
assert.equal( $details.attr( "open" ), "open", "open attribute presence indicates true" );
assert.equal( $details.attr( "open" ), "", "open attribute presence indicates true" );
assert.equal( $details.attr( "open", false ).attr( "open" ), undefined, "Setting open attribute to false removes it" );

$text.attr( "data-something", true );
assert.equal( $text.attr( "data-something" ), "true", "Set data attributes" );
assert.equal( $text.data( "something" ), true, "Setting data attributes are not affected by boolean settings" );
$text.attr( "data-another", false );
$text.attr( "data-another", "false" );
assert.equal( $text.attr( "data-another" ), "false", "Set data attributes" );
assert.equal( $text.data( "another" ), false, "Setting data attributes are not affected by boolean settings" );
assert.equal( $text.attr( "aria-disabled", false ).attr( "aria-disabled" ), "false", "Setting aria attributes are not affected by boolean settings" );
Expand Down Expand Up @@ -1790,21 +1790,79 @@ QUnit.test( "non-lowercase boolean attribute getters should not crash", function

var elem = jQuery( "<input checked required autofocus type='checkbox'>" );

jQuery.each( {
checked: "Checked",
required: "requiRed",
autofocus: "AUTOFOCUS"
}, function( lowercased, original ) {
[
"Checked", "requiRed", "AUTOFOCUS"
].forEach( function( inconsistentlyCased ) {
try {
assert.strictEqual( elem.attr( original ), lowercased,
"The '" + this + "' attribute getter should return the lowercased name" );
assert.strictEqual( elem.attr( inconsistentlyCased ), "",
"The '" + this + "' attribute getter should return an empty string" );
} catch ( e ) {
assert.ok( false, "The '" + this + "' attribute getter threw" );
}
} );
} );


QUnit.test( "false setter removes non-ARIA attrs (gh-5388)", function( assert ) {
assert.expect( 24 );

var elem = jQuery( "<input" +
" checked required autofocus" +
" type='checkbox'" +
" title='Example title'" +
" class='test-class'" +
" style='color: brown'" +
" aria-hidden='true'" +
" aria-checked='true'" +
" aria-label='Example ARIA label'" +
" data-prop='Example data value'" +
" data-title='Example data title'" +
" data-true='true'" +
">" );

function testFalseSetter( attributes, options ) {
var removal = options.removal;

attributes.forEach( function( attrName ) {
assert.ok( elem.attr( attrName ) != null,
"Attribute '" + attrName + "': initial defined value" );
elem.attr( attrName, false );

if ( removal ) {
assert.strictEqual( elem.attr( attrName ), undefined,
"Attribute '" + attrName + "' removed" );
} else {
assert.strictEqual( elem.attr( attrName ), "false",
"Attribute '" + attrName + "' set to 'false'" );
}
} );
}

// Boolean attributes
testFalseSetter(
[ "checked", "required", "autofocus" ],
{ removal: true }
);

// Regular attributes
testFalseSetter(
[ "title", "class", "style" ],
{ removal: true }
);

// `aria-*` attributes
testFalseSetter(
[ "aria-hidden", "aria-checked", "aria-label" ],
{ removal: false }
);

// `data-*` attributes
testFalseSetter(
[ "data-prop", "data-title", "data-true" ],
{ removal: true }
);
} );

// Test trustedTypes support in browsers where they're supported (currently Chrome 83+).
// Browsers with no TrustedScriptURL support still run tests on object wrappers with
// a proper `toString` function.
Expand Down
8 changes: 4 additions & 4 deletions test/unit/selector.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -948,10 +948,10 @@ QUnit.test( "pseudo - nth-last-of-type", function( assert ) {
QUnit[ QUnit.jQuerySelectors ? "test" : "skip" ]( "pseudo - has", function( assert ) {
assert.expect( 4 );

assert.t( "Basic test", "p:has(a)", [ "firstp", "ap", "en", "sap" ] );
assert.t( "Basic test (irrelevant whitespace)", "p:has( a )", [ "firstp", "ap", "en", "sap" ] );
assert.t( "Nested with overlapping candidates",
"#qunit-fixture div:has(div:has(div:not([id])))",
assert.selectInFixture( "Basic test", "p:has(a)", [ "firstp", "ap", "en", "sap" ] );
assert.selectInFixture( "Basic test (irrelevant whitespace)", "p:has( a )", [ "firstp", "ap", "en", "sap" ] );
assert.selectInFixture( "Nested with overlapping candidates",
"div:has(div:has(div:not([id])))",
[ "moretests", "t2037", "fx-test-group", "fx-queue" ] );

// Support: Safari 15.4+, Chrome 105+
Expand Down

0 comments on commit 063831b

Please sign in to comment.