8000 Attributes: Make `.attr( name, false )` remove for all non-ARIA attrs · mgol/jquery@e68ed59 · GitHub
[go: up one dir, main page]
Skip to content

Commit e68ed59

Browse files
mgolgibson042
mgol
and
gibson042
committed
Attributes: Make .attr( name, false ) remove for all non-ARIA attrs
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 jquerygh-5388 Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
1 parent f80e78e commit e68ed59

File tree

3 files changed

+84
-48
lines changed

3 files changed

+84
-48
lines changed

‎src/attributes/attr.js

Lines changed: 8 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,14 @@ jQuery.extend( {
3838
}
3939

4040
if ( value !== undefined ) {
41-
if ( value === null ) {
41+
if ( value === null ||
42+
43+
// For compat with previous handling of boolean attributes,
44+
// remove when `false` passed. For ARIA attributes -
45+
// many of which recognize a `"false"` value - continue to
46+
// set the `"false"` value as jQuery <4 did.
47+
( value === false && name.toLowerCase().indexOf( "aria-" ) !== 0 ) ) {
48+
4249
jQuery.removeAttr( elem, name );
4350
return;
4451
}
@@ -96,32 +103,3 @@ if ( isIE ) {
96103
}
97104
};
98105
}
99-
100-
// HTML boolean attributes have special behavior:
101-
// we consider the lowercase name to be the only valid value, so
102-
// getting (if the attribute is present) normalizes to that, as does
103-
// setting to any non-`false` value (and setting to `false` removes the attribute).
104-
// See https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes
105-
jQuery.each( (
106-
"checked selected async autofocus autoplay controls defer disabled " +
107-
"hidden ismap loop multiple open readonly required scoped"
108-
).split( " " ), function( _i, name ) {
109-
jQuery.attrHooks[ name ] = {
110-
get: function( elem ) {
111-
return elem.getAttribute( name ) != null ?
112-
name.toLowerCase() :
113-
null;
114-
},
115-
116-
set: function( elem, value, name ) {
117-
if ( value === false ) {
118-
119-
// Remove boolean attributes when set to false
120-
jQuery.removeAttr( elem, name );
121-
} else {
122-
elem.setAttribute( name, name );
123-
}
124-
return name;
125-
}
126-
};
127-
} );

‎test/unit/attributes.js

Lines changed: 72 additions & 14 deletions
+
" title='Example title'" +
Original file line numberDiff line numberDiff line change
@@ -304,12 +304,12 @@ QUnit.test( "attr(String, Object)", function( assert ) {
304304
assert.equal( $input[ 0 ].selected, true, "Setting selected updates property (verified by native property)" );
305305

306306
$input = jQuery( "#check2" );
307-
$input.prop( "checked", true ).prop( "checked", false ).attr( "checked", true );
307+
$input.prop( "checked", true ).prop( "checked", false ).attr( "checked", "checked" );
308308
assert.equal( $input.attr( "checked" ), "checked", "Set checked (verified by .attr)" );
309309
$input.prop( "checked", false ).prop( "checked", true ).attr( "checked", false );
310310
assert.equal( $input.attr( "checked" ), undefined, "Remove checked (verified by .attr)" );
311311

312-
$input = jQuery( "#text1" ).prop( "readOnly", true ).prop( "readOnly", false ).attr( "readonly", true );
312+
$input = jQuery( "#text1" ).prop( "readOnly", true ).prop( "readOnly", false ).attr( "readonly", "readonly" );
313313
assert.equal( $input.attr( "readonly" ), "readonly", "Set readonly (verified by .attr)" );
314314
$input.prop( "readOnly", false ).prop( "readOnly", true ).attr( "readonly", false );
315315
assert.equal( $input.attr( "readonly" ), undefined, "Remove readonly (verified by .attr)" );
@@ -318,7 +318,7 @@ QUnit.test( "attr(String, Object)", function( assert ) {
318318
assert.equal( $input[ 0 ].checked, true, "Set checked property (verified by native property)" );
319319
assert.equal( $input.prop( "checked" ), true, "Set checked property (verified by .prop)" );
320320
assert.equal( $input.attr( "checked" ), undefined, "Setting checked property doesn't affect checked attribute" );
321-
$input.attr( "checked", false ).attr( "checked", true ).prop( "checked", false );
321+
$input.attr( "checked", false ).attr( "checked", "checked" ).prop( "checked", false );
322322
assert.equal( $input[ 0 ].checked, false, "Clear checked property (verified by native property)" );
323323
assert.equal( $input.prop( "checked" ), false, "Clear checked property (verified by .prop)" );
324324
assert.equal( $input.attr( "checked" ), "checked", "Clearing checked property doesn't affect checked attribute" );
@@ -345,22 +345,22 @@ QUnit.test( "attr(String, Object)", function( assert ) {
345345

346346
// HTML5 boolean attributes
347347
$text = jQuery( "#text1" ).attr( {
348-
"autofocus": true,
349-
"required": true
348+
"autofocus": "autofocus",
349+
"required": "required"
350350
} );
351351
assert.equal( $text.attr( "autofocus" ), "autofocus", "Reading autofocus attribute yields 'autofocus'" );
352352
assert.equal( $text.attr( "autofocus", false ).attr( "autofocus" ), undefined, "Setting autofocus to false removes it" );
353353
assert.equal( $text.attr( "required" ), "required", "Reading required attribute yields 'required'" );
354354
assert.equal( $text.attr( "required", false ).attr( "required" ), undefined, "Setting required attribute to false removes it" );
355355

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

360360
$text.attr( "data-something", true );
361361
assert.equal( $text.attr( "data-something" ), "true", "Set data attributes" );
362362
assert.equal( $text.data( "something" ), true, "Setting data attributes are not affected by boolean settings" );
363-
$text.attr( "data-another", false );
363+
$text.attr( "data-another", "false" );
364364
assert.equal( $text.attr( "data-another" ), "false", "Set data attributes" );
365365
assert.equal( $text.data( "another" ), false, "Setting data attributes are not affected by boolean settings" );
366366
assert.equal( $text.attr( "aria-disabled", false ).attr( "aria-disabled" ), "false", "Setting aria attributes are not affected by boolean settings" );
@@ -1790,21 +1790,79 @@ QUnit.test( "non-lowercase boolean attribute getters should not crash", function
17901790

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

1793-
jQuery.each( {
1794-
checked: "Checked",
1795-
required: "requiRed",
1796-
autofocus: "AUTOFOCUS"
1797-
}, function( lowercased, original ) {
1793+
[
1794+
"Checked", "requiRed", "AUTOFOCUS"
1795+
].forEach( function( inconsistentlyCased ) {
17981796
try {
1799-
assert.strictEqual( elem.attr( original ), lowercased,
1800-
"The '" + this + "' attribute getter should return the lowercased name" );
1797+
assert.strictEqual( elem.attr( inconsistentlyCased ), "",
1798+
"The '" + this + "' attribute getter should return an empty string" );
18011799
} catch ( e ) {
18021800
assert.ok( false, "The '" + this + "' attribute getter threw" );
18031801
}
18041802
} );
18051803
} );
18061804

18071805

1806+
QUnit.test( "false setter removes non-ARIA attrs (gh-5388)", function( assert ) {
1807+
assert.expect( 24 );
1808+
1809+
var elem = jQuery( "<input" +
1810+
" checked required autofocus" +
1811+
" type='checkbox'" +
1812
1813+
" class='test-class'" +
1814+
" style='color: brown'" +
1815+
" aria-hidden='true'" +
1816+
" aria-checked='true'" +
1817+
" aria-label='Example ARIA label'" +
1818+
" data-prop='Example data value'" +
1819+
" data-title='Example data title'" +
1820+
" data-true='true'" +
1821+
">" );
1822+
1823+
function testFalseSetter( attributes, options ) {
1824+
var removal = options.removal;
1825+
1826+
attributes.forEach( function( attrName ) {
1827+
assert.ok( elem.attr( attrName ) != null,
1828+
"Attribute '" + attrName + "': initial defined value" );
1829+
elem.attr( attrName, false );
1830+
1831+
if ( removal ) {
1832+
assert.strictEqual( elem.attr( attrName ), undefined,
1833+
"Attribute '" + attrName + "' removed" );
1834+
} else {
1835+
assert.strictEqual( elem.attr( attrName ), "false",
1836+
"Attribute '" + attrName + "' set to 'false'" );
1837+
}
1838+
} );
1839+
}
1840+
1841+
// Boolean attributes
1842+
testFalseSetter(
1843+
[ "checked", "required", "autofocus" ],
1844+
{ removal: true }
1845+
);
1846+
1847+
// Regular attributes
1848+
testFalseSetter(
1849+
[ "title", "class", "style" ],
1850+
{ removal: true }
1851+
);
1852+
1853+
// `aria-*` attributes
1854+
testFalseSetter(
1855+
[ "aria-hidden", "aria-checked", "aria-label" ],
1856+
{ removal: false }
1857+
);
1858+
1859+
// `data-*` attributes
1860+
testFalseSetter(
1861+
[ "data-prop", "data-title", "data-true" ],
1862+
{ removal: true }
1863+
);
1864+
} );
1865+
18081866
// Test trustedTypes support in browsers where they're supported (currently Chrome 83+).
18091867
// Browsers with no TrustedScriptURL support still run tests on object wrappers with
18101868
// a proper `toString` function.

‎test/unit/selector.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -948,10 +948,10 @@ QUnit.test( "pseudo - nth-last-of-type", function( assert ) {
948948
QUnit[ QUnit.jQuerySelectors ? "test" : "skip" ]( "pseudo - has", function( assert ) {
949949
assert.expect( 4 );
950950

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

957957
// Support: Safari 15.4+, Chrome 105+

0 commit comments

Comments
 (0)
0