fix(ngStyle): skip setting empty value when new style has the property #16709

itchyny · 2018-10-02T10:30:22Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.

What is the current behavior? (You can also link to an open issue here)
Using ng-style with CSS transition with jQuery 3.3.1 (currently the latest tag), the animation is disabled in certain circumstances.
https://jsfiddle.net/qz5a46yc/1/

Without jQuery (using jqLite.css) or using jQuery 3.2.1, the div animates. I reported to the jQuery dev on this behavior difference: jquery/jquery#4185. I think this is a bug in jQuery.

This pull request suggests a workaround on the disabled animation issue. I'm not sure how the report to the jQuery will be handled, but we can fix the behavior with reasonable changes in AngularJS. Skipping the common properties with newStyles will fix the animation issue with jQuery 3.3.1, which can be confirmed by https://jsfiddle.net/g3st70fh/1/. I think the diff is reasonable and also improves the performance because the properties in oldStyles and newStyles are same in most cases.

What is the new behavior (if this is a feature change)?
I believe there's no changes except for fixing the animation issue.

Does this PR introduce a breaking change?
No, I believe.

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

googlebot · 2018-10-02T10:30:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-10-02T10:30:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

itchyny · 2018-10-02T10:37:06Z

I signed it!

googlebot · 2018-10-02T10:37:08Z

CLAs look good, thanks!

googlebot · 2018-10-02T10:37:09Z

CLAs look good, thanks!

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.

gkalpak · 2018-10-02T11:06:45Z

src/ng/directive/ngStyle.js

-      forEach(oldStyles, function(val, style) { element.css(style, '');});
+      forEach(oldStyles, function(val, style) {
+          if (!(newStyles && newStyles.hasOwnProperty(style))) {
+              element.css(style, '');


Incorrect indentation.

gkalpak · 2018-10-02T11:09:17Z

src/ng/directive/ngStyle.js

@@ -54,7 +54,11 @@
 var ngStyleDirective = ngDirective(function(scope, element, attr) {
  scope.$watchCollection(attr.ngStyle, function ngStyleWatchAction(newStyles, oldStyles) {
    if (oldStyles && (newStyles !== oldStyles)) {
-      forEach(oldStyles, function(val, style) { element.css(style, '');});
+      forEach(oldStyles, function(val, style) {
+          if (!(newStyles && newStyles.hasOwnProperty(style))) {


What if newStyles does have the property, but the value is undefined or null?

Right, in that case the style should be cleared. Fixed in 5d74f0f.

gkalpak · 2018-10-02T11:10:30Z

Looks reasonable. I'd still wait for a response from the jQuery team on whether this change was intentional or not.

… falsy

jbedard · 2018-10-03T18:56:53Z

src/ng/directive/ngStyle.js

-      forEach(oldStyles, function(val, style) { element.css(style, '');});
+      forEach(oldStyles, function(val, style) {
+        if (!(newStyles && newStyles[style])) {
+          element.css(style, '');


Would it be possible to do newStyles[style] = '' instead (maybe also needing a newStyles null check + assign). This way we will only make a single element.css(newStyles) call at the end instead of multiple .css calls.

Also should this be an undefined/null check instead of just falsey check? What if newStyles[style] is 0?

For sure. Fixed in 95d2458.

gkalpak

LGTM 👍
It would be nice to have a tests that verifies that falsy style values (e.g. 0) are still updated correctly.

…eric 0

src/ng/directive/ngStyle.js

mgol · 2018-10-04T14:47:10Z

Landed, thanks!

itchyny · 2018-10-04T16:29:14Z

Thank you!

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

googlebot added the cla: no label Oct 2, 2018

googlebot added cla: yes and removed cla: no labels Oct 2, 2018

gkalpak reviewed Oct 2, 2018

View reviewed changes

gkalpak added type: bug component: misc core labels Oct 2, 2018

gkalpak added this to the 1.7.x milestone Oct 2, 2018

itchyny added 3 commits October 2, 2018 20:38

style(ngStyle): fix indentation

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

02f0d0b

fix(ngStyle): clear the style even when newStyle has the property but…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

5d74f0f

… falsy

test(ngStyle): add a test for falsy values

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

704ce65

jbedard reviewed Oct 3, 2018

View reviewed changes

perf(ngStyle): use newStyles to reduce the number of element.css

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

95d2458

gkalpak suggested changes Oct 4, 2018

View reviewed changes

gkalpak added the needs: test label Oct 4, 2018

test(ngStyle): add a test for setting style when the new value is num…

cbb0b7d

…eric 0

gkalpak approved these changes Oct 4, 2018

View reviewed changes

mgol reviewed Oct 4, 2018

View reviewed changes

src/ng/directive/ngStyle.js Show resolved Hide resolved

test(ngStyle): add a test for the case the new model is null

c44b2f2

mgol approved these changes Oct 4, 2018

View reviewed changes

jbedard approved these changes Oct 4, 2018

View reviewed changes

mgol merged commit bb5a7e3 into angular:master Oct 4, 2018

itchyny deleted the improve-ng-style-skip-old-styles branch October 4, 2018 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngStyle): skip setting empty value when new style has the property #16709

fix(ngStyle): skip setting empty value when new style has the property #16709

itchyny commented Oct 2, 2018 •

edited

Loading

googlebot commented Oct 2, 2018

googlebot commented Oct 2, 2018

itchyny commented Oct 2, 2018

googlebot commented Oct 2, 2018

googlebot commented Oct 2, 2018

gkalpak Oct 2, 2018

gkalpak Oct 2, 2018

itchyny Oct 2, 2018

gkalpak commented Oct 2, 2018

jbedard Oct 3, 2018 •

edited

Loading

itchyny Oct 4, 2018

gkalpak left a comment

mgol commented Oct 4, 2018

itchyny commented Oct 4, 2018

fix(ngStyle): skip setting empty value when new style has the property #16709

fix(ngStyle): skip setting empty value when new style has the property #16709

Conversation

itchyny commented Oct 2, 2018 • edited Loading

googlebot commented Oct 2, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Oct 2, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

itchyny commented Oct 2, 2018

googlebot commented Oct 2, 2018

googlebot commented Oct 2, 2018

gkalpak Oct 2, 2018

Choose a reason for hiding this comment

gkalpak Oct 2, 2018

Choose a reason for hiding this comment

itchyny Oct 2, 2018

Choose a reason for hiding this comment

gkalpak commented Oct 2, 2018

jbedard Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

itchyny Oct 4, 2018

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

mgol commented Oct 4, 2018

itchyny commented Oct 4, 2018

itchyny commented Oct 2, 2018 •

edited

Loading

jbedard Oct 3, 2018 •

edited

Loading