fix(cdk/observers): don't observe content of comments

Author: mmalerba

src/cdk/observers/observe-content.spec.ts

@@ -1,11 +1,11 @@
 import {Component, ElementRef, ViewChild} from '@angular/core';
 import {
-  waitForAsync,
   ComponentFixture,
+  TestBed,
   fakeAsync,
   inject,
-  TestBed,
   tick,
+  waitForAsync,
 } from '@angular/core/testing';
 import {ContentObserver, MutationObserverFactory, ObserversModule} from './observe-content';
 
@@ -112,9 +112,9 @@ describe('Observe content directive', () => {
     }));
 
     it('should debounce the content changes', fakeAsync(() => {
-      invokeCallbacks();
-      invokeCallbacks();
-      invokeCallbacks();
+      invokeCallbacks([{type: 'fake'}]);
+      invokeCallbacks([{type: 'fake'}]);
+      invokeCallbacks([{type: 'fake'}]);
 
       tick(500);
       expect(fixture.componentInstance.spy).toHaveBeenCalledTimes(1);
@@ -166,7 +166,7 @@ describe('ContentObserver injectable', () => {
       expect(spy).not.toHaveBeenCalled();
 
       fixture.componentInstance.text = 'text';
-      invokeCallbacks();
+      invokeCallbacks([{type: 'fake'}]);
 
       expect(spy).toHaveBeenCalled();
     }));
@@ -186,19 +186,108 @@ describe('ContentObserver injectable', () => {
         expect(mof.create).toHaveBeenCalledTimes(1);
 
         fixture.componentInstance.text = 'text';
-        invokeCallbacks();
+        invokeCallbacks([{type: 'fake'}]);
 
         expect(spy).toHaveBeenCalledTimes(2);
 
         spy.calls.reset();
         sub1.unsubscribe();
         fixture.componentInstance.text = 'text text';
-        invokeCallbacks();
+        invokeCallbacks([{type: 'fake'}]);
 
         expect(spy).toHaveBeenCalledTimes(1);
       }),
     ));
   });
+
+  describe('real behavior', () => {
+    beforeEach(waitForAsync(() => {
+      TestBed.configureTestingModule({
+        imports: [ObserversModule, UnobservedComponentWithTextContent],
+      });
+
+      TestBed.compileComponents();
+    }));
+
+    it('should ignore addition or removal of comments', waitForAsync(
+      inject([ContentObserver], async (contentObserver: ContentObserver) => {
+        const spy = jasmine.createSpy('content observer');
+        const comment = document.createComment('cool');
+
+        const fixture = TestBed.createComponent(UnobservedComponentWithTextContent);
+        fixture.autoDetectChanges();
+        contentObserver.observe(fixture.componentInstance.contentEl).subscribe(spy);
+        await new Promise(r => setTimeout(r));
+        spy.calls.reset();
+
+        fixture.componentInstance.contentEl.nativeElement.appendChild(comment);
+        await new Promise(r => setTimeout(r));
+        expect(spy).not.toHaveBeenCalled();
+
+        fixture.componentInstance.contentEl.nativeElement.remove(comment);
+        await new Promise(r => setTimeout(r));
+        expect(spy).not.toHaveBeenCalled();
+      }),
+    ));
+
+    it('should not ignore addition or removal of text', waitForAsync(
+      inject([ContentObserver], async (contentObserver: ContentObserver) => {
+        const spy = jasmine.createSpy('content observer');
+        const text = document.createTextNode('cool');
+
+        const fixture = TestBed.createComponent(UnobservedComponentWithTextContent);
+        fixture.autoDetectChanges();
+        contentObserver.observe(fixture.componentInstance.contentEl).subscribe(spy);
+        await new Promise(r => setTimeout(r));
+
+        spy.calls.reset();
+        fixture.componentInstance.contentEl.nativeElement.appendChild(text);
+        await new Promise(r => setTimeout(r));
+        expect(spy).toHaveBeenCalled();
+
+        spy.calls.reset();
+        fixture.componentInstance.contentEl.nativeElement.remove(text);
+        await new Promise(r => setTimeout(r));
+        expect(spy).toHaveBeenCalled();
+      }),
+    ));
+
+    it('should ignore comment content change', waitForAsync(
+      inject([ContentObserver], async (contentObserver: ContentObserver) => {
+        const spy = jasmine.createSpy('content observer');
+        const comment = document.createComment('cool');
+
+        const fixture = TestBed.createComponent(UnobservedComponentWithTextContent);
+        fixture.autoDetectChanges();
+        contentObserver.observe(fixture.componentInstance.contentEl).subscribe(spy);
+        fixture.componentInstance.contentEl.nativeElement.appendChild(comment);
+        await new Promise(r => setTimeout(r));
+
+        spy.calls.reset();
+        comment.textContent = 'beans';
+        await new Promise(r => setTimeout(r));
+        expect(spy).not.toHaveBeenCalled();
+      }),
+    ));
+
+    it('should not ignore text content change', waitForAsync(
+      inject([ContentObserver], async (contentObserver: ContentObserver) => {
+        const spy = jasmine.createSpy('content observer');
+        const text = document.createTextNode('cool');
+
+        const fixture = TestBed.createComponent(UnobservedComponentWithTextContent);
+        fixture.autoDetectChanges();
+        contentObserver.observe(fixture.componentInstance.contentEl).subscribe(spy);
+        fixture.componentInstance.contentEl.nativeElement.appendChild(text);
+        await new Promise(r => setTimeout(r));
+
+        spy.calls.reset();
+        text.textContent = 'beans';
+        await new Promise(r => setTimeout(r));
+        expect(spy).toHaveBeenCalled();
+      }),
+    ));
+  });
 });
 
 @Component({

src/cdk/observers/observe-content.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {coerceNumberProperty, coerceElement, NumberInput} from '@angular/cdk/coercion';
+import {NumberInput, coerceElement, coerceNumberProperty} from '@angular/cdk/coercion';
 import {
   AfterContentInit,
   Directive,
@@ -20,8 +20,31 @@ import {
   Output,
   booleanAttribute,
 } from '@angular/core';
-import {Observable, Subject, Subscription, Observer} from 'rxjs';
-import {debounceTime} from 'rxjs/operators';
+import {Observable, Observer, Subject, Subscription} from 'rxjs';
+import {debounceTime, filter, map} from 'rxjs/operators';
+
+function shouldIgnoreRecord(record: MutationRecord) {
+  // Ignore changes to comment text.
+  if (record.type === 'characterData' && record.target instanceof Comment) {
+    return true;
+  }
+  // Ignore addition / removal of comments.
+  if (record.type === 'childList') {
+    for (let i = 0; i < record.addedNodes.length; i++) {
+      if (!(record.addedNodes[i] instanceof Comment)) {
+        return false;
+      }
+    }
+    for (let i = 0; i < record.removedNodes.length; i++) {
+      if (!(record.removedNodes[i] instanceof Comment)) {
+        return false;
+      }
+    }
+    return true;
+  }
+  // Observe everything else.
+  return false;
+}
 
 /**
  * Factory that creates a new MutationObserver and allows us to stub it out in unit tests.
@@ -70,7 +93,12 @@ export class ContentObserver implements OnDestroy {
 
     return new Observable((observer: Observer<MutationRecord[]>) => {
       const stream = this._observeElement(element);
-      const subscription = stream.subscribe(observer);
+      const subscription = stream
+        .pipe(
+          map(records => records.filter(record => !shouldIgnoreRecord(record))),
+          filter(records => !!records.length),
+        )
+        .subscribe(observer);
 
       return () => {
         subscription.unsubscribe();