From 39ccd8b7190e1292ececb53993b56c786dbddcee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Go=C5=82e=CC=A8biowski-Owczarek?= Date: Tue, 8 Oct 2024 02:10:39 +0200 Subject: [PATCH] Attributes: Restore boolean attribute & false setter treatment from 3.x Restore & warn against: * boolean attributes set to something different than their lowercase names * boolean attributes queried when set to something different than their lowercase names * non-boolean non-ARIA attributes set to `false` Fixes gh-504 Ref jquery/jquery#5452 Ref jquery/api.jquery.com#1243 --- src/jquery/attributes.js | 111 ++++++++++++++- test/unit/jquery/attributes.js | 249 ++++++++++++++++++++++++++++++--- warnings.md | 13 ++ 3 files changed, 354 insertions(+), 19 deletions(-) diff --git a/src/jquery/attributes.js b/src/jquery/attributes.js index c58a6852..c5a117f9 100644 --- a/src/jquery/attributes.js +++ b/src/jquery/attributes.js @@ -1,9 +1,116 @@ import { migratePatchFunc, migrateWarn } from "../main.js"; +import { jQueryVersionSince } from "../compareVersions.js"; var oldRemoveAttr = jQuery.fn.removeAttr, + oldJQueryAttr = jQuery.attr, oldToggleClass = jQuery.fn.toggleClass, - rbooleans = /^(?:checked|selected|async|autofocus|autoplay|controls|defer|disabled|hidden|ismap|loop|multiple|open|readonly|required|scoped)$/i, - rmatchNonSpace = /\S+/g; + booleans = "checked|selected|async|autofocus|autoplay|controls|defer|" + + "disabled|hidden|ismap|loop|multiple|open|readonly|required|scoped", + rbooleans = new RegExp( "^(?:" + booleans + ")$", "i" ), + rmatchNonSpace = /\S+/g, + + // Some formerly boolean attributes gained new values with special meaning. + // Skip the old boolean attr logic for those values. + extraBoolAttrValues = { + hidden: [ "until-found" ] + }; + +// 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( booleans.split( "|" ), function( _i, name ) { + var origAttrHooks = jQuery.attrHooks[ name ] || {}; + jQuery.attrHooks[ name ] = { + get: origAttrHooks.get || function( elem ) { + var attrValue; + + if ( jQuery.migrateIsPatchEnabled( "boolean-attributes" ) ) { + attrValue = elem.getAttribute( name ); + + if ( attrValue !== name && attrValue != null && + ( extraBoolAttrValues[ name ] || [] ).indexOf( attrValue ) === -1 + ) { + migrateWarn( "boolean-attributes", + "Boolean attribute '" + name + + "' value is different from its lowercased name" ); + + // jQuery <4 attr hooks setup is complex: there are attr + // hooks, bool hooks and selector attr handles. Only + // implement the logic in jQuery >=4 where it's missing + // and there are only attr hooks. + if ( jQueryVersionSince( "4.0.0" ) ) { + return name.toLowerCase(); + } + return null; + } + } + + return null; + }, + + set: origAttrHooks.set || function( elem, value, name ) { + if ( jQuery.migrateIsPatchEnabled( "boolean-attributes" ) ) { + if ( value !== name && + ( extraBoolAttrValues[ name ] || [] ).indexOf( value ) === -1 + ) { + if ( value !== false ) { + migrateWarn( "boolean-attributes", + "Boolean attribute '" + name + + "' is not set to its lowercased name" ); + } + + if ( value === false ) { + + // Remove boolean attributes when set to false + jQuery.removeAttr( elem, name ); + } else { + elem.setAttribute( name, name ); + } + return name; + } + } else if ( !jQueryVersionSince( "4.0.0" ) ) { + + // jQuery <4 uses a private `boolHook` for the boolean attribute + // setter. It's only activated if `attrHook` is not set, but we set + // it here in Migrate. Since we cannot access it, let's just repeat + // its contents here. + if ( value === false ) { + + // Remove boolean attributes when set to false + jQuery.removeAttr( elem, name ); + } else { + elem.setAttribute( name, name ); + } + return name; + } + } + }; +} ); + +migratePatchFunc( jQuery, "attr", function( elem, name, value ) { + var nType = elem.nodeType; + + // Fallback to the original method on text, comment and attribute nodes + // and when attributes are not supported. + if ( nType === 3 || nType === 8 || nType === 2 || + typeof elem.getAttribute === "undefined" ) { + return oldJQueryAttr.apply( this, arguments ); + } + + if ( value === false && name.toLowerCase().indexOf( "aria-" ) !== 0 && + !rbooleans.test( name ) ) { + migrateWarn( "attr-false", + "Setting the non-ARIA non-boolean attribute '" + name + + "' to false" ); + + jQuery.attr( elem, name, "false" ); + return; + } + + return oldJQueryAttr.apply( this, arguments ); +}, "attr-false" ); migratePatchFunc( jQuery.fn, "removeAttr", function( name ) { var self = this, diff --git a/test/unit/jquery/attributes.js b/test/unit/jquery/attributes.js index e4d32fd1..f336303e 100644 --- a/test/unit/jquery/attributes.js +++ b/test/unit/jquery/attributes.js @@ -1,23 +1,236 @@ - QUnit.module( "attributes" ); +( function() { + function runTests( options ) { + var patchEnabled = options.patchEnabled; + var stockJq4 = jQueryVersionSince( "4.0.0" ) && !patchEnabled; + + function ifOn( warningsCount ) { + return patchEnabled ? warningsCount : 0; + } + + QUnit.test( ".attr( boolean attribute ) - patch " + + ( patchEnabled ? "enabled" : "disabled" ), + function( assert ) { + assert.expect( 31 ); + + if ( !patchEnabled ) { + jQuery.migrateDisablePatches( "boolean-attributes" ); + } + + expectNoWarning( assert, "setting value to null", function() { + var $checkbox = jQuery( "" ); + + $checkbox.attr( "checked", null ); + assert.equal( + $checkbox.attr( "checked" ), + undefined, + "Remove checked by setting to null (verified by .attr)" + ); + } ); + + expectWarning( assert, "setting value to true", ifOn( 1 ), function() { + var $checkbox = jQuery( "" ); + + $checkbox.prop( "checked", true ).prop( "checked", false ).attr( "checked", true ); + assert.equal( + $checkbox.attr( "checked" ), + stockJq4 ? "true" : "checked", + "Set checked (verified by .attr)" + ); + } ); + + expectWarning( assert, "value-less inline attributes", ifOn( 3 ), function() { + var $checkbox = jQuery( "" ); + + jQuery.each( { + checked: "Checked", + required: "requiRed", + autofocus: "AUTOFOCUS" + }, function( lowercased, original ) { + try { + assert.strictEqual( + $checkbox.attr( original ), + stockJq4 ? "" : lowercased, + "The '" + this + + "' attribute getter should return " + + ( stockJq4 ? "an empty string" : "the lowercased name" ) + ); + } catch ( _ ) { + assert.ok( false, "The '" + this + "' attribute getter threw" ); + } + } ); + } ); + + expectWarning( assert, "checked: true", ifOn( 1 ), function() { + var $checkbox = jQuery( "" ); + $checkbox + .prop( "checked", true ) + .prop( "checked", false ) + .attr( "checked", true ); + assert.equal( + $checkbox.attr( "checked" ), + stockJq4 ? "true" : "checked", + "Set checked (verified by .attr)" + ); + } ); + expectNoWarning( assert, "checked: false", function() { + var $checkbox = jQuery( "" ); + $checkbox + .prop( "checked", false ) + .prop( "checked", true ) + .attr( "checked", false ); + assert.equal( + $checkbox.attr( "checked" ), + undefined, + "Remove checked (verified by .attr)" + ); + } ); + + expectWarning( assert, "readonly: true", ifOn( 1 ), function() { + var $input = jQuery( "" ); + $input + .prop( "readOnly", true ) + .prop( "readOnly", false ) + .attr( "readonly", true ); + assert.equal( + $input.attr( "readonly" ), + stockJq4 ? "true" : "readonly", + "Set readonly (verified by .attr)" + ); + } ); + expectNoWarning( assert, "readonly: false", function() { + var $input = jQuery( "" ); + $input + .prop( "readOnly", false ) + .prop( "readOnly", true ) + .attr( "readonly", false ); + assert.equal( + $input.attr( "readonly" ), + undefined, + "Remove readonly (verified by .attr)" + ); + } ); + + expectWarning( assert, "attribute/property interop", ifOn( 2 ), function() { + var $checkbox = jQuery( "" ); + $checkbox + .attr( "checked", true ) + .attr( "checked", false ) + .prop( "checked", true ); + assert.equal( $checkbox[ 0 ].checked, true, + "Set checked property (verified by native property)" ); + assert.equal( $checkbox.prop( "checked" ), true, + "Set checked property (verified by .prop)" ); + assert.equal( + $checkbox.attr( "checked" ), + undefined, + "Setting checked property doesn't affect checked attribute" + ); + $checkbox + .attr( "checked", false ) + .attr( "checked", true ) + .prop( "checked", false ); + assert.equal( $checkbox[ 0 ].checked, false, + "Clear checked property (verified by native property)" ); + assert.equal( $checkbox.prop( "checked" ), false, + "Clear checked property (verified by .prop)" ); + assert.equal( + $checkbox.attr( "checked" ), + stockJq4 ? "true" : "checked", + "Clearing checked property doesn't affect checked attribute" + ); + } ); + + expectWarning( assert, "HTML5 boolean attributes", ifOn( 2 ), function() { + var $input = jQuery( "" ); + $input.attr( { + "autofocus": true, + "required": true + } ); + assert.equal( + $input.attr( "autofocus" ), + stockJq4 ? "true" : "autofocus", + "Reading autofocus attribute yields 'autofocus'" + ); + assert.equal( + $input.attr( "autofocus", false ).attr( "autofocus" ), + undefined, + "Setting autofocus to false removes it" + ); + assert.equal( + $input.attr( "required" ), + stockJq4 ? "true" : "required", + "Reading required attribute yields 'required'" + ); + assert.equal( + $input.attr( "required", false ).attr( "required" ), + undefined, + "Setting required attribute to false removes it" + ); + } ); + + expectNoWarning( assert, "aria-* attributes", function() { + var $input = jQuery( "" ); + + $input.attr( "aria-disabled", true ); + assert.equal( + $input.attr( "aria-disabled" ), + "true", + "Setting aria attributes to true is not affected by boolean settings" + ); + + $input.attr( "aria-disabled", false ); + assert.equal( + $input.attr( "aria-disabled" ), + "false", + "Setting aria attributes to false is not affected by boolean settings" + ); + } ); + } ); + } + + runTests( { patchEnabled: true } ); + runTests( { patchEnabled: false } ); +} )(); + +QUnit.test( ".attr( data-* attribute )", function( assert ) { + assert.expect( 6 ); + + expectNoWarning( assert, "value: true", function() { + var $input = jQuery( "" ); + $input.attr( "data-something", true ); + assert.equal( $input.attr( "data-something" ), "true", "Set data attributes" ); + assert.equal( $input.data( "something" ), true, + "Setting data attributes are not affected by boolean settings" ); + } ); + + expectWarning( assert, "value: false", 1, function() { + var $input = jQuery( "" ); + $input.attr( "data-another", false ); + assert.equal( $input.attr( "data-another" ), "false", "Set data attributes" ); + assert.equal( $input.data( "another" ), false, + "Setting data attributes are not affected by boolean settings" ); + } ); +} ); + QUnit.test( ".removeAttr( boolean attribute )", function( assert ) { assert.expect( 14 ); expectNoWarning( assert, "non-boolean attr", function() { var $div = jQuery( "
" ) - .attr( "quack", "duck" ) - .removeAttr( "quack" ); + .attr( "quack", "duck" ) + .removeAttr( "quack" ); assert.equal( $div.attr( "quack" ), null, "non-boolean attribute was removed" ); assert.equal( $div.prop( "quack" ), undefined, "property was not set" ); } ); expectWarning( assert, "boolean attr", function() { - var $inp = jQuery( "" ) - .attr( "checked", "checked" ) - .prop( "checked", true ) - .removeAttr( "checked" ); + var $inp = jQuery( "" ) + .attr( "checked", "checked" ) + .prop( "checked", true ) + .removeAttr( "checked" ); assert.equal( $inp.attr( "checked" ), null, "boolean attribute was removed" ); assert.equal( $inp.prop( "checked" ), false, "property was changed" ); @@ -25,7 +238,7 @@ QUnit.test( ".removeAttr( boolean attribute )", function( assert ) { // One warning per attribute name expectWarning( assert, "multiple boolean attr", 2, function() { - jQuery( "" ) + jQuery( "" ) .attr( "checked", "checked" ) .attr( "readonly", "readonly" ) .removeAttr( "checked readonly" ); @@ -41,21 +254,23 @@ QUnit.test( ".removeAttr( boolean attribute )", function( assert ) { } ); expectNoWarning( assert, "boolean attr when prop false", function() { - var $inp = jQuery( "" ) - .attr( "checked", "checked" ) - .prop( "checked", false ) - .removeAttr( "checked" ); + var $inp = jQuery( "" ) + .attr( "checked", "checked" ) + .prop( "checked", false ) + .removeAttr( "checked" ); assert.equal( $inp.attr( "checked" ), null, "boolean attribute was removed" ); assert.equal( $inp.prop( "checked" ), false, "property was not changed" ); } ); expectWarning( assert, "boolean attr when only some props false", 1, function() { - var $inp = jQuery( "" ) - .attr( "checked", "checked" ) - .prop( "checked", false ) - .eq( 1 ).prop( "checked", true ).end() - .removeAttr( "checked" ); + var $inp = jQuery( + "" + ) + .attr( "checked", "checked" ) + .prop( "checked", false ) + .eq( 1 ).prop( "checked", true ).end() + .removeAttr( "checked" ); assert.equal( $inp.attr( "checked" ), null, "boolean attribute was removed" ); assert.equal( $inp.eq( 1 ).prop( "checked" ), false, "property was changed" ); diff --git a/warnings.md b/warnings.md index 57c657c8..166707e6 100644 --- a/warnings.md +++ b/warnings.md @@ -103,6 +103,19 @@ This is _not_ a warning, but a console log message the plugin shows when it firs **Solution**: It is almost always a mistake to use `.removeAttr( "checked" )` on a DOM element. The only time it might be useful is if the DOM is later going to be serialized back to an HTML string. In all other cases, `.prop( "checked", false )` should be used instead. +### \[boolean-attributes\] JQMIGRATE: Boolean attribute 'NAME' value is different from its lowercased name +### \[boolean-attributes\] JQMIGRATE: Boolean attribute 'NAME' value is not set to its lowercased name + +**Cause**: Prior to jQuery 4.0, when calling `.attr( name, value )` with any non-`false` non-`null` `value`, jQuery would actually set it to `name`. Similarly, regardless of the actual value, `.attr( name )` used to return `name` lowercased. jQuery 4.0 removes this special behavior. + +**Solution**: Always set boolean attributes to their names, whether when using jQuery (`.attr( name, name )`), native APIs (`.setAttribute( name, name )`) or directly in HTML (``). + +### \[attr-false\] JQMIGRATE: Setting the non-ARIA non-boolean attribute 'NAME' to false + +**Cause**: Prior to jQuery 4.0, calling `.attr( name, false )` was only removing the attribute when `name` was a boolean attribute; otherwise, it was setting the attribute value to `"false"`. In jQuery 4.x, it will remove any non-ARIA attribute. + +**Solution**: If you want to set the value of an attribute to `"false"`, wrap it in quotes: `.attr( name, "false" )`. + ### \[offset-valid-elem\] JQMIGRATE: jQuery.fn.offset() requires a valid DOM element **Cause:** In earlier versions of jQuery, the `.offset()` method would return a value of `{ top: 0, left: 0 }` for some cases of invalid input. jQuery 3.0 throws errors in some of these cases. The selected element in the jQuery collection must be a DOM element that has a `getBoundingClientRect` method. Text nodes, the `window` object, and plain JavaScript objects are not valid input to the `.offset()` method. jQuery *may* throw an error in those cases but in general does not guarantee specific results with invalid inputs.