Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

ngStyle not removing values #16860

Closed
1 of 4 tasks

Comments

@lmartorella
Copy link

I'm submitting a ...

  • regression from 1.6.x to 1.7.8
  • security issue
  • issue caused by a new browser version
  • other

Current behavior:
In AngularJS 1.6 (and even in 1.7.0) the ngStyle directive was able to add/remove single properties when the expression for the single property was evaluated as false.
For example:
ng-style="{'float': isRight && 'right' }"
was dynamically adding/removing the 'float: right' style when the isRight scope expression changed.
In Angular 1.7.8 this only happens when isRight switches from false to true. Switching back to false will leave the 'float: right' style set.

As a workaround, this expression instead works in both 1.6.x and 1.7.8:
ng-style="isRight && {'float': 'right' }"

Expected / new behavior:
Expressions like
ng-style="{'float': isRight && 'right' }"
should be still supported for backward compatibility. Or at least the change (if intentional) should be documented in the changelog documentation.
Regards

Minimal reproduction of the problem with instructions:
Here a plunkr to replicate the issue:
https://plnkr.co/edit/FsWbrVvfAYvuJCoGszi2?p=preview

AngularJS version: 1.7.8

Browser: [all]

@lmartorella
Copy link
Author

Seems to be introduced in 1.7.6

@gkalpak
Copy link
Member

gkalpak commented Apr 7, 2019

Yes, this was indeed introduced in d6098ee. The reason it happens is that browsers seem to ignore invalid values for styles.

Previously, ngStyle would always remove the old styles (by means of .css(someStyleProp, '')) and then apply the new style. After d6098ee, if the style property is included in the new styles object, we assume that it will be overwritten when setting the new styles, so we don't clear the old style first.

But, when the property value is invalid (in this case, when isRight is false, we will try to set element.style.color = false, which is invalid), it is just ignored by the browser, thus the old style remains.

Not sure how to address this. It is not possible to detect whether a value is valid or not (OK, it might be possible, but it would be prohibitively costly), so we need to either:

  1. Revert d6098ee and keep the old behavior (which fixes this regression, but re-introduces the issue fixed by that commit).
  2. Bite the bullet, call it a breaking change and just document it as such (which is not idealmgiven that there should be no breaking changes in patch releases - even more so in LTS period 😁)

@lmartorella
Copy link
Author

Correct, the 'false' value is the culprit.
Rewriting the expression as:
ng-style="{'float': (isRight && 'right') || null }"
triggers the correct behavior.

BTW, how many DOM style properties support boolean values instead of strings? Most probably in the majority of the cases the 'false' value can safely remove the property.

Thanks, L

gkalpak added a commit to gkalpak/angular.js that referenced this issue Apr 27, 2019
Since d6098ee, old styles were not removed if `newStyles` specified an
invalid value for the style (e.g. `false`). The assumption was that the
new style would overwrite the old style value, but using an invalid
value made browsers ignore the new value and thus keep the old style.
This would typically happen when guarding a style with a boolean flag;
e.g.: `ng-style="{backgroundColor: isError && 'red'}"`

This commit essentially revers commit d6098ee, whose main purpose was
to work around jquery/jquery#4185. The jQuery issue has been fixed in
3.4.0, so that should not be a problem any more.

Fixes angular#16860
@gkalpak gkalpak closed this as completed in 10d1b19 May 9, 2019
gkalpak added a commit that referenced this issue May 9, 2019
Since d6098ee, old styles were not removed if `newStyles` specified an
invalid value for the style (e.g. `false`). The assumption was that the
new style would overwrite the old style value, but using an invalid
value made browsers ignore the new value and thus keep the old style.
This would typically happen when guarding a style with a boolean flag;
e.g.: `ng-style="{backgroundColor: isError && 'red'}"`

This commit essentially revers commit d6098ee, whose main purpose was
to work around jquery/jquery#4185. The jQuery issue has been fixed in
3.4.0, so that should not be a problem any more.

Fixes #16860

Closes #16868
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.