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

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@mgol
Member

mgol commented Apr 24, 2018

Summary

Don't automatically add "px" to properties with a few exceptions

Fixes gh-2795
Ref gh-4009

This is an alternative to PR #4053.

Const over #4053:

  • it matches more than we theoretically want (the list in #4053 is precise), e.g. non-existent props like top-width
  • it's a little slower than #4053 as it uses a regex instead of a simple property checking.

Pros over #4053:

  • it only adds 1 byte 6 bytes instead of 120

Checklist

@mgol mgol added this to the 4.0.0 milestone Apr 24, 2018

@mgol mgol requested a review from dmethvin Apr 24, 2018

@mgol mgol added the Needs review label Apr 24, 2018

@dmethvin

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)

This comment has been minimized.

@dmethvin

dmethvin Apr 24, 2018

Member

Comment should be changed as well, something like If the value is a number, add px for certain CSS properties.

This comment has been minimized.

@mgol

mgol Apr 24, 2018

Member

Nice catch; changed.

// |-+-| Border |-+-+-| Bottom |-+-|-+-| |-+-END
// | \ Padding / \ Left / | \ Height /
// | |
// BEGIN -| /---------\ |

This comment has been minimized.

@dmethvin

dmethvin Apr 24, 2018

Member

This does recognize some strings that are not currently CSS properties (like PaddingLeftWidth) but it's safe to assume they will never be CSS properties. Besides it only comes into play if a number is passed in.

This comment has been minimized.

@mgol

mgol Apr 24, 2018

Member

Hmm, border is a little special here in that it differs from margin and padding. I could try extracting it but we'll end up with more than +1 byte then. ;)

This comment has been minimized.

@mgol

mgol Apr 24, 2018

Member

There are also others non-existing values like topWidth. That's not ideal but the important part is the list is finite and not that large.

This comment has been minimized.

@mgol

mgol Apr 24, 2018

Member

I worry it also matches min & 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.

@mgol mgol changed the title from CSS: Don't automatically add "px" to properties with a few exceptions to WIP CSS: Don't automatically add "px" to properties with a few exceptions Apr 24, 2018

@mgol

This comment has been minimized.

Member

mgol commented Apr 24, 2018

@dmethvin I updated the regexp (again). It now adds 7 6 bytes instead of 1 but I think it catches exactly what we want and nothing else. :) I updated the graph for the regex as well.

@dmethvin

This comment has been minimized.

Member

dmethvin commented Apr 25, 2018

Nice!

// Starting value computation is required for potential unit mismatches
initialInUnit = ( jQuery.cssNumber[ prop ] || unit !== "px" && +initial ) &&
initialInUnit = elem.nodeType &&

This comment has been minimized.

@mgol

mgol Apr 25, 2018

Member

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?

This comment has been minimized.

@dmethvin

dmethvin Apr 25, 2018

Member

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,.

This comment has been minimized.

@mgol

mgol Apr 26, 2018

Member

I extracted this change to PR #4061.

This comment has been minimized.

@mgol

mgol Apr 30, 2018

Member

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

mgol added a commit to mgol/jquery that referenced this pull request Apr 26, 2018

mgol added a commit to mgol/jquery that referenced this pull request Apr 26, 2018

CSS: Skip the px-appending logic for animations of non-element props
Without this change animating properties from jQuery.cssNumber on non-elements
throws an error.

Ref jquerygh-4055
// 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 ] ) &&

This comment has been minimized.

@timmywil

timmywil Apr 30, 2018

Member

Having a function is fine, but we should continue with the convention of creating regexes once outside of functions.

mgol added a commit that referenced this pull request Apr 30, 2018

CSS: Skip the px-appending logic for animations of non-element props
Without this change animating properties from jQuery.cssNumber on non-elements
throws an error.

Ref gh-4055
Closes gh-4061
@gibson042

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").

// 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 ] ) &&

This comment has been minimized.

@gibson042

gibson042 Apr 30, 2018

Member

Small savings here by replacing /^[a-z]$/.test( prop[ 0 ] ) with /^[a-z]/.test( prop ).

// 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 ) );

This comment has been minimized.

@gibson042

gibson042 Apr 30, 2018

Member

Moderate savings here by replacing prop.substr( 1 ) with prop.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 ) )
@mgol

This comment has been minimized.

Member

mgol commented Apr 30, 2018

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").

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!

@GulajavaMinistudio GulajavaMinistudio referenced this pull request Apr 30, 2018

Merged

Update upstream #47

0 of 4 tasks complete
@mgol

This comment has been minimized.

Member

mgol commented Apr 30, 2018

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. 😲

@mgol

This comment has been minimized.

Member

mgol commented Apr 30, 2018

@gibson042

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").

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 mgol changed the title from WIP CSS: Don't automatically add "px" to properties with a few exceptions to CSS: Don't automatically add "px" to properties with a few exceptions May 1, 2018

@mgol mgol referenced this pull request May 2, 2018

Merged

CSS: Don't auto-append "px" to CSS variables #4064

3 of 3 tasks complete
@mgol

This comment has been minimized.

Member

mgol commented Jun 4, 2018

PR rebased with a revert of the fix from PR #4064 as it's no longer needed when we switch to the whitelist.

-7 bytes as of now (compared to master @ 75b77b4).

@timmywil are your concerns resolved? Your re-review is needed.

@timmywil

This comment has been minimized.

Member

timmywil commented Jun 4, 2018

@mgol yes, that addressed my concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment