From 94e2ea7361554f258c72ecff2254b0fb9c582afa Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 27 Nov 2017 23:06:09 +0200 Subject: [PATCH] fix(aio): fix embedded ToC and improve ToC, destroying components and scroll timing (#18428) - Fix embedded ToC: Previously, the element was added too late and was never instantiated. - Improve ToC update timing: Previously, the ToC was updated after the entering animation was over, which resulted in the ToC being outdated for the duration of the animation. - Improve destroying components timing: Previously, the old embedded components were destroyed as soon as a new document was requested. Even if the transition ended up never happening (e.g. due to error while preparing the new document), the embedded components would have been destroyed and the displayed document would not work as expected. Now the old embedded components are destroyed only after the new document has been fully prepared. - Improve scroll-to-top timing: Previously, the page was scrolled to top after the entering animation was over, which resulted in "jumpi-ness". Now the scrolling happens after the leaving document has been removed and before the entering document has been inserted. PR Close #18428 --- aio/src/app/app.component.html | 2 +- aio/src/app/app.component.spec.ts | 63 +- aio/src/app/app.component.ts | 21 +- .../doc-viewer/doc-viewer.component.spec.ts | 581 +++++++++++------- .../layout/doc-viewer/doc-viewer.component.ts | 98 ++- aio/src/testing/doc-viewer-utils.ts | 4 +- 6 files changed, 483 insertions(+), 286 deletions(-) diff --git a/aio/src/app/app.component.html b/aio/src/app/app.component.html index 59dcf44a07..9101510190 100644 --- a/aio/src/app/app.component.html +++ b/aio/src/app/app.component.html @@ -28,7 +28,7 @@
- +
diff --git a/aio/src/app/app.component.spec.ts b/aio/src/app/app.component.spec.ts index b5addb5858..a2697c3eb5 100644 --- a/aio/src/app/app.component.spec.ts +++ b/aio/src/app/app.component.spec.ts @@ -252,25 +252,25 @@ describe('AppComponent', () => { const sidenavBackdrop = fixture.debugElement.query(By.css('.mat-drawer-backdrop')).nativeElement; sidenavBackdrop.click(); fixture.detectChanges(); - expect(sidenav.opened).toBe(false); + expect(sidenav.opened).toBe(false); }); it('should close when nav to another guide page', () => { locationService.go('guide/bags'); fixture.detectChanges(); - expect(sidenav.opened).toBe(false); + expect(sidenav.opened).toBe(false); }); it('should close when nav to api page', () => { locationService.go('api'); fixture.detectChanges(); - expect(sidenav.opened).toBe(false); + expect(sidenav.opened).toBe(false); }); it('should close again when nav to market page', () => { locationService.go('features'); fixture.detectChanges(); - expect(sidenav.opened).toBe(false); + expect(sidenav.opened).toBe(false); }); }); @@ -452,16 +452,19 @@ describe('AppComponent', () => { const scrollDelay = 500; let scrollService: ScrollService; let scrollSpy: jasmine.Spy; + let scrollToTopSpy: jasmine.Spy; beforeEach(() => { scrollService = fixture.debugElement.injector.get(ScrollService); scrollSpy = spyOn(scrollService, 'scroll'); + scrollToTopSpy = spyOn(scrollService, 'scrollToTop'); }); it('should not scroll immediately when the docId (path) changes', () => { locationService.go('guide/pipes'); - // deliberately not calling `fixture.detectChanges` because don't want `onDocRendered` + // deliberately not calling `fixture.detectChanges` because don't want `onDocInserted` expect(scrollSpy).not.toHaveBeenCalled(); + expect(scrollToTopSpy).not.toHaveBeenCalled(); }); it('should scroll when just the hash changes (# alone)', () => { @@ -491,7 +494,7 @@ describe('AppComponent', () => { expect(scrollSpy).toHaveBeenCalledTimes(1); }); - it('should scroll when e-nav to the empty path', () => { + it('should scroll when re-nav to the empty path', () => { locationService.go(''); scrollSpy.calls.reset(); @@ -499,17 +502,29 @@ describe('AppComponent', () => { expect(scrollSpy).toHaveBeenCalledTimes(1); }); - it('should scroll after a delay when call onDocRendered directly', fakeAsync(() => { - component.onDocRendered(); + it('should scroll to top when call `onDocRemoved` directly', () => { + scrollToTopSpy.calls.reset(); + + component.onDocRemoved(); + expect(scrollToTopSpy).toHaveBeenCalled(); + }); + + it('should scroll after a delay when call `onDocInserted` directly', fakeAsync(() => { + component.onDocInserted(); expect(scrollSpy).not.toHaveBeenCalled(); + tick(scrollDelay); expect(scrollSpy).toHaveBeenCalled(); })); - it('should scroll (via onDocRendered) when finish navigating to a new doc', fakeAsync(() => { + it('should scroll (via `onDocInserted`) when finish navigating to a new doc', fakeAsync(() => { + expect(scrollToTopSpy).not.toHaveBeenCalled(); + locationService.go('guide/pipes'); - fixture.detectChanges(); // triggers the event that calls onDocRendered + fixture.detectChanges(); // triggers the event that calls `onDocInserted` + expect(scrollToTopSpy).toHaveBeenCalled(); expect(scrollSpy).not.toHaveBeenCalled(); + tick(scrollDelay); expect(scrollSpy).toHaveBeenCalled(); })); @@ -872,7 +887,7 @@ describe('AppComponent', () => { describe('with mocked DocViewer', () => { const getDocViewer = () => fixture.debugElement.query(By.css('aio-doc-viewer')); - const triggerDocRendered = () => getDocViewer().triggerEventHandler('docRendered', {}); + const triggerDocReady = () => getDocViewer().triggerEventHandler('docReady', undefined); beforeEach(() => { createTestingModule('a/b'); @@ -884,7 +899,7 @@ describe('AppComponent', () => { }); describe('initial rendering', () => { - it('should initially add the starting class until the first document is rendered', fakeAsync(() => { + it('should initially add the starting class until the first document is ready', fakeAsync(() => { const getSidenavContainer = () => fixture.debugElement.query(By.css('mat-sidenav-container')); initializeTest(); @@ -892,7 +907,7 @@ describe('AppComponent', () => { expect(component.isStarting).toBe(true); expect(getSidenavContainer().classes['starting']).toBe(true); - triggerDocRendered(); + triggerDocReady(); fixture.detectChanges(); expect(component.isStarting).toBe(true); expect(getSidenavContainer().classes['starting']).toBe(true); @@ -915,7 +930,7 @@ describe('AppComponent', () => { const getProgressBar = () => fixture.debugElement.query(By.directive(MatProgressBar)); const initializeAndCompleteNavigation = () => { initializeTest(); - triggerDocRendered(); + triggerDocReady(); tick(HIDE_DELAY); }; @@ -952,7 +967,7 @@ describe('AppComponent', () => { it('should not be shown when re-navigating to the empty path', fakeAsync(() => { initializeAndCompleteNavigation(); locationService.urlSubject.next(''); - triggerDocRendered(); + triggerDocReady(); locationService.urlSubject.next(''); @@ -963,12 +978,12 @@ describe('AppComponent', () => { tick(HIDE_DELAY); // Fire the remaining timer or `fakeAsync()` complains. })); - it('should not be shown if the doc is rendered quickly', fakeAsync(() => { + it('should not be shown if the doc is prepared quickly', fakeAsync(() => { initializeAndCompleteNavigation(); locationService.urlSubject.next('c/d'); tick(SHOW_DELAY - 1); - triggerDocRendered(); + triggerDocReady(); tick(1); fixture.detectChanges(); @@ -977,12 +992,12 @@ describe('AppComponent', () => { tick(HIDE_DELAY); // Fire the remaining timer or `fakeAsync()` complains. })); - it('should be shown if rendering the doc takes too long', fakeAsync(() => { + it('should be shown if preparing the doc takes too long', fakeAsync(() => { initializeAndCompleteNavigation(); locationService.urlSubject.next('c/d'); tick(SHOW_DELAY); - triggerDocRendered(); + triggerDocReady(); fixture.detectChanges(); expect(getProgressBar()).toBeTruthy(); @@ -990,12 +1005,12 @@ describe('AppComponent', () => { tick(HIDE_DELAY); // Fire the remaining timer or `fakeAsync()` complains. })); - it('should be hidden (after a delay) once the doc is rendered', fakeAsync(() => { + it('should be hidden (after a delay) once the doc has been prepared', fakeAsync(() => { initializeAndCompleteNavigation(); locationService.urlSubject.next('c/d'); tick(SHOW_DELAY); - triggerDocRendered(); + triggerDocReady(); fixture.detectChanges(); expect(getProgressBar()).toBeTruthy(); @@ -1012,10 +1027,10 @@ describe('AppComponent', () => { it('should only take the latest request into account', fakeAsync(() => { initializeAndCompleteNavigation(); locationService.urlSubject.next('c/d'); // The URL changes. - locationService.urlSubject.next('e/f'); // The URL changes again before `onDocRendered()`. + locationService.urlSubject.next('e/f'); // The URL changes again before `onDocReady()`. - tick(SHOW_DELAY - 1); // `onDocRendered()` is triggered (for the last doc), - triggerDocRendered(); // before the progress bar is shown. + tick(SHOW_DELAY - 1); // `onDocReady()` is triggered (for the last doc), + triggerDocReady(); // before the progress bar is shown. tick(1); fixture.detectChanges(); diff --git a/aio/src/app/app.component.ts b/aio/src/app/app.component.ts index d954fd5183..2d2be1011b 100644 --- a/aio/src/app/app.component.ts +++ b/aio/src/app/app.component.ts @@ -203,22 +203,29 @@ export class AppComponent implements OnInit { this.scrollService.scroll(); } - onDocRendered() { + onDocReady() { // Stop fetching timeout (which, when render is fast, means progress bar never shown) clearTimeout(this.isFetchingTimeout); - // Put page in a clean visual state - this.scrollService.scrollToTop(); - - // Scroll 500ms after the doc-viewer has finished rendering the new doc - // The delay is to allow time for async layout to complete + // If progress bar has been shown, keep it for at least 500ms (to avoid flashing). setTimeout(() => { - this.autoScroll(); this.isStarting = false; this.isFetching = false; }, 500); } + onDocRemoved() { + // The previous document has been removed. + // Scroll to top to restore a clean visual state for the new document. + this.scrollService.scrollToTop(); + } + + onDocInserted() { + // Scroll 500ms after the new document has been inserted into the doc-viewer. + // The delay is to allow time for async layout to complete. + setTimeout(() => this.autoScroll(), 500); + } + onDocVersionChange(versionIndex: number) { const version = this.docVersions[versionIndex]; if (version.url) { diff --git a/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts b/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts index 609d3b4334..cf9d50bed9 100644 --- a/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts +++ b/aio/src/app/layout/doc-viewer/doc-viewer.component.spec.ts @@ -40,8 +40,7 @@ describe('DocViewerComponent', () => { expect(docViewer).toEqual(jasmine.any(DocViewerComponent)); }); - describe('#doc / #docRendered', () => { - let destroyEmbeddedComponentsSpy: jasmine.Spy; + describe('#doc', () => { let renderSpy: jasmine.Spy; const setCurrentDoc = (contents, id = 'fizz/buzz') => { @@ -49,10 +48,7 @@ describe('DocViewerComponent', () => { parentFixture.detectChanges(); }; - beforeEach(() => { - destroyEmbeddedComponentsSpy = spyOn(docViewer, 'destroyEmbeddedComponents'); - renderSpy = spyOn(docViewer, 'render').and.returnValue([null]); - }); + beforeEach(() => renderSpy = spyOn(docViewer, 'render').and.returnValue([null])); it('should render the new document', () => { setCurrentDoc('foo', 'bar'); @@ -64,30 +60,6 @@ describe('DocViewerComponent', () => { expect(renderSpy.calls.mostRecent().args).toEqual([{id: 'baz', contents: null}]); }); - it('should destroy the currently active components (before rendering the new document)', () => { - setCurrentDoc('foo'); - expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(1); - expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledBefore(renderSpy); - - destroyEmbeddedComponentsSpy.calls.reset(); - renderSpy.calls.reset(); - - setCurrentDoc(null); - expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(1); - expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledBefore(renderSpy); - }); - - it('should emit `docRendered` after the new document has been rendered', done => { - let completeRender: () => void; - renderSpy.and.returnValue(new Promise(resolve => completeRender = resolve)); - docViewer.docRendered.subscribe(done); - - setCurrentDoc('foo'); - expect(renderSpy).toHaveBeenCalledTimes(1); - - completeRender(); - }); - it('should unsubscribe from the previous "render" observable upon new document', () => { const obs = new ObservableWithSubscriptionSpies(); renderSpy.and.returnValue(obs); @@ -102,22 +74,15 @@ describe('DocViewerComponent', () => { }); it('should ignore falsy document values', () => { - const onDocRenderedSpy = jasmine.createSpy('onDocRendered'); - docViewer.docRendered.subscribe(onDocRenderedSpy); - parentComponent.currentDoc = null; parentFixture.detectChanges(); - expect(destroyEmbeddedComponentsSpy).not.toHaveBeenCalled(); expect(renderSpy).not.toHaveBeenCalled(); - expect(onDocRenderedSpy).not.toHaveBeenCalled(); parentComponent.currentDoc = undefined; parentFixture.detectChanges(); - expect(destroyEmbeddedComponentsSpy).not.toHaveBeenCalled(); expect(renderSpy).not.toHaveBeenCalled(); - expect(onDocRenderedSpy).not.toHaveBeenCalled(); }); }); @@ -160,166 +125,20 @@ describe('DocViewerComponent', () => { }); it('should stop responding to document changes', () => { - const destroyEmbeddedComponentsSpy = spyOn(docViewer, 'destroyEmbeddedComponents'); const renderSpy = spyOn(docViewer, 'render').and.returnValue([undefined]); - const onDocRenderedSpy = jasmine.createSpy('onDocRendered'); - docViewer.docRendered.subscribe(onDocRenderedSpy); - expect(destroyEmbeddedComponentsSpy).not.toHaveBeenCalled(); expect(renderSpy).not.toHaveBeenCalled(); - expect(onDocRenderedSpy).not.toHaveBeenCalled(); docViewer.doc = {contents: 'Some content', id: 'some-id'}; - expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(1); expect(renderSpy).toHaveBeenCalledTimes(1); - expect(onDocRenderedSpy).toHaveBeenCalledTimes(1); - docViewer.ngOnDestroy(); // Also calls `destroyEmbeddedComponents()`. + docViewer.ngOnDestroy(); docViewer.doc = {contents: 'Other content', id: 'other-id'}; - expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(2); expect(renderSpy).toHaveBeenCalledTimes(1); - expect(onDocRenderedSpy).toHaveBeenCalledTimes(1); docViewer.doc = {contents: 'More content', id: 'more-id'}; - expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(2); expect(renderSpy).toHaveBeenCalledTimes(1); - expect(onDocRenderedSpy).toHaveBeenCalledTimes(1); - }); - }); - - describe('#addTitleAndToc()', () => { - const EMPTY_DOC = ''; - const DOC_WITHOUT_H1 = 'Some content'; - const DOC_WITH_H1 = '

Features

Some content'; - const DOC_WITH_NO_TOC_H1 = '

Features

Some content'; - const DOC_WITH_HIDDEN_H1_CONTENT = '

linkFeatures

Some content'; - - const tryDoc = (contents: string, docId = '') => { - docViewer.currViewContainer.innerHTML = contents; - docViewer.addTitleAndToc(docId); - }; - - describe('(title)', () => { - let titleService: MockTitle; - - beforeEach(() => titleService = TestBed.get(Title)); - - it('should set the title if there is an `

` heading', () => { - tryDoc(DOC_WITH_H1); - expect(titleService.setTitle).toHaveBeenCalledWith('Angular - Features'); - }); - - it('should set the title if there is a `.no-toc` `

` heading', () => { - tryDoc(DOC_WITH_NO_TOC_H1); - expect(titleService.setTitle).toHaveBeenCalledWith('Angular - Features'); - }); - - it('should set the default title if there is no `

` heading', () => { - tryDoc(DOC_WITHOUT_H1); - expect(titleService.setTitle).toHaveBeenCalledWith('Angular'); - - tryDoc(EMPTY_DOC); - expect(titleService.setTitle).toHaveBeenCalledWith('Angular'); - }); - - it('should not include hidden content of the `

` heading in the title', () => { - tryDoc(DOC_WITH_HIDDEN_H1_CONTENT); - expect(titleService.setTitle).toHaveBeenCalledWith('Angular - Features'); - }); - - it('should fall back to `textContent` if `innerText` is not available', () => { - const viewContainer = docViewer.currViewContainer; - const querySelector_ = viewContainer.querySelector; - spyOn(viewContainer, 'querySelector').and.callFake((selector: string) => { - const elem = querySelector_.call(viewContainer, selector); - return Object.defineProperties(elem, { - innerText: {value: undefined}, - textContent: {value: 'Text Content'}, - }); - }); - - tryDoc(DOC_WITH_HIDDEN_H1_CONTENT); - - expect(titleService.setTitle).toHaveBeenCalledWith('Angular - Text Content'); - }); - - it('should still use `innerText` if available but empty', () => { - const viewContainer = docViewer.currViewContainer; - const querySelector_ = viewContainer.querySelector; - spyOn(viewContainer, 'querySelector').and.callFake((selector: string) => { - const elem = querySelector_.call(viewContainer, selector); - return Object.defineProperties(elem, { - innerText: { value: '' }, - textContent: { value: 'Text Content' } - }); - }); - - tryDoc(DOC_WITH_HIDDEN_H1_CONTENT); - - expect(titleService.setTitle).toHaveBeenCalledWith('Angular'); - }); - }); - - describe('(ToC)', () => { - let tocService: MockTocService; - - const getTocEl = () => docViewerEl.querySelector('aio-toc'); - - beforeEach(() => tocService = TestBed.get(TocService)); - - it('should have an (embedded) ToC if there is an `

` heading', () => { - tryDoc(DOC_WITH_H1, 'foo'); - const tocEl = getTocEl()!; - - expect(tocEl).toBeTruthy(); - expect(tocEl.classList.contains('embedded')).toBe(true); - expect(tocService.genToc).toHaveBeenCalledTimes(1); - expect(tocService.genToc).toHaveBeenCalledWith(docViewer.currViewContainer, 'foo'); - }); - - it('should have no ToC if there is a `.no-toc` `

` heading', () => { - tryDoc(DOC_WITH_NO_TOC_H1); - - expect(getTocEl()).toBeFalsy(); - expect(tocService.genToc).not.toHaveBeenCalled(); - }); - - it('should have no ToC if there is no `

` heading', () => { - tryDoc(DOC_WITHOUT_H1); - expect(getTocEl()).toBeFalsy(); - - tryDoc(EMPTY_DOC); - expect(getTocEl()).toBeFalsy(); - - expect(tocService.genToc).not.toHaveBeenCalled(); - }); - - it('should always reset the ToC (before generating the new one)', () => { - expect(tocService.reset).not.toHaveBeenCalled(); - expect(tocService.genToc).not.toHaveBeenCalled(); - - tocService.genToc.calls.reset(); - tryDoc(DOC_WITH_H1, 'foo'); - expect(tocService.reset).toHaveBeenCalledTimes(1); - expect(tocService.reset).toHaveBeenCalledBefore(tocService.genToc); - expect(tocService.genToc).toHaveBeenCalledWith(docViewer.currViewContainer, 'foo'); - - tocService.genToc.calls.reset(); - tryDoc(DOC_WITH_NO_TOC_H1, 'bar'); - expect(tocService.reset).toHaveBeenCalledTimes(2); - expect(tocService.genToc).not.toHaveBeenCalled(); - - tocService.genToc.calls.reset(); - tryDoc(DOC_WITHOUT_H1, 'baz'); - expect(tocService.reset).toHaveBeenCalledTimes(3); - expect(tocService.genToc).not.toHaveBeenCalled(); - - tocService.genToc.calls.reset(); - tryDoc(EMPTY_DOC, 'qux'); - expect(tocService.reset).toHaveBeenCalledTimes(4); - expect(tocService.genToc).not.toHaveBeenCalled(); - }); }); }); @@ -350,9 +169,174 @@ describe('DocViewerComponent', () => { }); }); + describe('#prepareTitleAndToc()', () => { + const EMPTY_DOC = ''; + const DOC_WITHOUT_H1 = 'Some content'; + const DOC_WITH_H1 = '

Features

Some content'; + const DOC_WITH_NO_TOC_H1 = '

Features

Some content'; + const DOC_WITH_HIDDEN_H1_CONTENT = '

linkFeatures

Some content'; + let titleService: MockTitle; + let tocService: MockTocService; + let targetEl: HTMLElement; + + const getTocEl = () => targetEl.querySelector('aio-toc'); + const doPrepareTitleAndToc = (contents: string, docId = '') => { + targetEl.innerHTML = contents; + return docViewer.prepareTitleAndToc(targetEl, docId); + }; + const doAddTitleAndToc = (contents: string, docId = '') => { + const addTitleAndToc = doPrepareTitleAndToc(contents, docId); + return addTitleAndToc(); + }; + + beforeEach(() => { + titleService = TestBed.get(Title); + tocService = TestBed.get(TocService); + + targetEl = document.createElement('div'); + document.body.appendChild(targetEl); // Required for `innerText` to work as expected. + }); + + afterEach(() => document.body.removeChild(targetEl)); + + it('should return a function for doing the actual work', () => { + const addTitleAndToc = doPrepareTitleAndToc(DOC_WITH_H1); + + expect(getTocEl()).toBeTruthy(); + expect(titleService.setTitle).not.toHaveBeenCalled(); + expect(tocService.reset).not.toHaveBeenCalled(); + expect(tocService.genToc).not.toHaveBeenCalled(); + + addTitleAndToc(); + + expect(titleService.setTitle).toHaveBeenCalledTimes(1); + expect(tocService.reset).toHaveBeenCalledTimes(1); + expect(tocService.genToc).toHaveBeenCalledTimes(1); + }); + + describe('(title)', () => { + it('should set the title if there is an `

` heading', () => { + doAddTitleAndToc(DOC_WITH_H1); + expect(titleService.setTitle).toHaveBeenCalledWith('Angular - Features'); + }); + + it('should set the title if there is a `.no-toc` `

` heading', () => { + doAddTitleAndToc(DOC_WITH_NO_TOC_H1); + expect(titleService.setTitle).toHaveBeenCalledWith('Angular - Features'); + }); + + it('should set the default title if there is no `

` heading', () => { + doAddTitleAndToc(DOC_WITHOUT_H1); + expect(titleService.setTitle).toHaveBeenCalledWith('Angular'); + + doAddTitleAndToc(EMPTY_DOC); + expect(titleService.setTitle).toHaveBeenCalledWith('Angular'); + }); + + it('should not include hidden content of the `

` heading in the title', () => { + doAddTitleAndToc(DOC_WITH_HIDDEN_H1_CONTENT); + expect(titleService.setTitle).toHaveBeenCalledWith('Angular - Features'); + }); + + it('should fall back to `textContent` if `innerText` is not available', () => { + const querySelector_ = targetEl.querySelector; + spyOn(targetEl, 'querySelector').and.callFake((selector: string) => { + const elem = querySelector_.call(targetEl, selector); + return Object.defineProperties(elem, { + innerText: {value: undefined}, + textContent: {value: 'Text Content'}, + }); + }); + + doAddTitleAndToc(DOC_WITH_HIDDEN_H1_CONTENT); + + expect(titleService.setTitle).toHaveBeenCalledWith('Angular - Text Content'); + }); + + it('should still use `innerText` if available but empty', () => { + const querySelector_ = targetEl.querySelector; + spyOn(targetEl, 'querySelector').and.callFake((selector: string) => { + const elem = querySelector_.call(targetEl, selector); + return Object.defineProperties(elem, { + innerText: { value: '' }, + textContent: { value: 'Text Content' } + }); + }); + + doAddTitleAndToc(DOC_WITH_HIDDEN_H1_CONTENT); + + expect(titleService.setTitle).toHaveBeenCalledWith('Angular'); + }); + }); + + describe('(ToC)', () => { + it('should add an embedded ToC element if there is an `

` heading', () => { + doPrepareTitleAndToc(DOC_WITH_H1); + const tocEl = getTocEl()!; + + expect(tocEl).toBeTruthy(); + expect(tocEl.classList.contains('embedded')).toBe(true); + }); + + it('should not add a ToC element if there is a `.no-toc` `

` heading', () => { + doPrepareTitleAndToc(DOC_WITH_NO_TOC_H1); + expect(getTocEl()).toBeFalsy(); + }); + + it('should not add a ToC element if there is no `

` heading', () => { + doPrepareTitleAndToc(DOC_WITHOUT_H1); + expect(getTocEl()).toBeFalsy(); + + doPrepareTitleAndToc(EMPTY_DOC); + expect(getTocEl()).toBeFalsy(); + }); + + it('should generate ToC entries if there is an `

` heading', () => { + doAddTitleAndToc(DOC_WITH_H1, 'foo'); + + expect(tocService.genToc).toHaveBeenCalledTimes(1); + expect(tocService.genToc).toHaveBeenCalledWith(targetEl, 'foo'); + }); + + it('should not generate ToC entries if there is a `.no-toc` `

` heading', () => { + doAddTitleAndToc(DOC_WITH_NO_TOC_H1); + expect(tocService.genToc).not.toHaveBeenCalled(); + }); + + it('should not generate ToC entries if there is no `

` heading', () => { + doAddTitleAndToc(DOC_WITHOUT_H1); + doAddTitleAndToc(EMPTY_DOC); + + expect(tocService.genToc).not.toHaveBeenCalled(); + }); + + it('should always reset the ToC (before generating the new one)', () => { + doAddTitleAndToc(DOC_WITH_H1, 'foo'); + expect(tocService.reset).toHaveBeenCalledTimes(1); + expect(tocService.reset).toHaveBeenCalledBefore(tocService.genToc); + expect(tocService.genToc).toHaveBeenCalledWith(targetEl, 'foo'); + + tocService.genToc.calls.reset(); + + doAddTitleAndToc(DOC_WITH_NO_TOC_H1, 'bar'); + expect(tocService.reset).toHaveBeenCalledTimes(2); + expect(tocService.genToc).not.toHaveBeenCalled(); + + doAddTitleAndToc(DOC_WITHOUT_H1, 'baz'); + expect(tocService.reset).toHaveBeenCalledTimes(3); + expect(tocService.genToc).not.toHaveBeenCalled(); + + doAddTitleAndToc(EMPTY_DOC, 'qux'); + expect(tocService.reset).toHaveBeenCalledTimes(4); + expect(tocService.genToc).not.toHaveBeenCalled(); + }); + }); + }); + describe('#render()', () => { - let addTitleAndTocSpy: jasmine.Spy; + let destroyEmbeddedComponentsSpy: jasmine.Spy; let embedIntoSpy: jasmine.Spy; + let prepareTitleAndTocSpy: jasmine.Spy; let swapViewsSpy: jasmine.Spy; const doRender = (contents: string | null, id = 'foo') => @@ -362,8 +346,9 @@ describe('DocViewerComponent', () => { beforeEach(() => { const embedComponentsService = TestBed.get(EmbedComponentsService) as MockEmbedComponentsService; - addTitleAndTocSpy = spyOn(docViewer, 'addTitleAndToc'); + destroyEmbeddedComponentsSpy = spyOn(docViewer, 'destroyEmbeddedComponents'); embedIntoSpy = embedComponentsService.embedInto.and.returnValue(of([])); + prepareTitleAndTocSpy = spyOn(docViewer, 'prepareTitleAndToc'); swapViewsSpy = spyOn(docViewer, 'swapViews').and.returnValue(of(undefined)); }); @@ -396,26 +381,37 @@ describe('DocViewerComponent', () => { expect(docViewerEl.textContent).toBe(''); }); + it('should prepare the title and ToC (before embedding components)', async () => { + prepareTitleAndTocSpy.and.callFake((targetEl: HTMLElement, docId: string) => { + expect(targetEl.innerHTML).toBe('Some content'); + expect(docId).toBe('foo'); + }); + + await doRender('Some content', 'foo'); + + expect(prepareTitleAndTocSpy).toHaveBeenCalledTimes(1); + expect(prepareTitleAndTocSpy).toHaveBeenCalledBefore(embedIntoSpy); + }); + it('should set the title and ToC (after the content has been set)', async () => { + const addTitleAndTocSpy = jasmine.createSpy('addTitleAndToc'); + prepareTitleAndTocSpy.and.returnValue(addTitleAndTocSpy); + addTitleAndTocSpy.and.callFake(() => expect(docViewerEl.textContent).toBe('Foo content')); - await doRender('Foo content', 'foo'); + await doRender('Foo content'); expect(addTitleAndTocSpy).toHaveBeenCalledTimes(1); - expect(addTitleAndTocSpy).toHaveBeenCalledWith('foo'); addTitleAndTocSpy.and.callFake(() => expect(docViewerEl.textContent).toBe('Bar content')); - await doRender('Bar content', 'bar'); + await doRender('Bar content'); expect(addTitleAndTocSpy).toHaveBeenCalledTimes(2); - expect(addTitleAndTocSpy).toHaveBeenCalledWith('bar'); addTitleAndTocSpy.and.callFake(() => expect(docViewerEl.textContent).toBe('')); - await doRender('', 'baz'); + await doRender(''); expect(addTitleAndTocSpy).toHaveBeenCalledTimes(3); - expect(addTitleAndTocSpy).toHaveBeenCalledWith('baz'); addTitleAndTocSpy.and.callFake(() => expect(docViewerEl.textContent).toBe('Qux content')); - await doRender('Qux content', 'qux'); + await doRender('Qux content'); expect(addTitleAndTocSpy).toHaveBeenCalledTimes(4); - expect(addTitleAndTocSpy).toHaveBeenCalledWith('qux'); }); }); @@ -461,12 +457,29 @@ describe('DocViewerComponent', () => { }); }); + describe('(destroying old embedded components)', () => { + it('should destroy old embedded components after creating new embedded components', async () => { + await doRender('
'); + + expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(1); + expect(embedIntoSpy).toHaveBeenCalledBefore(destroyEmbeddedComponentsSpy); + }); + + it('should still destroy old embedded components if the new document is empty', async () => { + await doRender(''); + expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(1); + + await doRender(null); + expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(2); + }); + }); + describe('(swapping views)', () => { - it('should swap the views after creating embedded components', async () => { + it('should swap the views after destroying old embedded components', async () => { await doRender('
'); expect(swapViewsSpy).toHaveBeenCalledTimes(1); - expect(embedIntoSpy).toHaveBeenCalledBefore(swapViewsSpy); + expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledBefore(swapViewsSpy); }); it('should still swap the views if the document is empty', async () => { @@ -477,6 +490,15 @@ describe('DocViewerComponent', () => { expect(swapViewsSpy).toHaveBeenCalledTimes(2); }); + it('should pass the `addTitleAndToc` callback', async () => { + const addTitleAndTocSpy = jasmine.createSpy('addTitleAndToc'); + prepareTitleAndTocSpy.and.returnValue(addTitleAndTocSpy); + + await doRender('
'); + + expect(swapViewsSpy).toHaveBeenCalledWith(addTitleAndTocSpy); + }); + it('should unsubscribe from the previous "swap" observable when unsubscribed from', () => { const obs = new ObservableWithSubscriptionSpies(); swapViewsSpy.and.returnValue(obs); @@ -499,6 +521,25 @@ describe('DocViewerComponent', () => { beforeEach(() => logger = TestBed.get(Logger)); + it('when `prepareTitleAndTocSpy()` fails', async () => { + const error = Error('Typical `prepareTitleAndToc()` error'); + prepareTitleAndTocSpy.and.callFake(() => { + expect(docViewer.nextViewContainer.innerHTML).not.toBe(''); + throw error; + }); + + await doRender('Some content', 'foo'); + + expect(prepareTitleAndTocSpy).toHaveBeenCalledTimes(1); + expect(embedIntoSpy).not.toHaveBeenCalled(); + expect(destroyEmbeddedComponentsSpy).not.toHaveBeenCalled(); + expect(swapViewsSpy).not.toHaveBeenCalled(); + expect(docViewer.nextViewContainer.innerHTML).toBe(''); + expect(logger.output.error).toEqual([ + ['[DocViewer]: Error preparing document \'foo\'.', error], + ]); + }); + it('when `EmbedComponentsService.embedInto()` fails', async () => { const error = Error('Typical `embedInto()` error'); embedIntoSpy.and.callFake(() => { @@ -506,14 +547,34 @@ describe('DocViewerComponent', () => { throw error; }); - await doRender('Some content', 'foo'); + await doRender('Some content', 'bar'); + expect(prepareTitleAndTocSpy).toHaveBeenCalledTimes(1); expect(embedIntoSpy).toHaveBeenCalledTimes(1); + expect(destroyEmbeddedComponentsSpy).not.toHaveBeenCalled(); expect(swapViewsSpy).not.toHaveBeenCalled(); - expect(addTitleAndTocSpy).not.toHaveBeenCalled(); expect(docViewer.nextViewContainer.innerHTML).toBe(''); expect(logger.output.error).toEqual([ - ['[DocViewer]: Error preparing document \'foo\'.', error], + ['[DocViewer]: Error preparing document \'bar\'.', error], + ]); + }); + + it('when `destroyEmbeddedComponents()` fails', async () => { + const error = Error('Typical `destroyEmbeddedComponents()` error'); + destroyEmbeddedComponentsSpy.and.callFake(() => { + expect(docViewer.nextViewContainer.innerHTML).not.toBe(''); + throw error; + }); + + await doRender('Some content', 'baz'); + + expect(prepareTitleAndTocSpy).toHaveBeenCalledTimes(1); + expect(embedIntoSpy).toHaveBeenCalledTimes(1); + expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(1); + expect(swapViewsSpy).not.toHaveBeenCalled(); + expect(docViewer.nextViewContainer.innerHTML).toBe(''); + expect(logger.output.error).toEqual([ + ['[DocViewer]: Error preparing document \'baz\'.', error], ]); }); @@ -524,33 +585,49 @@ describe('DocViewerComponent', () => { throw error; }); - await doRender('Some content', 'bar'); + await doRender('Some content', 'qux'); + expect(prepareTitleAndTocSpy).toHaveBeenCalledTimes(1); expect(embedIntoSpy).toHaveBeenCalledTimes(1); + expect(destroyEmbeddedComponentsSpy).toHaveBeenCalledTimes(1); expect(swapViewsSpy).toHaveBeenCalledTimes(1); - expect(addTitleAndTocSpy).not.toHaveBeenCalled(); expect(docViewer.nextViewContainer.innerHTML).toBe(''); expect(logger.output.error).toEqual([ - ['[DocViewer]: Error preparing document \'bar\'.', error], + ['[DocViewer]: Error preparing document \'qux\'.', error], ]); }); + }); - it('when `addTitleAndTocSpy()` fails', async () => { - const error = Error('Typical `addTitleAndToc()` error'); - addTitleAndTocSpy.and.callFake(() => { - expect(docViewer.nextViewContainer.innerHTML).not.toBe(''); - throw error; - }); + describe('(events)', () => { + it('should emit `docReady` after embedding components', async () => { + const onDocReadySpy = jasmine.createSpy('onDocReady'); + docViewer.docReady.subscribe(onDocReadySpy); - await doRender('Some content', 'baz'); + await doRender('Some content'); - expect(embedIntoSpy).toHaveBeenCalledTimes(1); - expect(swapViewsSpy).toHaveBeenCalledTimes(1); - expect(addTitleAndTocSpy).toHaveBeenCalledTimes(1); - expect(docViewer.nextViewContainer.innerHTML).toBe(''); - expect(logger.output.error).toEqual([ - ['[DocViewer]: Error preparing document \'baz\'.', error], - ]); + expect(onDocReadySpy).toHaveBeenCalledTimes(1); + expect(embedIntoSpy).toHaveBeenCalledBefore(onDocReadySpy); + }); + + it('should emit `docReady` before destroying old embedded components and swapping views', async () => { + const onDocReadySpy = jasmine.createSpy('onDocReady'); + docViewer.docReady.subscribe(onDocReadySpy); + + await doRender('Some content'); + + expect(onDocReadySpy).toHaveBeenCalledTimes(1); + expect(onDocReadySpy).toHaveBeenCalledBefore(destroyEmbeddedComponentsSpy); + expect(onDocReadySpy).toHaveBeenCalledBefore(swapViewsSpy); + }); + + it('should emit `docRendered` after swapping views', async () => { + const onDocRenderedSpy = jasmine.createSpy('onDocRendered'); + docViewer.docRendered.subscribe(onDocRenderedSpy); + + await doRender('Some content'); + + expect(onDocRenderedSpy).toHaveBeenCalledTimes(1); + expect(swapViewsSpy).toHaveBeenCalledBefore(onDocRenderedSpy); }); }); }); @@ -559,8 +636,9 @@ describe('DocViewerComponent', () => { let oldCurrViewContainer: HTMLElement; let oldNextViewContainer: HTMLElement; - const doSwapViews = () => new Promise((resolve, reject) => - docViewer.swapViews().subscribe(resolve, reject)); + const doSwapViews = (cb?: () => void) => + new Promise((resolve, reject) => + docViewer.swapViews(cb).subscribe(resolve, reject)); beforeEach(() => { oldCurrViewContainer = docViewer.currViewContainer; @@ -598,6 +676,73 @@ describe('DocViewerComponent', () => { expect(docViewer.nextViewContainer).toBe(oldNextViewContainer); }); + it('should emit `docRemoved` after removing the leaving view', async () => { + const onDocRemovedSpy = jasmine.createSpy('onDocRemoved').and.callFake(() => { + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(false); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(false); + }); + + docViewer.docRemoved.subscribe(onDocRemovedSpy); + + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(true); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(false); + + await doSwapViews(); + + expect(onDocRemovedSpy).toHaveBeenCalledTimes(1); + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(false); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(true); + }); + + it('should not emit `docRemoved` if the leaving view is already removed', async () => { + const onDocRemovedSpy = jasmine.createSpy('onDocRemoved'); + + docViewer.docRemoved.subscribe(onDocRemovedSpy); + docViewerEl.removeChild(oldCurrViewContainer); + + await doSwapViews(); + + expect(onDocRemovedSpy).not.toHaveBeenCalled(); + }); + + it('should emit `docInserted` after inserting the entering view', async () => { + const onDocInsertedSpy = jasmine.createSpy('onDocInserted').and.callFake(() => { + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(false); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(true); + }); + + docViewer.docInserted.subscribe(onDocInsertedSpy); + + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(true); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(false); + + await doSwapViews(); + + expect(onDocInsertedSpy).toHaveBeenCalledTimes(1); + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(false); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(true); + }); + + it('should call the callback after inserting the entering view', async () => { + const onInsertedCb = jasmine.createSpy('onInsertedCb').and.callFake(() => { + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(false); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(true); + }); + const onDocInsertedSpy = jasmine.createSpy('onDocInserted'); + + docViewer.docInserted.subscribe(onDocInsertedSpy); + + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(true); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(false); + + await doSwapViews(onInsertedCb); + + expect(onInsertedCb).toHaveBeenCalledTimes(1); + expect(onInsertedCb).toHaveBeenCalledBefore(onDocInsertedSpy); + expect(docViewerEl.contains(oldCurrViewContainer)).toBe(false); + expect(docViewerEl.contains(oldNextViewContainer)).toBe(true); + }); + it('should empty the previous view', async () => { await doSwapViews(); diff --git a/aio/src/app/layout/doc-viewer/doc-viewer.component.ts b/aio/src/app/layout/doc-viewer/doc-viewer.component.ts index 40de01b11e..0580f27778 100644 --- a/aio/src/app/layout/doc-viewer/doc-viewer.component.ts +++ b/aio/src/app/layout/doc-viewer/doc-viewer.component.ts @@ -48,8 +48,21 @@ export class DocViewerComponent implements DoCheck, OnDestroy { } } - @Output() - docRendered = new EventEmitter(); + // The new document is ready to be inserted into the viewer. + // (Embedded components have been loaded and instantiated, if necessary.) + @Output() docReady = new EventEmitter(); + + // The previous document has been removed from the viewer. + // (The leaving animation (if any) has been completed and the node has been removed from the DOM.) + @Output() docRemoved = new EventEmitter(); + + // The new document has been inserted into the viewer. + // (The node has been inserted into the DOM, but the entering animation may still be in progress.) + @Output() docInserted = new EventEmitter(); + + // The new document has been fully rendered into the viewer. + // (The entering animation has been completed.) + @Output() docRendered = new EventEmitter(); constructor( elementRef: ElementRef, @@ -70,9 +83,7 @@ export class DocViewerComponent implements DoCheck, OnDestroy { this.onDestroy$.subscribe(() => this.destroyEmbeddedComponents()); this.docContents$ - .do(() => this.destroyEmbeddedComponents()) .switchMap(newDoc => this.render(newDoc)) - .do(() => this.docRendered.emit()) .takeUntil(this.onDestroy$) .subscribe(); } @@ -85,27 +96,6 @@ export class DocViewerComponent implements DoCheck, OnDestroy { this.onDestroy$.emit(); } - /** - * Set up the window title and ToC. - */ - protected addTitleAndToc(docId: string): void { - this.tocService.reset(); - const titleEl = this.currViewContainer.querySelector('h1'); - let title = ''; - - // Only create TOC for docs with an

title - // If you don't want a TOC, add "no-toc" class to

- if (titleEl) { - title = (typeof titleEl.innerText === 'string') ? titleEl.innerText : titleEl.textContent; - if (!/(no-toc|notoc)/i.test(titleEl.className)) { - this.tocService.genToc(this.currViewContainer, docId); - titleEl.insertAdjacentHTML('afterend', ''); - } - } - - this.titleService.setTitle(title ? `Angular - ${title}` : 'Angular'); - } - /** * Destroy the embedded components to avoid memory leaks. */ @@ -114,20 +104,53 @@ export class DocViewerComponent implements DoCheck, OnDestroy { this.embeddedComponentRefs = []; } + /** + * Prepare for setting the window title and ToC. + * Return a function to actually set them. + */ + protected prepareTitleAndToc(targetElem: HTMLElement, docId: string): () => void { + const titleEl = targetElem.querySelector('h1'); + const hasToc = !!titleEl && !/no-?toc/i.test(titleEl.className); + + if (hasToc) { + titleEl.insertAdjacentHTML('afterend', ''); + } + + return () => { + this.tocService.reset(); + let title = ''; + + // Only create ToC for docs with an `

` heading. + // If you don't want a ToC, add "no-toc" class to `

`. + if (titleEl) { + title = (typeof titleEl.innerText === 'string') ? titleEl.innerText : titleEl.textContent; + + if (hasToc) { + this.tocService.genToc(targetElem, docId); + } + } + + this.titleService.setTitle(title ? `Angular - ${title}` : 'Angular'); + }; + } + /** * Add doc content to host element and build it out with embedded components. */ protected render(doc: DocumentContents): Observable { + let addTitleAndToc: () => void; + return this.void$ - .do(() => { - // Security: `doc.contents` is always authored by the documentation team - // and is considered to be safe. - this.nextViewContainer.innerHTML = doc.contents || ''; - }) + // Security: `doc.contents` is always authored by the documentation team + // and is considered to be safe. + .do(() => this.nextViewContainer.innerHTML = doc.contents || '') + .do(() => addTitleAndToc = this.prepareTitleAndToc(this.nextViewContainer, doc.id)) .switchMap(() => this.embedComponentsService.embedInto(this.nextViewContainer)) + .do(() => this.docReady.emit()) + .do(() => this.destroyEmbeddedComponents()) .do(componentRefs => this.embeddedComponentRefs = componentRefs) - .switchMap(() => this.swapViews()) - .do(() => this.addTitleAndToc(doc.id)) + .switchMap(() => this.swapViews(addTitleAndToc)) + .do(() => this.docRendered.emit()) .catch(err => { this.nextViewContainer.innerHTML = ''; this.logger.error(`[DocViewer]: Error preparing document '${doc.id}'.`, err); @@ -139,8 +162,12 @@ export class DocViewerComponent implements DoCheck, OnDestroy { * Swap the views, removing `currViewContainer` and inserting `nextViewContainer`. * (At this point all content should be ready, including having loaded and instantiated embedded * components.) + * + * Optionally, run a callback as soon as `nextViewContainer` has been inserted, but before the + * entering animation has been completed. This is useful for work that needs to be done as soon as + * the element has been attached to the DOM. */ - protected swapViews(): Observable { + protected swapViews(onInsertedCb = () => undefined): Observable { const raf$ = new Observable(subscriber => { const rafId = requestAnimationFrame(() => { subscriber.next(); @@ -174,12 +201,15 @@ export class DocViewerComponent implements DoCheck, OnDestroy { done$ = done$ // Remove the current view from the viewer. .switchMap(() => animateLeave(this.currViewContainer)) - .do(() => this.currViewContainer.parentElement.removeChild(this.currViewContainer)); + .do(() => this.currViewContainer.parentElement.removeChild(this.currViewContainer)) + .do(() => this.docRemoved.emit()); } return done$ // Insert the next view into the viewer. .do(() => this.hostElement.appendChild(this.nextViewContainer)) + .do(() => onInsertedCb()) + .do(() => this.docInserted.emit()) .switchMap(() => animateEnter(this.nextViewContainer)) // Update the view references and clean up unused nodes. .do(() => { diff --git a/aio/src/testing/doc-viewer-utils.ts b/aio/src/testing/doc-viewer-utils.ts index 1240b489ac..75bd3e7d86 100644 --- a/aio/src/testing/doc-viewer-utils.ts +++ b/aio/src/testing/doc-viewer-utils.ts @@ -21,10 +21,10 @@ export class TestDocViewerComponent extends DocViewerComponent { currViewContainer: HTMLElement; nextViewContainer: HTMLElement; - addTitleAndToc(docId: string): void { return null as any; } destroyEmbeddedComponents(): void { return null as any; } + prepareTitleAndToc(targetElem: HTMLElement, docId: string): () => void { return null as any; } render(doc: DocumentContents): Observable { return null as any; } - swapViews(): Observable { return null as any; } + swapViews(onInsertedCb?: () => void): Observable { return null as any; } }