ngTransclude breaks directive execution when priority is set to -1 or lower

[dknell]

I'm submitting a ...

• bug report
• feature request
• other

Current behavior:

As of angularjs 1.5.8, directives that transclude other directives (for example a requireAuth directive to block any events unless the user is authenticated) that have a priority of less than 0 will not fire.

Expected / new behavior:

Directives with a priority: -1 should ALWAYS fire before priority: 0 directives.

Minimal reproduction of the problem with instructions:

https://codepen.io/dknell/pen/dJjvVy

AngularJS version: 1.x.y

1.5.8

Browser: [all | Chrome XX | Firefox XX | Edge XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

all

Anything else:

This bug was introduced when terminal: true was added to ngTransclude here. Why would we want ngTransclude to block all execution of transcluded directives? This was not the case with versions 1.5.7 and below. There is no mention in the PR for why this was added.

[Narretz]

Strange! I can't tell from the commit why terminal: true was included. No tests failed when I removed it. Maybe to prevent memory leaks? Idk. Maybe it was needed in this PR, but then made obsolete in the follow-up PR: #14787 @dcherman and @petebacondarwin can you take a look at this, since you made these changes?

[dcherman]

@Narretz Based on my original PR and memory from a while ago, terminal was used in order to stop the fallback content from being linked unnecessarily in the event that transcluded content was available.

In the followup PR, the contents are being detached in the compile step in order to accomplish the same thing.

While terminal was required in the original PR, I don't think it's doing anything anymore since the contents are being detached, so you can probably remove it.

[Narretz]

@dcherman thanks for the quick response - this must be the case. I confirmed that your implementation only works with terminal: true, but the new implementation doesn't need it. I'll prepare a PR to remove it from ngTransclude.

[dknell]

Thanks @dcherman and @Narretz for the quick responses!

[dknell]

@Narretz I noticed this was added to the Ice Box milestone, although this issue has a major impact on our app after upgrading to 1.5.11. We can downgrade to 1.5.7, but I really would like to avoid that. What are the chances of this making this into a patch in the next 7 days?

[Narretz]

It was added to Icebox because I didn't know if it was easy to fix. But there's already a PR: #16412
However, we don't update 1.5.x anymore. You'll have to update to 1.6.9 to get it. There's a good chance we'll release that in the next 7 days though.