Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

ngAnimateSwap is not compatible with ngIf #16616

Closed
1 of 3 tasks
bersLucas opened this issue Jun 27, 2018 · 5 comments
Closed
1 of 3 tasks

ngAnimateSwap is not compatible with ngIf #16616

bersLucas opened this issue Jun 27, 2018 · 5 comments
Assignees
@gkalpak
Labels
component: ngAnimate needs: investigation type: bug
Milestone
1.7.x

Comments

@bersLucas
Copy link

I'm submitting a ...

  • bug report
  • feature request
  • other

Current behavior:

ngAnimateSwap registers a comment of the element, not the element itself, so if an element contains the ng-if attribute, ng-animate-swap will not work.

Expected / new behavior:

Having a conditional element also be animated if it's visible.

Minimal reproduction of the problem with instructions:

  1. Have an element with both ng-animate-swap and ng-if
  2. The element will always be invisible, as it swaps the comments.

https://codepen.io/anon/pen/LrgBwY

AngularJS version: 1.7.2

Browser: [all]

(tested on FF 62, OSX High Seirra )
@gkalpak
Copy link
Member

gkalpak commented Jun 27, 2018 •
edited
Loading

The problem is that both ngIf and ngAnimateSwap have a priority of 600, which means that (while both are terminal directives) they are executed on top of each other (essentially messing each other's comment node).

I can't be sure, but it seems like an oversight to me. ngAnimateSwap should have a lower priority.
As a workaround, you can overwrite ngAnimateSwap's default priority (demo):

angular.
  module('myApp', ['ngAnimate']).
  ...
  decorator('ngAnimateSwapDirective', $delegate => {
    // Give `ngAnimateSwap` a lower priority than `ngIf`'s 600.
    $delegate[0].priority = 599;
    return $delegate;
  });

Ideally, we should add this fix into ngAnimate, but I wonder if it might break some other usecase.

@gkalpak gkalpak added this to the 1.7.x milestone Jun 27, 2018
@gkalpak gkalpak self-assigned this Jun 27, 2018
@codymikol
Copy link

Is there any reason the team has decided this will not get a fix? Would you accept a community PR?

@gkalpak
Copy link
Member

gkalpak commented Sep 25, 2018

We haven't made our final decision yet (we'll do this week). My main concern is the possibly breaking change (as mentioned in #16616 (comment)).

@petebacondarwin
Copy link
Contributor

The reason that this has been moved to 1.7.x - won't fix is that we are now in LTS mode, which means that we are only fixing security and other significant bugs. But what @gkalpak is referring to, is that we are considering one more bug fix release, which may contain some issues from this milestone.

@gkalpak gkalpak modified the milestones: 1.7.x - won't fix, 1.7.x Sep 26, 2018
gkalpak added a commit to gkalpak/angular.js that referenced this issue Oct 11, 2018
@gkalpak
fix(ngAnimateSwap): make it compatible with ngIf on the same element
bce6533 
Previously, both `ngAnimateSwap` and `ngIf` had a priority of 600, which
meant that (while both are [terminal directives][1]) they were executed
on top of each other (essentially messing each other's comment node).

This commit fixes it, by giving `ngAnimateSwap` a priority of 550, which
is lower than `ngIf` but still higher than other directives.

For reference, here is a list of built-in directive per priority:

```
-400: ngInclude, ngView
  -1: ngRef
   1: ngMessage, ngMessageDefault, ngMessageExp, ngModel, select
  10: ngModelOptions
  99: ngHref, ngSrc, ngSrcset
 100: attr interpolation, ngChecked, ngDisabled, ngList, ngMax, ngMaxlength, ngMin, ngMinlength, ngModel (aria), ngMultiple, ngOpen, ngPattern, ngProp*, ngReadonly, ngRequired, ngSelected, ngStep, ngValue, option
 400: ngInclude, ngView
 450: ngInit
 500: ngController
 600: ngAnimateSwap, ngIf
1000: ngNonBindable, ngRepeat
1200: ngSwitchDefault, ngSwitchWhen
```

[1]: https://docs.angularjs.org/api/ng/service/$compile#-terminal-

Fixes angular#16616
@gkalpak gkalpak mentioned this issue Oct 11, 2018
Closed
gkalpak added a commit to gkalpak/angular.js that referenced this issue Oct 11, 2018
@gkalpak
fix(ngAnimateSwap): make it compatible with ngIf on the same element
fa335ef 
Previously, both `ngAnimateSwap` and `ngIf` had a priority of 600, which
meant that (while both are [terminal directives][1]) they were executed
on top of each other (essentially messing each other's comment node).

This commit fixes it, by giving `ngAnimateSwap` a priority of 550, which
is lower than `ngIf` but still higher than other directives.

For reference, here is a list of built-in directive per priority:

```
-400: ngInclude, ngView
  -1: ngRef
   1: ngMessage, ngMessageDefault, ngMessageExp, ngModel, select
  10: ngModelOptions
  99: ngHref, ngSrc, ngSrcset
 100: attr interpolation, ngChecked, ngDisabled, ngList, ngMax, ngMaxlength, ngMin, ngMinlength, ngModel (aria), ngMultiple, ngOpen, ngPattern, ngProp*, ngReadonly, ngRequired, ngSelected, ngStep, ngValue, option
 400: ngInclude, ngView
 450: ngInit
 500: ngController
 600: ngAnimateSwap, ngIf
1000: ngNonBindable, ngRepeat
1200: ngSwitchDefault, ngSwitchWhen
```

[1]: https://docs.angularjs.org/api/ng/service/$compile#-terminal-

Fixes angular#16616
gkalpak added a commit to gkalpak/angular.js that referenced this issue Oct 11, 2018
@gkalpak
fix(ngAnimateSwap): make it compatible with ngIf on the same element
f5e0525 
Previously, both `ngAnimateSwap` and `ngIf` had a priority of 600, which
meant that (while both are [terminal directives][1]) they were executed
on top of each other (essentially messing each other's comment node).

This commit fixes it, by giving `ngAnimateSwap` a priority of 550, which
is lower than `ngIf` but still higher than other directives.

For reference, here is a list of built-in directive per priority:

```
-400: ngInclude, ngView
  -1: ngRef
   1: ngMessage, ngMessageDefault, ngMessageExp, ngModel, select
  10: ngModelOptions
  99: ngHref, ngSrc, ngSrcset
 100: attr interpolation, ngChecked, ngDisabled, ngList, ngMax,
      ngMaxlength, ngMin, ngMinlength, ngModel (aria), ngMultiple,
      ngOpen, ngPattern, ngProp*, ngReadonly, ngRequired, ngSelected,
      ngStep, ngValue, option
 400: ngInclude, ngView
 450: ngInit
 500: ngController
 600: ngAnimateSwap, ngIf
1000: ngNonBindable, ngRepeat
1200: ngSwitchDefault, ngSwitchWhen
```

[1]: https://docs.angularjs.org/api/ng/service/$compile#-terminal-

Fixes angular#16616
gkalpak added a commit that referenced this issue Oct 15, 2018
@gkalpak
fix(ngAnimateSwap): make it compatible with ngIf on the same element
b27080d 
Previously, both `ngAnimateSwap` and `ngIf` had a priority of 600, which
meant that (while both are [terminal directives][1]) they were executed
on top of each other (essentially messing each other's comment node).

This commit fixes it, by giving `ngAnimateSwap` a priority of 550, which
is lower than `ngIf` but still higher than other directives.

For reference, here is a list of built-in directive per priority:

```
-400: ngInclude, ngView
  -1: ngRef
   1: ngMessage, ngMessageDefault, ngMessageExp, ngModel, select
  10: ngModelOptions
  99: ngHref, ngSrc, ngSrcset
 100: attr interpolation, ngChecked, ngDisabled, ngList, ngMax,
      ngMaxlength, ngMin, ngMinlength, ngModel (aria), ngMultiple,
      ngOpen, ngPattern, ngProp*, ngReadonly, ngRequired, ngSelected,
      ngStep, ngValue, option
 400: ngInclude, ngView
 450: ngInit
 500: ngController
 600: ngAnimateSwap, ngIf
1000: ngNonBindable, ngRepeat
1200: ngSwitchDefault, ngSwitchWhen
```

[1]: https://docs.angularjs.org/api/ng/service/$compile#-terminal-

Fixes #16616

Closes #16729
@codymikol
Copy link

Thanks @gkalpak 🐔 💯

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

@gkalpak gkalpak

Labels
component: ngAnimate needs: investigation type: bug
Projects
None yet
Milestone
1.7.x
Development

Successfully merging a pull request may close this issue.

fix(ngAnimateSwap): make it compatible with ngIf on the same element gkalpak/angular.js
4 participants
@petebacondarwin @bersLucas @gkalpak @codymikol