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(), the values like +=10% aren't ignored anymore (like .animate()) #2011

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/css.js
@@ -1,13 +1,15 @@
define([
"./core",
"./var/pnum",
"./var/rfxnum",
"./core/access",
"./css/var/rmargin",
"./css/var/rnumnonpx",
"./css/var/cssExpand",
"./css/var/isHidden",
"./css/var/getStyles",
"./css/curCSS",
"./css/adjustCSS",
"./css/defaultDisplay",
"./css/addGetHookIf",
"./css/support",
Expand All @@ -17,16 +19,15 @@ define([
"./css/swap",
"./core/ready",
"./selector" // contains
], function( jQuery, pnum, access, rmargin, rnumnonpx, cssExpand, isHidden,
getStyles, curCSS, defaultDisplay, addGetHookIf, support, dataPriv ) {
], function( jQuery, pnum, rfxnum, access, rmargin, rnumnonpx, cssExpand, isHidden,
getStyles, curCSS, adjustCSS, defaultDisplay, addGetHookIf, support, dataPriv ) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the "r" variables together (e.g., rfxnum before or after rnumnonpx).


var
// Swappable if display is none or starts with table
// except "table", "table-cell", or "table-caption"
// See here for display values: https://developer.mozilla.org/en-US/docs/CSS/display
rdisplayswap = /^(none|table(?!-c[ea]).+)/,
rnumsplit = new RegExp( "^(" + pnum + ")(.*)$", "i" ),
rrelNum = new RegExp( "^([+-])=(" + pnum + ")", "i" ),

cssShow = { position: "absolute", visibility: "hidden", display: "block" },
cssNormalTransform = {
Expand Down Expand Up @@ -272,8 +273,8 @@ jQuery.extend({
type = typeof value;

// Convert "+=" or "-=" to relative numbers (#7345)
if ( type === "string" && (ret = rrelNum.exec( value )) ) {
value = ( ret[1] + 1 ) * ret[2] + parseFloat( jQuery.css( elem, name ) );
if ( type === "string" && (ret = rfxnum.exec( value )) && ret[ 1 ] ) {
value = adjustCSS( elem, name, ret );
// Fixes bug #9237
type = "number";
}
Expand All @@ -283,9 +284,13 @@ jQuery.extend({
return;
}

// If a number was passed in, add 'px' (except for certain CSS properties)
if ( type === "number" && !jQuery.cssNumber[ origName ] ) {
value += "px";
// If a number was passed in, add the unit (except for certain CSS properties)
if ( type === "number" ) {
if ( ret && ret[ 3 ] ) {
value += ret[ 3 ];
} else if ( !jQuery.cssNumber[ origName ] ) {
value += "px";
}
Copy link
Member

Choose a reason for hiding this comment

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

This would probably be smaller as value += ret && ret[ 3 ] || ( jQuery.cssNumber[ origName ] ? "" : "px" ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I didn't know what is the best between less charactere or a concat with an empty string "".

}

// Support: IE9-11+
Expand Down
60 changes: 60 additions & 0 deletions src/css/adjustCSS.js
@@ -0,0 +1,60 @@
define([
"../core",
"../var/rfxnum"
], function( jQuery, rfxnum ) {

function adjustCSS( elem, prop, value, tween ) {
var adjusted,
scale = 1,
maxIterations = 20,
currentValue = tween ?
function() { return tween.cur(); } :
function() { return jQuery.css( elem, prop, "" ); },
initial = currentValue(),
unit = value && value[ 3 ] || ( jQuery.cssNumber[ prop ] ? "" : "px" ),
// Starting value computation is required for potential unit mismatches
recasted = ( jQuery.cssNumber[ prop ] || unit !== "px" && +initial ) &&
rfxnum.exec( jQuery.css( elem, prop ) );
Copy link
Member

Choose a reason for hiding this comment

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

Seeing it all together, maybe a couple more renames are in order:

  • valuevalueParts
  • recastedinitialInUnit


if ( recasted && recasted[ 3 ] !== unit ) {
// Trust units reported by jQuery.css
unit = unit || recasted[ 3 ];

// Make sure we update the tween properties later on
value = value || [];

// Iteratively approximate from a nonzero starting point
recasted = +initial || 1;

do {
// If previous iteration zeroed out, double until we get *something*.
// Use string for doubling so we don't accidentally see scale as unchanged below
scale = scale || ".5";

// Adjust and apply
recasted = recasted / scale;
jQuery.style( elem, prop, recasted + unit );

// Update scale, tolerating zero or NaN from tween.cur()
// Break the loop if scale is unchanged or perfect, or if we've just had enough.
} while (
scale !== (scale = currentValue() / initial) && scale !== 1 && --maxIterations
);
}

if ( value ) {
recasted = +recasted || +initial || 0;
adjusted = value[ 1 ] ?
Copy link
Member

Choose a reason for hiding this comment

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

This needs a comment like Apply relative offset (+=/-=) if specified.

recasted + ( value[ 1 ] + 1 ) * value[ 2 ] :
+value[ 2 ];
if ( tween ) {
tween.unit = unit;
tween.start = recasted;
tween.end = adjusted;
}
}
return adjusted;
}

return adjustCSS;
});
56 changes: 6 additions & 50 deletions src/effects.js
@@ -1,9 +1,10 @@
define([
"./core",
"./var/document",
"./var/pnum",
"./var/rfxnum",
"./css/var/cssExpand",
"./css/var/isHidden",
"./css/adjustCSS",
"./css/defaultDisplay",
"./data/var/dataPriv",

Expand All @@ -13,63 +14,18 @@ define([
"./css",
"./deferred",
"./traversing"
], function( jQuery, document, pnum, cssExpand, isHidden, defaultDisplay, dataPriv ) {
], function( jQuery, document, rfxnum, cssExpand,
isHidden, adjustCSS, defaultDisplay, dataPriv ) {

var
fxNow, timerId,
rfxtypes = /^(?:toggle|show|hide)$/,
rfxnum = new RegExp( "^(?:([+-])=|)(" + pnum + ")([a-z%]*)$", "i" ),
rrun = /queueHooks$/,
animationPrefilters = [ defaultPrefilter ],
tweeners = {
"*": [ function( prop, value ) {
var tween = this.createTween( prop, value ),
target = tween.cur(),
parts = rfxnum.exec( value ),
unit = parts && parts[ 3 ] || ( jQuery.cssNumber[ prop ] ? "" : "px" ),

// Starting value computation is required for potential unit mismatches
start = ( jQuery.cssNumber[ prop ] || unit !== "px" && +target ) &&
rfxnum.exec( jQuery.css( tween.elem, prop ) ),
scale = 1,
maxIterations = 20;

if ( start && start[ 3 ] !== unit ) {
// Trust units reported by jQuery.css
unit = unit || start[ 3 ];

// Make sure we update the tween properties later on
parts = parts || [];

// Iteratively approximate from a nonzero starting point
start = +target || 1;

do {
// If previous iteration zeroed out, double until we get *something*.
// Use string for doubling so we don't accidentally see scale as unchanged below
scale = scale || ".5";

// Adjust and apply
start = start / scale;
jQuery.style( tween.elem, prop, start + unit );

// Update scale, tolerating zero or NaN from tween.cur(),
// break the loop if scale is unchanged or perfect, or if we've just had enough
} while (
scale !== (scale = tween.cur() / target) && scale !== 1 && --maxIterations
);
}

// Update tween properties
if ( parts ) {
start = tween.start = +start || +target || 0;
tween.unit = unit;
// If a +=/-= token was provided, we're doing a relative animation
tween.end = parts[ 1 ] ?
start + ( parts[ 1 ] + 1 ) * parts[ 2 ] :
+parts[ 2 ];
}

var tween = this.createTween( prop, value );
adjustCSS( tween.elem, prop, rfxnum.exec( value ), tween );
return tween;
} ]
};
Expand Down
7 changes: 7 additions & 0 deletions src/var/rfxnum.js
@@ -0,0 +1,7 @@
define([
"../var/pnum"
], function( pnum ) {

return new RegExp( "^(?:([+-])=|)(" + pnum + ")([a-z%]*)$", "i" );
Copy link
Member

Choose a reason for hiding this comment

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

One more rename, now that it holds: rfxnumrcssNum.


});
53 changes: 53 additions & 0 deletions test/unit/css.js
Expand Up @@ -203,6 +203,59 @@ test( "css() explicit and relative values", 29, function() {
equal( $elem.css("opacity"), "1", "'+=0.5' on opacity (params)" );
});

test( "css() non-px relative values (gh-1711)", 17, function() {
var cssCurrent,
units = {},
$parent = jQuery("#nothiddendiv"),
$child = jQuery("#nothiddendivchild", $parent),
Copy link
Member

Choose a reason for hiding this comment

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

Drop this $parent, id is enough to get the right element.

add = function( prop, val, unit ) {
var str = ( val < 0 ? "-=" : "+=" ) + Math.abs(val) + unit;
Copy link
Member

Choose a reason for hiding this comment

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

The jQuery and Math.abs function calls need spaces around their arguments.

$child.css( prop, str );
equal(
Math.round( parseFloat( $child.css( prop ) ) ),
Math.round( cssCurrent += val * units[ prop ][ unit ] ),
"'" + str + "' on " + prop
Copy link
Member

Choose a reason for hiding this comment

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

Let's use prop + ": '" + str + "'" (which reads as a valid key-value pair in a $el.css({ … }) call).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I prefer that way, but I did that because the previous unit test "css() explicit and relative values" write the debug like: "'-=9px' on width (hash)".
Do you want change this part too? It's fast and easy to do that with multi cursor ^^

);
},
getUnits = function( prop ) {
units[ prop ] = {
"px": 1,
"em": parseFloat( $child.css( "fontSize", "16px" ).css( "fontSize" )),
Copy link
Member

Choose a reason for hiding this comment

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

"em" appears to be buggy with respect to unnecessarily different from the following lines; why not parseFloat( $child.css( prop, "100em" ).css( prop ) ) / 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's more logical that way, I used the font-size to set the unit em and not only to find it.
But finally it's the same :D

"pt": parseFloat( $child.css( prop, "100pt" ).css( prop ) ) / 100,
"pc": parseFloat( $child.css( prop, "100pc" ).css( prop ) ) / 100,
"cm": parseFloat( $child.css( prop, "100cm" ).css( prop ) ) / 100,
"mm": parseFloat( $child.css( prop, "100mm" ).css( prop ) ) / 100,
"%" : parseFloat( $child.css( prop, "100%" ).css( prop ) ) / 100
};
};

$parent.css({ height: 1, padding: 0, width: 400 });
$child.css({ height: 1, padding: 0 });

getUnits( "width" );
cssCurrent = $child.css( "width", "50%" ).width();
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to pull in code from other modules here; this should use .css( "width" ) in analogy to the "lineHeight" block below.

add( "width", 25, "%" );
add( "width", -50, "%" );
add( "width", 10, "em" );
add( "width", 10, "pt" );
add( "width", -2.3, "pt" );
add( "width", 5, "pc" );
add( "width", -5, "em" );
add( "width", +2, "cm" );
add( "width", -15, "mm" );
add( "width", 21, "px" );

getUnits( "lineHeight" );
cssCurrent = parseFloat( $child.css( "lineHeight", "1em" ).css( "lineHeight" ) );
add( "lineHeight", 2, "em" );
add( "lineHeight", -10, "px" );
add( "lineHeight", 20, "pt" );
add( "lineHeight", 30, "pc" );
add( "lineHeight", 1, "cm" );
add( "lineHeight", -20, "mm" );
add( "lineHeight", 50, "%" );
});

Copy link
Member

Choose a reason for hiding this comment

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

Could you also add some tests against line-height instead of width? That property has caused problems in the past, and I'd like to know it's solid.

test("css(String, Object)", function() {
expect( 19 );
var j, div, display, ret, success;
Expand Down