From 66191e8a37c5fe2eb31b6f0b4ecc075b6e605ed8 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Sun, 21 Jan 2018 14:41:52 +0200 Subject: [PATCH] fix(aio): reduce flicker and reflows for initial rendering (#21695) For the initial rendering, where there is no transition from a previous visual state to a new one, animations make little sense. The page should load with as few reflows as possible. Similarly, while we typically want to defer updating the SideNav state (e.g. opened/closed) until the "leaving" document is animated out of the page, on the initial rendering (where there is no "leaving" document) this leads to the SideNav flashing (from closed to open). These worked as expected before, but several parts (mostly related to documents with a SideNav) have been accidentally broken in recent commits (e.g. when upgraded to latest material, or enabled animations for DocViewer transitions, etc.). This commit restores the previous behavior by ensuring that (on the initial rendering) the SideNav state is updated as soon as possible and that there will be no animations when: 1. The hamburger button appears. 2. The SideNav is opened. 3. The main section's width is adjusted to make room for the SideNav. PR Close #21695 --- aio/src/app/app.component.html | 2 +- aio/src/app/app.component.spec.ts | 33 ++++++++++++ aio/src/app/app.component.ts | 67 ++++++++++++++----------- aio/src/styles/1-layouts/_sidenav.scss | 30 +++++------ aio/src/styles/1-layouts/_top-menu.scss | 9 ++-- 5 files changed, 91 insertions(+), 50 deletions(-) diff --git a/aio/src/app/app.component.html b/aio/src/app/app.component.html index bad7f69913..1a7d457127 100644 --- a/aio/src/app/app.component.html +++ b/aio/src/app/app.component.html @@ -18,7 +18,7 @@ - diff --git a/aio/src/app/app.component.spec.ts b/aio/src/app/app.component.spec.ts index 4ce21669d3..bc77fed2bd 100644 --- a/aio/src/app/app.component.spec.ts +++ b/aio/src/app/app.component.spec.ts @@ -34,6 +34,7 @@ import { TocItem, TocService } from 'app/shared/toc.service'; const sideBySideBreakPoint = 992; const hideToCBreakPoint = 800; +const startedDelay = 100; describe('AppComponent', () => { let component: AppComponent; @@ -910,36 +911,68 @@ describe('AppComponent', () => { }); describe('initial rendering', () => { + beforeEach(jasmine.clock().install); + afterEach(jasmine.clock().uninstall); + + it('should initially disable Angular animations until a document is rendered', () => { + initializeTest(false); + jasmine.clock().tick(1); // triggers the HTTP response for the document + + expect(component.isStarting).toBe(true); + expect(fixture.debugElement.properties['@.disabled']).toBe(true); + + triggerDocViewerEvent('docInserted'); + jasmine.clock().tick(startedDelay); + fixture.detectChanges(); + expect(component.isStarting).toBe(true); + expect(fixture.debugElement.properties['@.disabled']).toBe(true); + + triggerDocViewerEvent('docRendered'); + jasmine.clock().tick(startedDelay); + fixture.detectChanges(); + expect(component.isStarting).toBe(false); + expect(fixture.debugElement.properties['@.disabled']).toBe(false); + }); + it('should initially add the starting class until a document is rendered', () => { initializeTest(false); + jasmine.clock().tick(1); // triggers the HTTP response for the document const sidenavContainer = fixture.debugElement.query(By.css('mat-sidenav-container')).nativeElement; expect(component.isStarting).toBe(true); + expect(hamburger.classList.contains('starting')).toBe(true); expect(sidenavContainer.classList.contains('starting')).toBe(true); triggerDocViewerEvent('docInserted'); + jasmine.clock().tick(startedDelay); fixture.detectChanges(); expect(component.isStarting).toBe(true); + expect(hamburger.classList.contains('starting')).toBe(true); expect(sidenavContainer.classList.contains('starting')).toBe(true); triggerDocViewerEvent('docRendered'); + jasmine.clock().tick(startedDelay); fixture.detectChanges(); expect(component.isStarting).toBe(false); + expect(hamburger.classList.contains('starting')).toBe(false); expect(sidenavContainer.classList.contains('starting')).toBe(false); }); it('should initially disable animations on the DocViewer for the first rendering', () => { initializeTest(false); + jasmine.clock().tick(1); // triggers the HTTP response for the document expect(component.isStarting).toBe(true); expect(docViewer.classList.contains('no-animations')).toBe(true); triggerDocViewerEvent('docInserted'); + jasmine.clock().tick(startedDelay); fixture.detectChanges(); expect(component.isStarting).toBe(true); expect(docViewer.classList.contains('no-animations')).toBe(true); triggerDocViewerEvent('docRendered'); + jasmine.clock().tick(startedDelay); fixture.detectChanges(); expect(component.isStarting).toBe(false); expect(docViewer.classList.contains('no-animations')).toBe(false); diff --git a/aio/src/app/app.component.ts b/aio/src/app/app.component.ts index ae48e6533c..094688dccd 100644 --- a/aio/src/app/app.component.ts +++ b/aio/src/app/app.component.ts @@ -28,7 +28,7 @@ export class AppComponent implements OnInit { currentDocument: DocumentContents; currentDocVersion: NavigationNode; - currentNodes: CurrentNodes; + currentNodes: CurrentNodes = {}; currentPath: string; docVersions: NavigationNode[]; dtOn = false; @@ -57,9 +57,11 @@ export class AppComponent implements OnInit { @HostBinding('class') hostClasses = ''; - isFetching = false; + // Disable all Angular animations for the initial render. + @HostBinding('@.disabled') isStarting = true; isTransitioning = true; + isFetching = false; isSideBySide = false; private isFetchingTimeout: any; private isSideNavDoc = false; @@ -118,12 +120,6 @@ export class AppComponent implements OnInit { /* No need to unsubscribe because this root component never dies */ this.documentService.currentDocument.subscribe(doc => this.currentDocument = doc); - // Generally, we want to delay updating the host classes for the new document, until after the - // leaving document has been removed (to avoid having the styles for the new document applied - // prematurely). - // On the first document, though, (when we know there is no previous document), we want to - // ensure the styles are applied as soon as possible to avoid flicker. - this.documentService.currentDocument.first().subscribe(doc => this.updateHostClassesForDoc(doc)); this.locationService.currentPath.subscribe(path => { // Redirect to docs if we are in archive mode and are not hitting a docs page @@ -175,11 +171,22 @@ export class AppComponent implements OnInit { this.topMenuNarrowNodes = views['TopBarNarrow'] || this.topMenuNodes; }); - this.navigationService.versionInfo.subscribe( vi => this.versionInfo = vi ); + this.navigationService.versionInfo.subscribe(vi => this.versionInfo = vi); const hasNonEmptyToc = this.tocService.tocList.map(tocList => tocList.length > 0); combineLatest(hasNonEmptyToc, this.showFloatingToc) .subscribe(([hasToc, showFloatingToc]) => this.hasFloatingToc = hasToc && showFloatingToc); + + // Generally, we want to delay updating the shell (e.g. host classes, sidenav state) for the new + // document, until after the leaving document has been removed (to avoid having the styles for + // the new document applied prematurely). + // For the first document, though, (when we know there is no previous document), we want to + // ensure the styles are applied as soon as possible to avoid flicker. + combineLatest( + this.documentService.currentDocument, // ...needed to determine host classes + this.navigationService.currentNodes) // ...needed to determine `sidenav` state + .first() + .subscribe(() => this.updateShell()); } // Scroll to the anchor in the hash fragment or top of doc. @@ -205,14 +212,11 @@ export class AppComponent implements OnInit { } onDocInserted() { - // TODO: Find a better way to avoid `ExpressionChangedAfterItHasBeenChecked` error. - setTimeout(() => { - // Update the SideNav state (if necessary). - this.updateSideNav(); - - // Update the host classes to match the new document. - this.updateHostClassesForDoc(this.currentDocument); - }); + // Update the shell (host classes, sidenav state) to match the new document. + // This may be called as a result of actions initiated by view updates. + // In order to avoid errors (e.g. `ExpressionChangedAfterItHasBeenChecked`), updating the view + // (e.g. sidenav, host classes) needs to happen asynchronously. + setTimeout(() => this.updateShell()); // Scroll 500ms after the new document has been inserted into the doc-viewer. // The delay is to allow time for async layout to complete. @@ -220,7 +224,14 @@ export class AppComponent implements OnInit { } onDocRendered() { - this.isStarting = false; + if (this.isStarting) { + // In order to ensure that the initial sidenav-content left margin + // adjustment happens without animation, we need to ensure that + // `isStarting` remains `true` until the margin change is triggered. + // (Apparently, this happens with a slight delay.) + setTimeout(() => this.isStarting = false, 100); + } + this.isTransitioning = false; } @@ -241,7 +252,7 @@ export class AppComponent implements OnInit { // items in the top-bar, ensure the sidenav is closed. // (This condition can only be met when the resize event changes the value of `isSideBySide` // from `false` to `true` while on a non-sidenav doc.) - this.sideNavToggle(false); + this.sidenav.toggle(false); } } @@ -272,10 +283,6 @@ export class AppComponent implements OnInit { return true; } - sideNavToggle(value?: boolean) { - this.sidenav.toggle(value); - } - setPageId(id: string) { // Special case the home page this.pageId = (id === 'index') ? 'home' : id.replace('/', '-'); @@ -300,7 +307,7 @@ export class AppComponent implements OnInit { const sideNavOpen = `sidenav-${this.sidenav.opened ? 'open' : 'closed'}`; const pageClass = `page-${this.pageId}`; const folderClass = `folder-${this.folderId}`; - const viewClasses = Object.keys(this.currentNodes || {}).map(view => `view-${view}`).join(' '); + const viewClasses = Object.keys(this.currentNodes).map(view => `view-${view}`).join(' '); const notificationClass = `aio-notification-${this.notification.showNotification}`; const notificationAnimatingClass = this.notificationAnimating ? 'aio-notification-animating' : ''; @@ -315,9 +322,13 @@ export class AppComponent implements OnInit { ].join(' '); } - updateHostClassesForDoc(doc: DocumentContents) { - this.setPageId(doc.id); - this.setFolderId(doc.id); + updateShell() { + // Update the SideNav state (if necessary). + this.updateSideNav(); + + // Update the host classes. + this.setPageId(this.currentDocument.id); + this.setFolderId(this.currentDocument.id); this.updateHostClasses(); } @@ -333,7 +344,7 @@ export class AppComponent implements OnInit { } // May be open or closed when wide; always closed when narrow. - this.sideNavToggle(this.isSideBySide && openSideNav); + this.sidenav.toggle(this.isSideBySide && openSideNav); } // Dynamically change height of table of contents container diff --git a/aio/src/styles/1-layouts/_sidenav.scss b/aio/src/styles/1-layouts/_sidenav.scss index 4aa2f583f6..2e8b8c7baf 100644 --- a/aio/src/styles/1-layouts/_sidenav.scss +++ b/aio/src/styles/1-layouts/_sidenav.scss @@ -1,11 +1,6 @@ -// Disable sidenav animations while starting the app -// See https://github.com/angular/material2/blob/master/src/lib/sidenav/sidenav-transitions.scss -.starting.mat-sidenav-transition { - .mat-sidenav, - .mat-sidenav-content, - .mat-sidenav-backdrop.mat-sidenav-shown { - transition: none; - } +// Disable sidenav animations for the initial render. +.starting.mat-drawer-transition .mat-drawer-content { + transition: none; } aio-nav-menu { @@ -42,19 +37,19 @@ mat-sidenav.mat-sidenav.sidenav { } mat-sidenav-container.sidenav-container { - min-height: 100%; - height: auto !important; - max-width: 100%; - margin: 0; - transform: none; + min-height: 100%; + height: auto !important; + max-width: 100%; + margin: 0; + transform: none; - &.has-floating-toc { - max-width: 82%; - } + &.has-floating-toc { + max-width: 82%; + } } mat-sidenav-container div.mat-sidenav-content { - height: auto; + height: auto; } .vertical-menu-item { @@ -165,7 +160,6 @@ button.vertical-menu-item { .level-1:not(.expanded) .mat-icon, .level-2:not(.expanded) .mat-icon { @include rotate(0deg); - // margin: 4px; } aio-nav-menu.top-menu { diff --git a/aio/src/styles/1-layouts/_top-menu.scss b/aio/src/styles/1-layouts/_top-menu.scss index 5fc6611596..71da0aaf21 100644 --- a/aio/src/styles/1-layouts/_top-menu.scss +++ b/aio/src/styles/1-layouts/_top-menu.scss @@ -68,9 +68,12 @@ aio-shell.folder-tutorial mat-toolbar.mat-toolbar { height: 100%; margin: $hamburgerShownMargin; padding: 0; - transition-duration: .4s; - transition-property: color, margin; - transition-timing-function: cubic-bezier(.25, .8, .25, 1); + + &:not(.starting) { + transition-duration: .4s; + transition-property: color, margin; + transition-timing-function: cubic-bezier(.25, .8, .25, 1); + } @media (min-width: 992px) { // Hamburger hidden by default on large screens.