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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1540,6 +1540,13 @@ 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 */
Expand Down
17 changes: 17 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really be modifying the window object here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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>&mdash; {{ "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();
}));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

if (!window.MutationObserver) {
return;
}
new window.MutationObserver(function() {}).observe(document.body, {
childList: true,
subtree: true
});
};
var base = jqLite('<div>&mdash; {{ "This doesn\'t." }}</div>');
element = $compile(base)($rootScope);
$rootScope.$digest();
expect(element.text()).toBe("— This doesn't.");
}));


it('should support custom start/end interpolation symbols in template and directive template',
function() {
module(function($interpolateProvider, $compileProvider) {
Expand Down