Fixed bug with using angular-off-click in ng-repeat

[willhoney7]

I found a bug where the directive would have issues when it was used multiple times. This pull request fixes that. 🚀

In directives, the compile function itself is only called once, meaning that in the current code, elmId is set to 0 at the very beginning and never changes... This causes issues when multiple offClick directives are added since the listeners will overwrite each other (since they have the same elmId of 0).

compile: (elem, attrs) => {
    const fn = $parse(attrs.offClick);

    const elmId = id++; // only called once
    let removeWatcher;

    return (scope, element) => {
        const on = () => {
            listeners[elmId] = {
                elm: element[0],
                cb: fn,
                scope: scope
            };
        };

        ...
    }
}

This can be fixed be placing the code that was outside of the inner compile function on the inside, like so.

compile: (elem, attrs) => {
    return (scope, element) => {
        const fn = $parse(attrs.offClick);

        const elmId = id++;
        let removeWatcher;

        const on = () => {
            listeners[elmId] = {
                elm: element[0],
                cb: fn,
                scope: scope
            };
        };
        .....
    }
}

I made this change, and then changed it from a compile function to a link... since there's no need for it to be a compile. Compile is typically for early DOM manipulation – which isn't necessary. I found this article to be super helpful while I was investigating this bug: http://www.jvandemo.com/the-nitty-gritty-of-compile-and-link-functions-inside-angularjs-directives/

Thanks so much for the directive! Let me know if I missed something!

Will

[TheSharpieOne]

Not too sure I understand what you are saying the problem that this change fixes is.
Yes, elmId is set to 0 for the first directive instance it finds, then the next directive the code runs again and this time elmId is set to 1, then for the third instance, it will be 2. Each element with the off-click attribute will have a different, incremented elmId.

See the console output of: https://jsfiddle.net/TheSharpieOne/kff4qeqx/ (Note, I just copied the non-minified dist and added a console.log to after elmId is set.

[Ricardo-Marques]

I found a bug where the directive would have issues when it was used multiple times.

What do you mean exactly?

[willhoney7]

Hrrrmmmmm. You're right – that fiddle works properly. But it definitely doesn't work properly in my production code. I was thinking it may have to do with the version of angular, but I don't believe that's it. I'm going to keep playing with it until I can replicate in a fiddle the bug as it appears in my production code.

The issue is that when I use multiple off-click directives, sometimes the off click will fail to fire, because the listener was getting overwritten since elmId was always 0.

Thanks!

[willhoney7]

There! I figured out the cause. The issue occurs when one uses a compile function in a directive with ng-repeat; compile only gets called once while link will get called every time. This StackOverflow answers explains a little bit: http://stackoverflow.com/a/29989258

Here's a fiddle that demonstrates the issue: https://jsfiddle.net/gafv1xgv/

This pull request will fix that issue and place all the code in a link function which gets executed on every instance of the directive – regardless of whether it was placed in an ng-repeat (as is to be expected).

[TheSharpieOne]

I see. Thanks for tracking down the issue.
The idea behind using compile was to be more inline with what angular is doing for it's event handling. Somewhere during the rewrite, more functionality was added outside of the function which is returned (the link function). I'd say that just the $parse piece should be kept there and the rest moved down a few lines to the returned function. This solves the issue just the same but saves some cycles with the parse call since in the repeat the attribute will always be the same expression in the loop/repeat (though it may be different once parsed).

[Ricardo-Marques]

Excellent job tracking this one down, @Tibfib.
I will publish this new version tomorrow!

[willhoney7]

Oh, can you guys do a release with the fix?

[Ricardo-Marques]

@Tibfib Done - sorry about that

[Ricardo-Marques]

@Tibfib Terribly sorry - I pushed and published without running the build first. Resolved in v1.0.7