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

Invalid argument in IE when using $compile #8659

Closed
Matsemann opened this issue Aug 18, 2014 · 32 comments
Closed

Invalid argument in IE when using $compile #8659

Matsemann opened this issue Aug 18, 2014 · 32 comments

Comments

@Matsemann
Copy link

A fiddle with a trivial example: http://jsfiddle.net/1t4crp6e/

<div ng-app="myApp">
    <div ng-controller="myctrl">
        <div test>the value is: {{testmodel}}</div>

        <br>
        <input type="text" ng-model="testmodel">
    </div>
</div>
....
link: function(scope, elm, attrs) {
            var standardText = elm.text();

            elm.html('something');

            $timeout(function() {
                console.log(standardText);
                elm.html('').append($compile('<span>' + standardText + '</span>')(scope.$parent));
            }, 2000);
        }

Wait 2 secs, see the content of directive change to a binding. In Firefox, everything works. In IE it works, but it throws a lot of errors in the console.
(Don't worry about what I'm doing here, I just made a small example showing the issue)

Error: Invalid argument.
at Anonymous function (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:62:80)
at k.prototype.$digest (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:109:350)
at k.prototype.$apply (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:112:343)
at Anonymous function (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:18:158)
at d (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:34:474)
at c (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:18:66)
at fc (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:18:270)
at Xc (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:17:362)
at Anonymous function (http://ajax.googleapis.com/ajax/libs/angularjs/1.2.22/angular.min.js:214:78)
at o (https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js:2:14725)

However, most of my tries of actually writing this worked in all browsers except IE. For instance, if I remove the span in the compile, and instead of elm.text() use elm.contents().
http://jsfiddle.net/1t4crp6e/1/ Only changed line 13 and 19 in the js, breaks horribly in IE. It's almost impossible to compile stuff in IE.

Am I doing something wrong? And any workarounds?

@Matsemann
Copy link
Author

Update: The error message here wasn't that helpful. When doing it not inside an anonymous function this is the message:

Error: Invalid argument.
at interpolateFnWatchAction (http://localhost:8080/app/libs/angular/angular.js:6957:19)
at Scope.prototype.$digest (http://localhost:8080/app/libs/angular/angular.js:12497:23)
at Scope.prototype.$apply (http://localhost:8080/app/libs/angular/angular.js:12762:13)
at listener (http://localhost:8080/app/libs/angular/angular.js:16998:9)
at jQuery.event.dispatch (http://localhost:8080/app/libs/jquery/jquery.js:3073:6)
at elemData.handle (http://localhost:8080/app/libs/jquery/jquery.js:2749:5)

@caitp
Copy link
Contributor

caitp commented Aug 18, 2014

Seems to throw one error, it has issues assigning an interpolated value to the node (not sure if it's the subsequently compiled <span>, or if it's the <div>).

If you can figure out what it's trying to assign to nodeValue when it throws, that would be for the best.

I don't think this is breaking any of your other watches though (i'd have to double check), but it looks like $exceptionHandler is dealing with this nicely

@Matsemann
Copy link
Author

It will rethrow the error each time you change the value in the input box. It happens also in my second example (but this time everything breaks in IE), so it may not be the span?

Not sure how to debug this? It happens the next digest as far as I can tell, so difficult to track with my limited experience since it happens so much between my code and the error.

I agree, it seems to work as it should (the first example) except for all the errors. Why doesn't the second one work, though? Any ideas for a better way to compile stuff inside a directive?

@caitp
Copy link
Contributor

caitp commented Aug 18, 2014

Any ideas for a better way to compile stuff inside a directive?

Don't compile stuff inside a directive, this isn't what you want to do unless you really understand the html compiler

@Matsemann
Copy link
Author

Any ideas for a different way to achieve what I'm trying?

I have a button, which the directive is applied to. The directive watches a value set by the controller, that changes the content of the button.
<button button-loader="isLoading">Save {{model.value}}</button>

So originally the content of the button would be:
Save article 1
then $scope.isLoading=true would make the content be:
Saving
then $scope.isLoading=false would make the content be:
Saved
and then after 1500ms the content again should be
Save article 1
with the original databinding.

My code is here, but it contains some other stuff as well.
What I'm trying to do is add a compile to showStandard (around line 70) to support databinding inside the button, not just raw text.

@Matsemann
Copy link
Author

Transcluding it, and then just show/hide the original binding and show something else when needed seem to work fine.

@caitp caitp added this to the Backlog milestone Aug 20, 2014
@siemiatj
Copy link

I've noticed that in my case returning from textInterpolateLinkFn, when node.nodeValue is an empty string fixes all problems in IE:

            return function textInterpolateLinkFn(scope, node) {
              var parent = node.parent();
              if (!hasCompileParent) compile.$$addBindingClass(parent);
              compile.$$addBindingInfo(parent, interpolateFn.expressions);
              scope.$watch(interpolateFn, function interpolateFnWatchAction(value) {
                if (!node[0].nodeValue) {
                  return;
                }

                node[0].nodeValue = value;
              });
            };

I can describe my use case or create a runnable test case if needed.

@kentcdodds
Copy link
Member

I've noticed this behavior in my app. Not certain how to reproduce it, but based on this stack overflow question it appears to be an issue when the node has no child nodes and therefore nodeValue is null (I guess?) which Internet Explorer has issues with. I'm testing with IE 10. I'm not certain of optimal fix here... In my case, it's simply trying to assign node[0].nodeValue to "Save". But I believe what it is attempting to assign the value to is irrelevant. I think IE just has the problem when you attempt to access nodeValue on a node with no child nodes...

Here's a jsbin based on that Stack Overflow question which shows the issue (when using IE).

@kentcdodds
Copy link
Member

I was able to resolve my issue by going from this kind of binding:

<span>{{foo}}</span>

to using the directive version:

<span ng-bind="foo"></span>

Would like to not have to do that if I can help it though :-)

@just-boris
Copy link
Contributor

related issue angular-ui/ui-select#276
solved same way that @kentcdodds did

@Javarome
Copy link

Javarome commented May 7, 2015

Had the same issue on the dynamic text of a button ; solved by kentcdodds' ng-bind workaround.

@JeanMeche
Copy link
Member

While there is a workaround. A fix would be welcome :)

@daniel-nalbach
Copy link

I ran into a variant of this issue as well with Angular 1.2.15 with IE 11. It was caused by this dynamic button text:

<button class="someclasses" ng-click="toggleSomething()">{{ showText ? 'Hide' : 'View' }} Some Text</button>

The error was caused by the addTextInterpolateDirective function last compile line of:

node[0].nodeValue = value

When console logging the node, it was a parentless text node and would error out in IE.
Moving the expression to an ng-bind resolved the issue.

@cassilup
Copy link

cassilup commented Jul 7, 2015

I am also eagerly waiting for a fix to this issue.

Meanwhile, changing this:

    <button class="someclasses" ng-click="toggleSomething()">
        {{ showText ? 'Hide' : 'View' }} Some Text
    </button>

to this (notice the ng-if):

    <button class="someclasses" ng-click="toggleSomething()" ng-if="showText">
        {{ showText ? 'Hide' : 'View' }} Some Text
    </button>

fixes it for now.

Logic: ng-if adds/removes element from DOM based on the truthfulness of the expression. If the element doesn't exist in the DOM, IE won't complain if no text node is present on it.

Downside: template code pollution.

@wawyed
Copy link

wawyed commented Sep 3, 2015

Any updates on this issue?

@siemiatj
Copy link

siemiatj commented Sep 9, 2015

How about we just use this 3 line fix that I've posted and be done with it ?

@TombolaShepless
Copy link

Also experiencing this issue, any word on a timescale for the fix proposed to be implemented?

@vibrant-aaronstreate
Copy link

I'm seeing the same issue. Is this going to be fixed soon?

@wombleton
Copy link

If you're using webpack, I added this loader to solve this problem as per @siemiatj's response: https://www.npmjs.com/package/falafel-loader

@bogdanmatra
Copy link

This approach fixed it for me.
empty().append() instead of html()
knalli/angular-translate@c4b16d3

element.empty().append("<p>my html</p>");
$compile(element.contents())(scope);

// instead of:

element.html("<p>my html</p>");
$compile(element.contents())(scope);

@wwwillchen
Copy link
Contributor

Would love to see an official fix for this as well. We are using this for a large-scale Angular app and the interim solutions will be a pain to use.

@Narretz Narretz self-assigned this Oct 27, 2015
@silentHoo
Copy link

+1 on that issue here, we also have a very large AngularJS app to maintain.

@kentcdodds
Copy link
Member

For people experiencing this issue, I would recommend you simply use the work-around of ng-bind. There are actually a few reasons it's better than {{ }} anyway.

@bogdanmatra
Copy link

Hey kentcdodds, ng-bind can be a valid solution but when you have to use $compile on a lot of code changing all {{ }} to ng-bind can be frustrating. The above solution fixed the issue for me. The difference in the jQuery implementation between .html() and .empty().append() seems to do the trick on IE for me.

@Narretz
Copy link
Contributor

Narretz commented Nov 3, 2015

I've tested the fix that @siemiatj provided, but it makes a ton of other tests fail, so we cannot use it. Given that there are plenty of workarounds for this issue, this issue doesn't have high priority. I'll invite you to build on the PR and find a solution, though: #13243

@rent-a-developer
Copy link

Please correct me if I am wrong...

To me the observed behavior (the 'Invalid argument' error) does not seem to be a bug in IE or in angular at all.

Lets examine the code in the opening post:

  1. There is a div where a directive is applied to:
    <div test>the value is: {{testmodel}}</div>
  2. The div contains an angular expression:
    {{testmodel}}
  3. The directive then removes all child nodes of the div:
    elm.html('something');

(You could also do elm.html("") or elm.empty())

So what happens here?

  1. Angular must watch the scope variable "testmodell" for changes and must update the DOM node that contains the {{testmodel}} expression whenever the scope variable has changed.
    So angular creates a watch for the {{testmodel}} expression, which means it must also keep a reference to the DOM node that contains the {{testmodel}} expression, to be able to update it when the scope variable has changed.
  2. When the directive removes the DOM node (elm.html('something')) that contains the {{testmodel}} expression, angular does not know about the fact that this DOM node does not need to be updated anymore and that the watch for the expression should be removed.
    It still holds a reference to the DOM node that contains the {{testmodel}} expression and the watch on the scope variable is still active.
    But this DOM node (the text node containing {{testmodel}}) is no longer part of the DOM tree of the webpage.
  3. When the scope variable has been changed angular tires to update the DOM node (that is now not a part of the DOM tree of the web page anymore), because the watch on the testmodel scope variable is still active.
    Of course updating a detached (abandoned) DOM node is not a good idea.
    However, this seems to work in browsers like Chrome or FF, but fails in IE.
    It fails in IE for a good reason: The DOM node is not part of the DOM tree anymore (in some sense like a memory leak).

So to me it seems to not be a bug in IE or in angular, but rather in the code presented in the opening post.
One should not modify DOM nodes directly where angular watches exist on.
Instead angular should modify the DOM (via ng-if, ng-repeat, etc...), so it knows when removed DOM nodes do not need to be updated anymore.

However it's not always possible to let angular manage the DOM.
But when we modify the DOM directly, we must in some way notify angular about it.
The simplest way is to destroy the scope of removed DOM nodes.

So the solution would be to create a child scope for the DOM element that contains the {{testmodel}} expression.
When we remove that DOM node, we can call $destroy() on the child scope and angular knows it must not update the DOM node any longer.
Problem solved.

Well...
Not completely solved.
There seems to be some other cases where a similar problem happens to:
#5025

@Narretz
Copy link
Contributor

Narretz commented Nov 13, 2015

@rent-a-developer Thanks for investigating, I think you are right that this is actually expected behavior, it's just that many browsers tolerate it, while IE does not.
Thinking about it again, the example in the OP includes a bad practice: namely re-compiling already compiled and linked DOM nodes. The interpolated text node is already compiled and linked before it is removed from the DOM, and then it is recompiled, which not only makes IE complain, but also adds another watcher on the scope, which is bad for perf reasons.
If you want to extract text nodes / content from the DOM and re-compile it, you should either do it in the compile fn or make sure no other directive is executed before you do your stuff in the link fn:
http://jsfiddle.net/1t4crp6e/8/

@lgalfaso Do you agree that this is actually expected behavior?

@petebacondarwin
Copy link
Member

I agree with @Narretz - that if you are going to modify the contents of the node and potentially recompile then you should be doing this in the compile function and probably terminating the compilation too.
On top of that, whenever you intend to be recompiling elements during the application run then you need to create new scopes to ensure that they can be destroyed correctly to prevent leaks.

@Narretz
Copy link
Contributor

Narretz commented Nov 26, 2015

Based on the explanations of @rent-a-developer and @petebacondarwin and my recommendations, I will close this issue as "wontfix"

@Cerbrus
Copy link

Cerbrus commented Dec 1, 2015

I recently came across the same issue rendering a kendo treeview with an angular template.

The template used to be:

<span k-template>
    <span id="{{dataItem.id + '_' + dataItem.entityType}}"
          ng-attr-title="{{dataItem.title}}">
        {{dataItem.text}}
    </span>
</span>

Adding an ng-if in there fixed the issue:

<span k-template>
    <span ng-if="dataItem"
          id="{{dataItem.id + '_' + dataItem.entityType}}"
          ng-attr-title="{{dataItem.title}}">
        {{dataItem.text}}
    </span>
</span>

@askucher
Copy link

askucher commented Feb 3, 2016

+1

@Narretz
Copy link
Contributor

Narretz commented Feb 4, 2016

I'm going to lock this issue. Please see the explanations in #8659 (comment) and the recommended course of action in #8659 (comment).
If you see any 3rd party module doing this (re-compiling a node), then this is a problem that should be pointed out to them.
If you think you've found a genuine bug, please open a new issue.

@angular angular locked and limited conversation to collaborators Feb 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests