fix($compile): workaround for IE11 MutationObserver #11796

lgalfaso · 2015-05-03T19:28:24Z

IE11 MutationObserver breaks consecutive text nodes into several text nodes.
This patch merges consecutive text nodes into a single node before
looking for interpolations.

Closes #11781

petebacondarwin · 2015-05-05T10:56:51Z

Does this only happen on IE11 or other versions of IE?
Can you wrap this code in a conditional: if(msie===11) {...}?

lgalfaso · 2015-05-05T15:30:00Z

@petebacondarwin AFAICT, this is IE11 only and when there is a MutationObserver

lgalfaso · 2015-05-05T20:39:19Z

BTW, the test as is fails on all browsers. The main issue is that only IE11 generates multiple consecutive text elements/

petebacondarwin · 2015-05-06T06:56:19Z

test/ng/compileSpec.js

+    it('should handle consecutive text elements as a single text element', inject(function($rootScope, $compile) {
+      var base = jqLite('<div></div>');
+      base[0].appendChild(document.createTextNode('Lucas {{1+'));
+      base[0].appendChild(document.createTextNode('2}} Lucas'));


I think we should test the specific situation that causes the bug, i.e. that there is a mutation observer and that the text contains some special characters that trigger IE11 to split it into multiple text nodes.

lgalfaso · 2015-05-10T21:12:33Z

@petebacondarwin can you take a second look at this?

petebacondarwin · 2015-06-03T11:38:22Z

test/ng/compileSpec.js

@@ -2762,6 +2762,23 @@ describe('$compile', function() {
    }));


+    it('should handle consecutive text elements as a single text element', inject(function($rootScope, $compile) {
+      window.addMutationObserver = function() {


Should we really be modifying the window object here?

Does this have any effect anyway ? Shouldn't we call the function as well ?
(And indeed there doesn't seem to be a reason to modify window.)

The test, without fiddling with window passes without the fix on IE 11.
IE11 parses text node differently when there is a mutation observer, this is why the test add it.

But if the function isn't called, then there is no MutationObserver added right ?
How does the function help if we don't call it ?
And why does the function need to be assigned on window ?

@gkalpak I think I am missing something. This is a test. IE11 parses html different when there is any mutation observer registered. The test reproduces this scenario.

@lgalfaso, if I recall correctly, the issue appeared only when there was a childList+subtree MutationObserver registered on the page. I would expect a test that calls new MutationObserver(...).observe(...).

Instead I see:

window.addMutationObserver = function () { // A function that _would_ register a MutationObserver on body // IF it where executed. }; // Then I see the actual test code being executed without ever calling `window.addMutationObserver()` // which would have registered the MutationObserver. AFAICT, the MutationObserver is never created.

Based on the above, there doesn't seem to be any MutationObserver during this test, so it would pass even without the fix (I am just speculating - I haven't actually tried it).

I would expect something along these lines:

it ('...', inject(function($compile, $rootScope) { // No point it running the test, if there is no MutationObserver if (!window.MutationObserver) return; // Create and register the MutationObserver var observer = new window.MutationObserver(noop); observer.observe(document.body, {childList: true, sublist: true}); // Run the actual test var base = jqLite('<div>— {{ "This doesn\'t." }}</div>'); element = $compile(base)($rootScope); $rootScope.$digest(); expect(element.text()).toBe("— This doesn't."); // Unregister the MutationObserver (and hope it doesn't mess up with subsequent tests) observer.disconnect(); }));

Updated the test

@lgalfaso, I noticed that the added test would pass on IE11 even without the fix.
Looking into it, it turned out I (unwittingly) tricked you into changing subtree: true to sublist: true, which effectively makes the test useless 😞

Submitted a fix in #12061.

petebacondarwin · 2015-06-03T11:38:42Z

Other than my comment about window this looks good to me.

IE11 MutationObserver breaks consecutive text nodes into several text nodes. This patch merges consecutive text nodes into a single node before looking for interpolations. Closes angular#11781

lgalfaso · 2015-06-06T09:27:11Z

landed as f3b1d0b

Backport angular#11796 to 1.2 branch. IE11 MutationObserver breaks consecutive text nodes into several text nodes. This patch merges consecutive text nodes into a single node before looking for interpolations. Closes angular#11781

Backport angular#11796 to 1.2 branch. IE11 MutationObserver breaks consecutive text nodes into several text nodes. This patch merges consecutive text nodes into a single node before looking for interpolations. Also had to modify npm-shrinkwrap.json because [email protected] was unpublished from npm. Closes angular#11781

Backport #11796 to 1.2 branch. IE11 MutationObserver breaks consecutive text nodes into several text nodes. This patch merges consecutive text nodes into a single node before looking for interpolations. Also had to modify npm-shrinkwrap.json because [email protected] was unpublished from npm. Closes #11781 Closes #12613

Related to angular#11796. Fixes angular#14924 BREAKING CHANGE: // TODO

As explained in angular#11781 and angular#14924, IE11 can (under certain circumstances) break up a text node into multiple consecutive ones, breaking interpolated expressions (e.g. `{{'foo'}}` would become `{{` + `'foo'}}`). To work-around this IE11 bug, angular#11796 introduced extra logic to merge consecutive text nodes (on IE11 only), which relies on the text nodes' having the same `parentNode`. This approach works fine in the common case, where `compileNodes` is called with a live NodeList object, because removing a text node from its parent will automatically update the latter's `.childNodes` NodeList. It falls short though, when calling `compileNodes` with either a jqLite/jQuery collection or an Array. In fails in two ways: 1. If the text nodes do not have a parent at the moment of compiling, there will be no merging. (This happens for example on directives with `$transclude: {...}`.) 2. If the text nodes do have a parent, just removing a text node from its parent does **not** remove it from the collection/array, which means that the merged text nodes will still get compiled and linked (and possibly be displayed in the view). E.g. `['{{text1}}', '{{text2}}', '{{text3}}']` will become `['{{text1}}{{text2}}{{text3}}', '{{text2}}', '{{text3}}']`. -- This commit works around the above problems by: 1. Merging consecutive text nodes in the provided list, even if they have no parent. 2. When merging a txt node, explicitly remove it from the list (unless it is a live, auto-updating list). This can nonetheless have undesirable (albeit rare) side effects by overzealously merging text nodes that are not meant to be merged (see "BREAKING CHANGE" section below). Fixes angular#14924 BREAKING CHANGE: **Note:** Everything described below affects **IE11 only**. Previously, consecutive text nodes would not get merged if they had no parent. They will now, which might have unexpectd side effects in the following cases: 1. Passing an array or jqLite/jQuery collection of parent-less text nodes to `$compile` directly: ```js // Assuming: var textNodes = [ document.createTextNode('{{'), document.createTextNode('"foo"'), document.createTextNode('}}') ]; var compiledNodes = $compile(textNodes)($rootScope); // Before: console.log(compiledNodes.length); // 3 console.log(compiledNodes.text()); // {{'foo'}} // After: console.log(compiledNodes.length); // 1 console.log(compiledNodes.text()); // foo // To get the old behavior, compile each node separately: var textNodes = [ document.createTextNode('{{'), document.createTextNode('"foo"'), document.createTextNode('}}') ]; var compiledNodes = angular.element(textNodes.map(function (node) { return $compile(node)($rootScope)[0]; })); ``` 2. Using multi-slot transclusion with non-consecutive, default-content text nodes (that form interpolated expressions when merged): ```js // Assuming the following component: .compoent('someThing', { template: '<ng-transclude></ng-transclude>' transclude: { ignored: 'veryImportantContent' } }) ``` ```html  <some-thing> <ng-transclude> {{ <-- Two separate 'foo'}} <-- text nodes </ng-transclude> </some-thing>  <some-thing> <ng-transclude> foo <-- The text nodes were merged into `{{'foo'}}`, which was then interpolated </ng-transclude> </some-thing>  <some-thing> <ng-transclude> {{ <-- Two separate 'foo'}} <-- nodes </ng-transclude> </some-thing> ```

As explained in angular#11781 and angular#14924, IE11 can (under certain circumstances) break up a text node into multiple consecutive ones, breaking interpolated expressions (e.g. `{{'foo'}}` would become `{{` + `'foo'}}`). To work-around this IE11 bug, angular#11796 introduced extra logic to merge consecutive text nodes (on IE11 only), which relies on the text nodes' having the same `parentNode`. This approach works fine in the common case, where `compileNodes` is called with a live NodeList object, because removing a text node from its parent will automatically update the latter's `.childNodes` NodeList. It falls short though, when calling `compileNodes` with either a jqLite/jQuery collection or an Array. In fails in two ways: 1. If the text nodes do not have a parent at the moment of compiling, there will be no merging. (This happens for example on directives with `$transclude: {...}`.) 2. If the text nodes do have a parent, just removing a text node from its parent does **not** remove it from the collection/array, which means that the merged text nodes will still get compiled and linked (and possibly be displayed in the view). E.g. `['{{text1}}', '{{text2}}', '{{text3}}']` will become `['{{text1}}{{text2}}{{text3}}', '{{text2}}', '{{text3}}']`. -- This commit works around the above problems by: 1. Merging consecutive text nodes in the provided list, even if they have no parent. 2. When merging a txt node, explicitly remove it from the list (unless it is a live, auto-updating list). This can nonetheless have undesirable (albeit rare) side effects by overzealously merging text nodes that are not meant to be merged (see "BREAKING CHANGE" section below). Fixes angular#14924 BREAKING CHANGE: **Note:** Everything described below affects **IE11 only**. Previously, consecutive text nodes would not get merged if they had no parent. They will now, which might have unexpectd side effects in the following cases: 1. Passing an array or jqLite/jQuery collection of parent-less text nodes to `$compile` directly: ```js // Assuming: var textNodes = [ document.createTextNode('{{'), document.createTextNode('"foo"'), document.createTextNode('}}') ]; var compiledNodes = $compile(textNodes)($rootScope); // Before: console.log(compiledNodes.length); // 3 console.log(compiledNodes.text()); // {{'foo'}} // After: console.log(compiledNodes.length); // 1 console.log(compiledNodes.text()); // foo // To get the old behavior, compile each node separately: var textNodes = [ document.createTextNode('{{'), document.createTextNode('"foo"'), document.createTextNode('}}') ]; var compiledNodes = angular.element(textNodes.map(function (node) { return $compile(node)($rootScope)[0]; })); ``` 2. Using multi-slot transclusion with non-consecutive, default-content text nodes (that form interpolated expressions when merged): ```js // Assuming the following component: .compoent('someThing', { template: '<ng-transclude></ng-transclude>' transclude: { ignored: 'veryImportantContent' } }) ``` ```html  <some-thing> {{ <very-important-content>Nooot</very-important-content> 'foo'}} </some-thing>  <some-thing> <ng-transclude> {{ <-- Two separate 'foo'}} <-- text nodes </ng-transclude> </some-thing>  <some-thing> <ng-transclude> foo <-- The text nodes were merged into `{{'foo'}}`, which was then interpolated </ng-transclude> </some-thing>   <some-thing> {{ <very-important-content>Nooot</very-important-content> 'foo'}} </some-thing>  <some-thing> <ng-transclude> {{ <-- Two separate 'foo'}} <-- nodes </ng-transclude> </some-thing> ```

As explained in #11781 and #14924, IE11 can (under certain circumstances) break up a text node into multiple consecutive ones, breaking interpolated expressions (e.g. `{{'foo'}}` would become `{{` + `'foo'}}`). To work-around this IE11 bug, #11796 introduced extra logic to merge consecutive text nodes (on IE11 only), which relies on the text nodes' having the same `parentNode`. This approach works fine in the common case, where `compileNodes` is called with a live NodeList object, because removing a text node from its parent will automatically update the latter's `.childNodes` NodeList. It falls short though, when calling `compileNodes` with either a jqLite/jQuery collection or an Array. In fails in two ways: 1. If the text nodes do not have a parent at the moment of compiling, there will be no merging. (This happens for example on directives with `transclude: {...}`.) 2. If the text nodes do have a parent, just removing a text node from its parent does **not** remove it from the collection/array, which means that the merged text nodes will still get compiled and linked (and possibly be displayed in the view). E.g. `['{{text1}}', '{{text2}}', '{{text3}}']` will become `['{{text1}}{{text2}}{{text3}}', '{{text2}}', '{{text3}}']`. -- This commit works around the above problems by: 1. Merging consecutive text nodes in the provided list, even if they have no parent. 2. When merging a text node, explicitly remove it from the list (unless it is a live, auto-updating list). This can nonetheless have undesirable (albeit rare) side effects by overzealously merging text nodes that are not meant to be merged (see the "BREAKING CHANGE" section below). Fixes #14924 Closes #15025 BREAKING CHANGE: **Note:** Everything described below affects **IE11 only**. Previously, consecutive text nodes would not get merged if they had no parent. They will now, which might have unexpectd side effects in the following cases: 1. Passing an array or jqLite/jQuery collection of parent-less text nodes to `$compile` directly: ```js // Assuming: var textNodes = [ document.createTextNode('{{'), document.createTextNode('"foo"'), document.createTextNode('}}') ]; var compiledNodes = $compile(textNodes)($rootScope); // Before: console.log(compiledNodes.length); // 3 console.log(compiledNodes.text()); // {{'foo'}} // After: console.log(compiledNodes.length); // 1 console.log(compiledNodes.text()); // foo // To get the old behavior, compile each node separately: var textNodes = [ document.createTextNode('{{'), document.createTextNode('"foo"'), document.createTextNode('}}') ]; var compiledNodes = angular.element(textNodes.map(function (node) { return $compile(node)($rootScope)[0]; })); ``` 2. Using multi-slot transclusion with non-consecutive, default-content text nodes (that form interpolated expressions when merged): ```js // Assuming the following component: .compoent('someThing', { template: '<ng-transclude></ng-transclude>' transclude: { ignored: 'veryImportantContent' } }) ``` ```html  <some-thing> {{ <very-important-content>Nooot</very-important-content> 'foo'}} </some-thing>  <some-thing> <ng-transclude> {{ <-- Two separate 'foo'}} <-- text nodes </ng-transclude> </some-thing>  <some-thing> <ng-transclude> foo <-- The text nodes were merged into `{{'foo'}}`, which was then interpolated </ng-transclude> </some-thing>   <some-thing> {{ <very-important-content>Nooot</very-important-content> 'foo'}} </some-thing>  <some-thing> <ng-transclude> {{ <-- Two separate 'foo'}} <-- nodes </ng-transclude> </some-thing> ```

googlebot added the cla: yes label May 3, 2015

lgalfaso mentioned this pull request May 3, 2015

Interpolations containing HTML entities fail in IE11 when MutationObservers are active #11781

Closed

petebacondarwin added this to the 1.4.x - jaracimrman-existence milestone May 5, 2015

petebacondarwin reviewed May 6, 2015
View reviewed changes

lgalfaso force-pushed the issue-11781 branch 2 times, most recently from b286e3e to cfef162 Compare May 10, 2015 20:26

Narretz assigned petebacondarwin Jun 3, 2015

petebacondarwin reviewed Jun 3, 2015
View reviewed changes

fix($compile): workaround for IE11 MutationObserver

063a333

IE11 MutationObserver breaks consecutive text nodes into several text nodes. This patch merges consecutive text nodes into a single node before looking for interpolations. Closes angular#11781

lgalfaso force-pushed the issue-11781 branch from cfef162 to 063a333 Compare June 6, 2015 09:25

lgalfaso closed this Jun 6, 2015

smdvdsn mentioned this pull request Aug 18, 2015

fix($compile): backport to 1.2 of workaround for IE11 MutationObserver #12613

Closed

gkalpak mentioned this pull request Jul 16, 2016

Interpolations in CJK and Cyrillic languages broken in IE11 #14924

Closed

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Aug 13, 2016

fix($compile): correctly merge consecutive text nodes on IE11

3ad0acd

Related to angular#11796. Fixes angular#14924 BREAKING CHANGE: // TODO

gkalpak mentioned this pull request Aug 15, 2016

fix($compile): correctly merge consecutive text nodes on IE11 #15025

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($compile): workaround for IE11 MutationObserver #11796

fix($compile): workaround for IE11 MutationObserver #11796

lgalfaso commented May 3, 2015

petebacondarwin commented May 5, 2015

lgalfaso commented May 5, 2015

lgalfaso commented May 5, 2015

petebacondarwin May 6, 2015

lgalfaso commented May 10, 2015

petebacondarwin Jun 3, 2015

gkalpak Jun 3, 2015

lgalfaso Jun 4, 2015

gkalpak Jun 4, 2015

lgalfaso Jun 4, 2015

gkalpak Jun 4, 2015

lgalfaso Jun 6, 2015

gkalpak Jun 9, 2015

petebacondarwin commented Jun 3, 2015

lgalfaso commented Jun 6, 2015

fix($compile): workaround for IE11 MutationObserver #11796

fix($compile): workaround for IE11 MutationObserver #11796

Conversation

lgalfaso commented May 3, 2015

petebacondarwin commented May 5, 2015

lgalfaso commented May 5, 2015

lgalfaso commented May 5, 2015

Choose a reason for hiding this comment

lgalfaso commented May 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Jun 3, 2015

lgalfaso commented Jun 6, 2015