Skip to content

CSS: Don't warn against .css( "z-index", numberValue ) in jQuery >=4 #439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 9, 2021
Merged

CSS: Don't warn against .css( "z-index", numberValue ) in jQuery >=4 #439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions src/jquery/css.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,33 @@ if ( jQueryVersionSince( "3.4.0" ) && typeof Proxy !== "undefined" ) {
} );
}

// Create a dummy jQuery.cssNumber if missing. It won't be used by jQuery but
// it will prevent code adding new keys to it unconditionally from crashing.
// In jQuery >=4 where jQuery.cssNumber is missing fill it with the latest 3.x version:
// https://github.com/jquery/jquery/blob/3.6.0/src/css.js#L212-L233
// This way, number values for the CSS properties below won't start triggering
// Migrate warnings when jQuery gets updated to >=4.0.0 (gh-438).
if ( !jQuery.cssNumber ) {
jQuery.cssNumber = {};
jQuery.cssNumber = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Man we can never win. 😸

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not 100% necessary as Core doesn't need this object in 4.x; we could as well just enumerate allowlisted properties in Migrate. However, by doing it via jQuery.cssNumber we have the same Migrate code path for all jQuery versions and it's easier to copy the object from the 3.x-stable Core branch if there are any changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it seems easier to just copy over the list. It's an exposed object so who knows, maybe some software out there is looking at it and not just setting properties.

animationIterationCount: true,
columnCount: true,
fillOpacity: true,
flexGrow: true,
flexShrink: true,
fontWeight: true,
gridArea: true,
gridColumn: true,
gridColumnEnd: true,
gridColumnStart: true,
gridRow: true,
gridRowEnd: true,
gridRowStart: true,
lineHeight: true,
opacity: true,
order: true,
orphans: true,
widows: true,
zIndex: true,
zoom: true
};
}

function isAutoPx( prop ) {
Expand Down
16 changes: 12 additions & 4 deletions test/css.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ QUnit.test( "jQuery.css with arrays", function( assert ) {

QUnit.test( "jQuery.css with numbers", function( assert ) {
var jQuery3OrOlder = compareVersions( jQuery.fn.jquery, "4.0.0" ) < 0,
whitelist = [
allowlist = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

"margin",
"marginTop",
"marginRight",
Expand Down Expand Up @@ -95,7 +95,7 @@ QUnit.test( "jQuery.css with numbers", function( assert ) {
"borderLeftWidth"
];

assert.expect( jQuery3OrOlder ? 7 : 6 );
assert.expect( jQuery3OrOlder ? 8 : 7 );

function kebabCase( string ) {
return string.replace( /[A-Z]/g, function( match ) {
Expand Down Expand Up @@ -127,8 +127,8 @@ QUnit.test( "jQuery.css with numbers", function( assert ) {
} );
} );

expectNoWarning( assert, "Number value (whitelisted props)", function() {
whitelist.forEach( function( prop ) {
expectNoWarning( assert, "Number value (allowlisted props)", function() {
allowlist.forEach( function( prop ) {
jQuery( "<div />" ).css( prop, 1 );
jQuery( "<div />" ).css( kebabCase( prop ), 1 );
} );
Expand All @@ -147,6 +147,14 @@ QUnit.test( "jQuery.css with numbers", function( assert ) {
}
} );

// z-index is tested explicitly as raw jQuery 4.0 will not have `jQuery.cssNumber`
// so iterating over it won't find anything and we'd like to ensure number values
// are not warned against for safe CSS props like z-index (gh-438).
expectNoWarning( assert, "z-index", function() {
jQuery( "<div />" ).css( "z-index", 1 );
jQuery( "<div />" ).css( kebabCase( "zIndex" ), 1 );
} );

} );

QUnit.test( "jQuery.cssNumber", function( assert ) {
Expand Down