Skip to content
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.

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

CSS: Don't automatically add "px" to properties with a few exceptions #4055

Merged
merged 1 commit into from
Apr 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 5 additions & 30 deletions src/css.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ define( [
"./var/rcssNum",
"./css/var/rnumnonpx",
"./css/var/cssExpand",
"./css/isAutoPx",
"./css/var/getStyles",
"./css/var/swap",
"./css/curCSS",
Expand All @@ -16,7 +17,7 @@ define( [
"./core/init",
"./core/ready",
"./selector" // contains
], function( jQuery, access, camelCase, rcssNum, rnumnonpx, cssExpand,
], function( jQuery, access, camelCase, rcssNum, rnumnonpx, cssExpand, isAutoPx,
getStyles, swap, curCSS, adjustCSS, addGetHookIf, support, finalPropName ) {

"use strict";
Expand Down Expand Up @@ -198,30 +199,6 @@ jQuery.extend( {
}
},

// Don't automatically add "px" to these possibly-unitless properties
cssNumber: {
"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
},

// Add in properties whose names you wish to fix before
// setting or getting the value
cssProps: {},
Expand Down Expand Up @@ -267,11 +244,9 @@ jQuery.extend( {
return;
}

// If a number was passed in, add the unit (except for certain CSS properties)
// The isCustomProp check can be removed in jQuery 4.0 when we only auto-append
// "px" to a few hardcoded values.
if ( type === "number" && !isCustomProp ) {
value += ret && ret[ 3 ] || ( jQuery.cssNumber[ origName ] ? "" : "px" );
// If the value is a number, add `px` for certain CSS properties
if ( type === "number" ) {
value += ret && ret[ 3 ] || ( isAutoPx( origName ) ? "px" : "" );
}

// background-* props affect original clone's values
Expand Down
7 changes: 4 additions & 3 deletions src/css/adjustCSS.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
define( [
"../core",
"./isAutoPx",
"../var/rcssNum"
], function( jQuery, rcssNum ) {
], function( jQuery, isAutoPx, rcssNum ) {

"use strict";

Expand All @@ -16,11 +17,11 @@ function adjustCSS( elem, prop, valueParts, tween ) {
return jQuery.css( elem, prop, "" );
},
initial = currentValue(),
unit = valueParts && valueParts[ 3 ] || ( jQuery.cssNumber[ prop ] ? "" : "px" ),
unit = valueParts && valueParts[ 3 ] || ( isAutoPx( prop ) ? "px" : "" ),

// Starting value computation is required for potential unit mismatches
initialInUnit = elem.nodeType &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this change, effects tests were failing on non-element objects on an a property. I've noticed they were already failing if a was changed to anything from jQuery.cssNumber and this PR makes almost everything "like from jQuery.cssNumber".

The nodeType check should most likely already be there, perhaps we should add it for 3.4.0 as well?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, ugh. Put another way, we only want to add "px" to property values if they're on DOM elements. I still wish we hadn't supported animating plain objects,.

Copy link
Member Author

Choose a reason for hiding this comment

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

I extracted this change to PR #4061.

Copy link
Member Author

Choose a reason for hiding this comment

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

#4601 landed, this PR rebased so this particular change is no longer a part of this PR.

( jQuery.cssNumber[ prop ] || unit !== "px" && +initial ) &&
( !isAutoPx( prop ) || unit !== "px" && +initial ) &&
rcssNum.exec( jQuery.css( elem, prop ) );

if ( initialInUnit && initialInUnit[ 3 ] !== unit ) {
Expand Down
41 changes: 41 additions & 0 deletions src/css/isAutoPx.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
define( function() {

"use strict";

var ralphaStart = /^[a-z]/,

// The regex visualized:
//
// /----------\
// | | /-------\
// | / Top \ | | |
// /--- Border ---+-| Right |-+---+- Width -+---\
// | | Bottom | |
// | \ Left / |
// | |
// | /----------\ |
// | /-------------\ | | |- END
// | | | | / Top \ | |
// | | / Margin \ | | | Right | | |
// |---------+-| |-+---+-| Bottom |-+----|
// | \ Padding / \ Left / |
// BEGIN -| |
// | /---------\ |
// | | | |
// | | / Min \ | / Width \ |
// \--------------+-| |-+---| |---/
// \ Max / \ Height /
rautoPx = /^(?:Border(?:Top|Right|Bottom|Left)?(?:Width|)|(?:Margin|Padding)?(?:Top|Right|Bottom|Left)?|(?:Min|Max)?(?:Width|Height))$/;

function isAutoPx( prop ) {

// The first test is used to ensure that:
// 1. The prop starts with a lowercase letter (as we uppercase it for the second regex).
// 2. The prop is not empty.
return ralphaStart.test( prop ) &&
rautoPx.test( prop[ 0 ].toUpperCase() + prop.slice( 1 ) );
};

return isAutoPx;

} );
5 changes: 3 additions & 2 deletions src/effects/Tween.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
define( [
"../core",
"../css/isAutoPx",
"../css/finalPropName",

"../css"
], function( jQuery, finalPropName ) {
], function( jQuery, isAutoPx, finalPropName ) {

"use strict";

Expand All @@ -21,7 +22,7 @@ Tween.prototype = {
this.options = options;
this.start = this.now = this.cur();
this.end = end;
this.unit = unit || ( jQuery.cssNumber[ prop ] ? "" : "px" );
this.unit = unit || ( isAutoPx( prop ) ? "px" : "" );
},
cur: function() {
var hooks = Tween.propHooks[ this.prop ];
Expand Down
82 changes: 80 additions & 2 deletions test/unit/css.js
Original file line number Diff line number Diff line change
Expand Up @@ -1198,14 +1198,17 @@ if ( jQuery.fn.offset ) {
}

QUnit.test( "Do not append px (#9548, #12990, #2792)", function( assert ) {
assert.expect( 3 );
assert.expect( 4 );

var $div = jQuery( "<div>" ).appendTo( "#qunit-fixture" );

$div.css( "fill-opacity", 1 );

assert.equal( $div.css( "fill-opacity" ), 1, "Do not append px to 'fill-opacity'" );

$div.css( "font-size", "27px" );
$div.css( "line-height", 2 );
assert.equal( $div.css( "line-height" ), "54px", "Do not append px to 'line-height'" );

$div.css( "column-count", 1 );
if ( $div.css( "column-count" ) !== undefined ) {
assert.equal( $div.css( "column-count" ), 1, "Do not append px to 'column-count'" );
Expand Down Expand Up @@ -1273,6 +1276,81 @@ QUnit[
}
} );

QUnit.test( "Do not append px to most properties not accepting integer values", function( assert ) {
assert.expect( 3 );

var $div = jQuery( "<div>" ).appendTo( "#qunit-fixture" );

$div.css( "font-size", "27px" );

$div.css( "font-size", 2 );
assert.equal( $div.css( "font-size" ), "27px", "Do not append px to 'font-size'" );

$div.css( "fontSize", 2 );
assert.equal( $div.css( "fontSize" ), "27px", "Do not append px to 'fontSize'" );

$div.css( "letter-spacing", "2px" );
$div.css( "letter-spacing", 3 );
assert.equal( $div.css( "letter-spacing" ), "2px", "Do not append px to 'letter-spacing'" );
} );

QUnit.test( "Append px to whitelisted properties", function( assert ) {
var prop,
$div = jQuery( "<div>" ).appendTo( "#qunit-fixture" ),
whitelist = {
margin: "marginTop",
marginTop: undefined,
marginRight: undefined,
marginBottom: undefined,
marginLeft: undefined,
padding: "paddingTop",
paddingTop: undefined,
paddingRight: undefined,
paddingBottom: undefined,
paddingLeft: undefined,
top: undefined,
right: undefined,
bottom: undefined,
left: undefined,
width: undefined,
height: undefined,
minWidth: undefined,
minHeight: undefined,
maxWidth: undefined,
maxHeight: undefined,
border: "borderTopWidth",
borderWidth: "borderTopWidth",
borderTop: "borderTopWidth",
borderTopWidth: undefined,
borderRight: "borderRightWidth",
borderRightWidth: undefined,
borderBottom: "borderBottomWidth",
borderBottomWidth: undefined,
borderLeft: "borderLeftWidth",
borderLeftWidth: undefined
};

assert.expect( ( Object.keys( whitelist ).length ) * 2 );

for ( prop in whitelist ) {
var propToCheck = whitelist[ prop ] || prop,
kebabProp = prop.replace( /[A-Z]/g, function( match ) {
return "-" + match.toLowerCase();
} ),
kebabPropToCheck = propToCheck.replace( /[A-Z]/g, function( match ) {
return "-" + match.toLowerCase();
} );
$div.css( prop, 3 )
.css( "position", "absolute" )
.css( "border-style", "solid" );
assert.equal( $div.css( propToCheck ), "3px", "Append px to '" + prop + "'" );
$div.css( kebabProp, 3 )
.css( "position", "absolute" )
.css( "border-style", "solid" );
assert.equal( $div.css( kebabPropToCheck ), "3px", "Append px to '" + kebabProp + "'" );
}
} );

QUnit.test( "css('width') and css('height') should respect box-sizing, see #11004", function( assert ) {
assert.expect( 4 );

Expand Down