From 1c9cabf2fd6429ec0c310895d1026854f558ae52 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Mon, 8 Aug 2016 13:48:11 +0300 Subject: [PATCH 1/2] fix($compile): correctly merge consecutive text nodes on IE11 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 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 #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: '' transclude: { ignored: 'veryImportantContent' } }) ``` ```html {{ Nooot 'foo'}} {{ <-- Two separate 'foo'}} <-- text nodes foo <-- The text nodes were merged into `{{'foo'}}`, which was then interpolated {{ Nooot 'foo'}} {{ <-- Two separate 'foo'}} <-- nodes ``` --- src/ng/compile.js | 45 +++++++++++++++++------ test/ng/compileSpec.js | 83 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index f5b0d450edea..0274ab8667fc 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1842,12 +1842,40 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, ignoreDirective, previousCompileContext) { var linkFns = [], - attrs, directives, nodeLinkFn, childNodes, childLinkFn, linkFnFound, nodeLinkFnFound; + // `nodeList` can be either an element's `.childNodes` (live NodeList) + // or a jqLite/jQuery collection or an array + notLiveList = isArray(nodeList) || (nodeList instanceof jqLite), + attrs, directives, node, nodeLinkFn, childNodes, childLinkFn, linkFnFound, nodeLinkFnFound; + for (var i = 0; i < nodeList.length; i++) { attrs = new Attributes(); + node = nodeList[i]; + + // Workaround for #11781 and #14924 + if ((msie === 11) && (node.nodeType === NODE_TYPE_TEXT)) { + var parent = node.parentNode; + var sibling; + + while (true) { + sibling = parent ? node.nextSibling : nodeList[i + 1]; + if (!sibling || sibling.nodeType !== NODE_TYPE_TEXT) { + break; + } - // we must always refer to nodeList[i] since the nodes can be replaced underneath us. + node.nodeValue = node.nodeValue + sibling.nodeValue; + + if (sibling.parentNode) { + sibling.parentNode.removeChild(sibling); + } + if (notLiveList && sibling === nodeList[i + 1]) { + nodeList.splice(i + 1, 1); + } + } + } + + // We must always refer to `nodeList[i]` hereafter, + // since the nodes can be replaced underneath us. directives = collectDirectives(nodeList[i], [], attrs, i === 0 ? maxPriority : undefined, ignoreDirective); @@ -2046,13 +2074,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } break; case NODE_TYPE_TEXT: /* Text Node */ - if (msie === 11) { - // Workaround for #11781 - while (node.parentNode && node.nextSibling && node.nextSibling.nodeType === NODE_TYPE_TEXT) { - node.nodeValue = node.nodeValue + node.nextSibling.nodeValue; - node.parentNode.removeChild(node.nextSibling); - } - } addTextInterpolateDirective(directives, node.nodeValue); break; case NODE_TYPE_COMMENT: /* Comment */ @@ -2325,9 +2346,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { var slots = createMap(); - $template = jqLite(jqLiteClone(compileNode)).contents(); - - if (isObject(directiveValue)) { + if (!isObject(directiveValue)) { + $template = jqLite(jqLiteClone(compileNode)).contents(); + } else { // We have transclusion slots, // collect them up, compile them and store their transclusion functions diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index bfed32aed046..77cfca8f8ff1 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -3260,6 +3260,26 @@ describe('$compile', function() { })); + it('should not process text nodes merged into their sibling', inject(function($compile, $rootScope) { + var div = document.createElement('div'); + div.appendChild(document.createTextNode('1{{ value }}')); + div.appendChild(document.createTextNode('2{{ value }}')); + div.appendChild(document.createTextNode('3{{ value }}')); + + element = jqLite(div.childNodes); + + var initialWatcherCount = $rootScope.$countWatchers(); + $compile(element)($rootScope); + $rootScope.$apply('value = 0'); + var newWatcherCount = $rootScope.$countWatchers() - initialWatcherCount; + + expect(element.text()).toBe('102030'); + expect(newWatcherCount).toBe(3); + + dealoc(div); + })); + + it('should support custom start/end interpolation symbols in template and directive template', function() { module(function($interpolateProvider, $compileProvider) { @@ -8530,10 +8550,38 @@ describe('$compile', function() { element = $compile('
')($rootScope); expect(capturedChildCtrl).toBeTruthy(); }); - }); + // See issue https://github.com/angular/angular.js/issues/14924 + it('should not process top-level transcluded text nodes merged into their sibling', + function() { + module(function() { + directive('transclude', valueFn({ + template: '', + transclude: true, + scope: {} + })); + }); + + inject(function($compile) { + element = jqLite('
'); + element[0].appendChild(document.createTextNode('1{{ value }}')); + element[0].appendChild(document.createTextNode('2{{ value }}')); + element[0].appendChild(document.createTextNode('3{{ value }}')); + + var initialWatcherCount = $rootScope.$countWatchers(); + $compile(element)($rootScope); + $rootScope.$apply('value = 0'); + var newWatcherCount = $rootScope.$countWatchers() - initialWatcherCount; + + expect(element.text()).toBe('102030'); + expect(newWatcherCount).toBe(3); + }); + } + ); + + // see issue https://github.com/angular/angular.js/issues/9413 describe('passing a parent bound transclude function to the link ' + 'function returned from `$compile`', function() { @@ -9996,6 +10044,39 @@ describe('$compile', function() { expect(element.children().eq(2).text()).toEqual('dorothy'); }); }); + + + // See issue https://github.com/angular/angular.js/issues/14924 + it('should not process top-level transcluded text nodes merged into their sibling', + function() { + module(function() { + directive('transclude', valueFn({ + template: '', + transclude: {}, + scope: {} + })); + }); + + inject(function($compile) { + element = jqLite('
'); + element[0].appendChild(document.createTextNode('1{{ value }}')); + element[0].appendChild(document.createTextNode('2{{ value }}')); + element[0].appendChild(document.createTextNode('3{{ value }}')); + + var initialWatcherCount = $rootScope.$countWatchers(); + $compile(element)($rootScope); + $rootScope.$apply('value = 0'); + var newWatcherCount = $rootScope.$countWatchers() - initialWatcherCount; + + expect(element.text()).toBe('102030'); + expect(newWatcherCount).toBe(3); + + if (msie === 11) { + expect(element.find('ng-transclude').contents().length).toBe(1); + } + }); + } + ); }); From 77172ac41df6ec302e326abe4a609150e736d6a8 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Wed, 17 Aug 2016 17:56:15 +0300 Subject: [PATCH 2/2] fixup! fix($compile): correctly merge consecutive text nodes on IE11 --- src/ng/compile.js | 50 +++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 0274ab8667fc..74c14752a319 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1845,33 +1845,15 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // `nodeList` can be either an element's `.childNodes` (live NodeList) // or a jqLite/jQuery collection or an array notLiveList = isArray(nodeList) || (nodeList instanceof jqLite), - attrs, directives, node, nodeLinkFn, childNodes, childLinkFn, linkFnFound, nodeLinkFnFound; + attrs, directives, nodeLinkFn, childNodes, childLinkFn, linkFnFound, nodeLinkFnFound; for (var i = 0; i < nodeList.length; i++) { attrs = new Attributes(); - node = nodeList[i]; // Workaround for #11781 and #14924 - if ((msie === 11) && (node.nodeType === NODE_TYPE_TEXT)) { - var parent = node.parentNode; - var sibling; - - while (true) { - sibling = parent ? node.nextSibling : nodeList[i + 1]; - if (!sibling || sibling.nodeType !== NODE_TYPE_TEXT) { - break; - } - - node.nodeValue = node.nodeValue + sibling.nodeValue; - - if (sibling.parentNode) { - sibling.parentNode.removeChild(sibling); - } - if (notLiveList && sibling === nodeList[i + 1]) { - nodeList.splice(i + 1, 1); - } - } + if (msie === 11) { + mergeConsecutiveTextNodes(nodeList, i, notLiveList); } // We must always refer to `nodeList[i]` hereafter, @@ -1966,6 +1948,32 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } } + function mergeConsecutiveTextNodes(nodeList, idx, notLiveList) { + var node = nodeList[idx]; + var parent = node.parentNode; + var sibling; + + if (node.nodeType !== NODE_TYPE_TEXT) { + return; + } + + while (true) { + sibling = parent ? node.nextSibling : nodeList[idx + 1]; + if (!sibling || sibling.nodeType !== NODE_TYPE_TEXT) { + break; + } + + node.nodeValue = node.nodeValue + sibling.nodeValue; + + if (sibling.parentNode) { + sibling.parentNode.removeChild(sibling); + } + if (notLiveList && sibling === nodeList[idx + 1]) { + nodeList.splice(idx + 1, 1); + } + } + } + function createBoundTranscludeFn(scope, transcludeFn, previousBoundTranscludeFn) { function boundTranscludeFn(transcludedScope, cloneFn, controllers, futureParentElement, containingScope) {