- Notifications
You must be signed in to change notification settings - Fork20.6k
CSS: Don't automatically add "px" to properties with a few exceptions#4055
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
a1c0a1e to096afcfCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, just the comment change
src/css.js Outdated
| @@ -247,7 +231,7 @@ jQuery.extend( { | |||
| // If a number was passed in, add the unit (except for certain CSS properties) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Comment should be changed as well, something likeIf the value is a number, addpx for certain CSS properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice catch; changed.
src/css/var/isAutoPx.js Outdated
| // |-+-| Border |-+-+-| Bottom |-+-|-+-| |-+-END | ||
| // | \ Padding / \ Left / | \ Height / | ||
| // | | | ||
| // BEGIN -| /---------\ | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This does recognize some strings that are not currently CSS properties (like PaddingLeftWidth) but it's safe to assume they willnever be CSS properties. Besides it only comes into play if a number is passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm,border is a little special here in that it differs frommargin andpadding. I could try extracting it but we'll end up with more than +1 byte then. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are also others non-existing values liketopWidth. That's not ideal but the important part is the list is finite and not that large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I worry it also matchesmin &max - there are no such things in CSS but I wouldn't be surprised if they were added; it'd be worth to exclude them.
cd5c18a tob15c36bComparemgol commentedApr 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@dmethvin I updated the regexp (again). It now adds |
55af87f to9dcc840CompareNice! |
| // Starting value computation is required for potential unit mismatches | ||
| initialInUnit = ( jQuery.cssNumber[ prop ] || unit !== "px" && +initial ) && | ||
| initialInUnit = elem.nodeType && |
There was a problem hiding this comment.
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 ana property. I've noticed they were already failing ifa was changed to anything fromjQuery.cssNumber and this PR makes almost everything "like fromjQuery.cssNumber".
ThenodeType check should most likely already be there, perhaps we should add it for 3.4.0 as well?
There was a problem hiding this comment.
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,.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Without this change animating properties from jQuery.cssNumber on non-elementsthrows an error.Refjquerygh-4055
src/css/var/isAutoPx.js Outdated
| // 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 /^[a-z]$/.test( prop[ 0 ] ) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Having a function is fine, but we should continue with the convention of creating regexes once outside of functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would be nice to see explicit tests covering "px" appending for these 14 + 6 + 10 properties and non-appending for other properties (both real ones like "line-height" and similarly-named fakes like "MarginLeftWidth" and "BorderTopHeight").
src/css/var/isAutoPx.js Outdated
| // 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 /^[a-z]$/.test( prop[ 0 ] ) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Small savings here by replacing/^[a-z]$/.test( prop[ 0 ] ) with/^[a-z]/.test( prop ).
src/css/var/isAutoPx.js Outdated
| // 1. The prop starts with a lowercase letter (as we uppercase it for the second regex). | ||
| // 2. The prop is not empty. | ||
| return /^[a-z]$/.test( prop[ 0 ] ) && | ||
| /^(?:((?:Margin|Padding)?(?:Top|Right|Bottom|Left)?)|(?:Min|Max)?(?:Width|Height)|Border(?:Top|Right|Bottom|Left)?(?:Width)?)$/.test( prop[ 0 ].toUpperCase() + prop.substr( 1 ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Moderate savings here by replacingprop.substr( 1 ) withprop.slice( 1 ), and small savings by changing the order of alternatives. Suggested gzip-friendly representation:
/^(?:Border(?:Top|Right|Bottom|Left)?(?:Width|)|(?:Margin|Padding)?(?:Top|Right|Bottom|Left)?|(?:Min|Max)?(?:Width|Height))$/.test(prop[0].toUpperCase()+prop.slice(1))
I intend to write the test for all matched props (I'll take the list from#4053) and add a few negative ones to the props from outside of the set. I just haven't gotten to it yet. Thanks for the size reduction tips, I'll try them! |
mgol commentedApr 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
PR updated, tests were added, all remarks by@timmywil &@gibson042 addressed. Thanks to@gibson042's clever remarks we're now down from +6 bytes to -9. 😲 |
Most of these were added but it's hard to test fake ones as the browser throws them away so there's no observable effect. |
@mgol yes, that addressed my concern. |
d3458b9 toad41ce9CompareGood news, suddenly it got down to -45 bytes compared to current |
Uh oh!
There was an error while loading.Please reload this page.
Summary
Don't automatically add "px" to properties with a few exceptions
Fixesgh-2795
Refgh-4009
This is an alternative to PR#4053.
Const over#4053:
it matches more than we theoretically want (the list inWIP CSS: Don't automatically add "px" to properties with a few exceptions #4053 is precise), e.g. non-existent props liketop-widthPros over#4053:
1 byte6 bytes instead of 120Checklist