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

jQuery.css with empty value disables transition from 3.3.0 #4185

Closed
itchyny opened this Issue Oct 2, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@itchyny

itchyny commented Oct 2, 2018

Description

Setting some specific properties to empty value with jQuery.css once and setting a new style disables the animation (CSS transition). See the following examples.

This looks like a breaking change introduced in 3.3.0. I believe this is an unexpected behavior. It animates when I use jQuery 2 (https://jsfiddle.net/L1u7o8zv/).

Link to test case

https://jsfiddle.net/j5ranug1/

itchyny added a commit to itchyny/angular.js that referenced this issue Oct 2, 2018

fix(ngStyle): skip setting empty value when new style has the property
Previously, all the properties in oldStyles are set to empty value once.
Using AngularJS with jQuery 3.3.1, this disables the CSS transition as
reported in jquery/jquery#4185.

itchyny added a commit to itchyny/angular.js that referenced this issue Oct 2, 2018

fix(ngStyle): skip setting empty value when new style has the property
Previously, all the properties in oldStyles are set to empty value once.
Using AngularJS with jQuery 3.3.1, this disables the CSS transition as
reported in jquery/jquery#4185.
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 3, 2018

Member

Thanks for the report.

This is caused by 3fcddd6 (cc @gibson042) and is a result of the width setter now reading the box-sizing style which requires the browser to recompute the styles. If that wasn't done then the browser would simply ignore the first few .css calls that reset the properties since they are set to different values shortly after.

Note that this problem already exists in jQuery 2, although in a more limited way. The box-sizing check was always done, just usually only for outerWidth & similar APIs; see an example with jQuery 2.2.4: https://jsfiddle.net/y34h7aef/3/

This issue has a larger impact when it affects bare .css('width', ...) setter, though. I think we may have to hide the border-box check again in an extra check. We've got another usage of it below (see the mentioned 3fcddd6 commit) but we could first check for scrollBoxSize (currently named that way, was borderBoxReliable in the commit) and only then query box-sizing.

Even if not for this issue, IMO we should do it for performance reasons. As it stands, just calling the .css('width', ...) setter repeatedly will cause layout thrashing.

Member

mgol commented Oct 3, 2018

Thanks for the report.

This is caused by 3fcddd6 (cc @gibson042) and is a result of the width setter now reading the box-sizing style which requires the browser to recompute the styles. If that wasn't done then the browser would simply ignore the first few .css calls that reset the properties since they are set to different values shortly after.

Note that this problem already exists in jQuery 2, although in a more limited way. The box-sizing check was always done, just usually only for outerWidth & similar APIs; see an example with jQuery 2.2.4: https://jsfiddle.net/y34h7aef/3/

This issue has a larger impact when it affects bare .css('width', ...) setter, though. I think we may have to hide the border-box check again in an extra check. We've got another usage of it below (see the mentioned 3fcddd6 commit) but we could first check for scrollBoxSize (currently named that way, was borderBoxReliable in the commit) and only then query box-sizing.

Even if not for this issue, IMO we should do it for performance reasons. As it stands, just calling the .css('width', ...) setter repeatedly will cause layout thrashing.

@mgol mgol self-assigned this Oct 3, 2018

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 3, 2018

Member

Hmm, boxSizing is queried only when needed, though - if either the scrollboxSize support test fails or when we need to adjust for margins etc. so there should be no layout thrashing...

I verified 3fcddd6 is the commit causing this issue, though. Investigating further...

Member

mgol commented Oct 3, 2018

Hmm, boxSizing is queried only when needed, though - if either the scrollboxSize support test fails or when we need to adjust for margins etc. so there should be no layout thrashing...

I verified 3fcddd6 is the commit causing this issue, though. Investigating further...

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 3, 2018

Member

OK, the issue is the comparison to styles.position. So we do have layout thrashing in the end, it seems...

Member

mgol commented Oct 3, 2018

OK, the issue is the comparison to styles.position. So we do have layout thrashing in the end, it seems...

mgol added a commit to mgol/jquery that referenced this issue Oct 3, 2018

CSS: Don't read styles.position in the width/height cssHook unless ne…
…cessary

Current width/height cssHook reads the computed position style even if not
necessary as the browser passes the scrollboxSize support test. That has been
changed.

This commit also makes the scrollboxSize support test in line with all others
(i.e. only return true or false) and changes the variable name in the hook
to make the code clearer.

Fixes jquerygh-4185

@mgol mgol added this to the 3.4.0 milestone Oct 3, 2018

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 3, 2018

Member

I have a PR: #4187.

Setting milestone to 3.4.0.

Member

mgol commented Oct 3, 2018

I have a PR: #4187.

Setting milestone to 3.4.0.

mgol added a commit to angular/angular.js that referenced this issue Oct 4, 2018

fix(ngStyle): skip setting empty value when new style has the property
Previously, all the properties in oldStyles are set to empty value once.
Using AngularJS with jQuery 3.3.1, this disables the CSS transition as
reported in jquery/jquery#4185.

Closes #16709

mgol added a commit to angular/angular.js that referenced this issue Oct 4, 2018

fix(ngStyle): skip setting empty value when new style has the property
Previously, all the properties in oldStyles are set to empty value once.
Using AngularJS with jQuery 3.3.1, this disables the CSS transition as
reported in jquery/jquery#4185.

Closes #16709

@mgol mgol closed this in #4187 Oct 8, 2018

mgol added a commit that referenced this issue Oct 8, 2018

CSS: Don't read styles.position in the width/height cssHook unless ne…
…cessary

Current width/height cssHook reads the computed position style even if not
necessary as the browser passes the scrollboxSize support test. That has been
changed.

This commit also makes the scrollboxSize support test in line with all others
(i.e. only return true or false) and changes the variable name in the hook
to make the code clearer.

Fixes gh-4185
Closes gh-4187

@GulajavaMinistudio GulajavaMinistudio referenced this issue Oct 9, 2018

Merged

Update documentation for #62

0 of 4 tasks complete

@mgol mgol removed the Needs review label Oct 15, 2018

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