From 97fec9236565bdd4eb84c2442645fac6c3ecdcab Mon Sep 17 00:00:00 2001 From: Manu MA Date: Thu, 18 Jul 2019 10:26:54 +0200 Subject: [PATCH] fix(router-outlet): attach entering view before first change detection (#18821) --- .../navigation/ion-router-outlet.ts | 1 - .../directives/navigation/stack-controller.ts | 44 ++++++++++------ angular/src/providers/angular-delegate.ts | 33 ++++++------ .../test-app/e2e/src/router-link.e2e-spec.ts | 50 +++++++++++-------- angular/test/test-app/e2e/src/utils.ts | 18 +++++-- .../src/app/alert/alert.component.html | 4 +- .../test-app/src/app/alert/alert.component.ts | 7 +++ .../router-link-page.component.html | 1 + .../router-link-page.component.ts | 13 +++++ core/scripts/swiper.rollup.config.js | 4 +- 10 files changed, 112 insertions(+), 63 deletions(-) diff --git a/angular/src/directives/navigation/ion-router-outlet.ts b/angular/src/directives/navigation/ion-router-outlet.ts index 55cf6e3610..9bfacd11f3 100644 --- a/angular/src/directives/navigation/ion-router-outlet.ts +++ b/angular/src/directives/navigation/ion-router-outlet.ts @@ -205,7 +205,6 @@ export class IonRouterOutlet implements OnDestroy, OnInit { // Calling `markForCheck` to make sure we will run the change detection when the // `RouterOutlet` is inside a `ChangeDetectionStrategy.OnPush` component. enteringView = this.stackCtrl.createView(this.activated, activatedRoute); - enteringView.ref.changeDetectorRef.detectChanges(); // Store references to the proxy by component this.proxyMap.set(cmpRef.instance, activatedRouteProxy); diff --git a/angular/src/directives/navigation/stack-controller.ts b/angular/src/directives/navigation/stack-controller.ts index ba647815c6..e4b04436ac 100644 --- a/angular/src/directives/navigation/stack-controller.ts +++ b/angular/src/directives/navigation/stack-controller.ts @@ -31,7 +31,7 @@ export class StackController { createView(ref: ComponentRef, activatedRoute: ActivatedRoute): RouteView { const url = getUrl(this.router, activatedRoute); const element = (ref && ref.location && ref.location.nativeElement) as HTMLElement; - const unlistenEvents = bindLifecycleEvents(ref.instance, element); + const unlistenEvents = bindLifecycleEvents(this.zone, ref.instance, element); return { id: this.nextId++, stackId: computeStackId(this.tabsPrefix, url), @@ -90,16 +90,28 @@ export class StackController { } } + const reused = this.views.includes(enteringView); const views = this.insertView(enteringView, direction); - return this.wait(() => { - return this.transition(enteringView, leavingView, animation, this.canGoBack(1), false) - .then(() => cleanupAsync(enteringView, views, viewsSnapshot, this.location)) - .then(() => ({ - enteringView, - direction, - animation, - tabSwitch - })); + + // Trigger change detection before transition starts + // This will call ngOnInit() the first time too, just after the view + // was attached to the dom, but BEFORE the transition starts + if (!reused) { + enteringView.ref.changeDetectorRef.detectChanges(); + } + + // Wait until previous transitions finish + return this.zone.runOutsideAngular(() => { + return this.wait(() => { + return this.transition(enteringView, leavingView, animation, this.canGoBack(1), false) + .then(() => cleanupAsync(enteringView, views, viewsSnapshot, this.location)) + .then(() => ({ + enteringView, + direction, + animation, + tabSwitch + })); + }); }); } @@ -199,11 +211,11 @@ export class StackController { if (enteringView) { enteringView.ref.changeDetectorRef.reattach(); } - // TODO: disconnect leaving page from change detection to + // disconnect leaving page from change detection to // reduce jank during the page transition - // if (leavingView) { - // leavingView.ref.changeDetectorRef.detach(); - // } + if (leavingView) { + leavingView.ref.changeDetectorRef.detach(); + } const enteringEl = enteringView ? enteringView.element : undefined; const leavingEl = leavingView ? leavingView.element : undefined; const containerEl = this.containerEl; @@ -213,13 +225,13 @@ export class StackController { containerEl.appendChild(enteringEl); } - return this.zone.runOutsideAngular(() => containerEl.commit(enteringEl, leavingEl, { + return containerEl.commit(enteringEl, leavingEl, { deepWait: true, duration: direction === undefined ? 0 : undefined, direction, showGoBack, progressAnimation - })); + }); } return Promise.resolve(false); } diff --git a/angular/src/providers/angular-delegate.ts b/angular/src/providers/angular-delegate.ts index 36ac4fc001..301823e91a 100644 --- a/angular/src/providers/angular-delegate.ts +++ b/angular/src/providers/angular-delegate.ts @@ -34,10 +34,10 @@ export class AngularFrameworkDelegate implements FrameworkDelegate { ) {} attachViewToDom(container: any, component: any, params?: any, cssClasses?: string[]): Promise { - return new Promise(resolve => { - this.zone.run(() => { + return this.zone.run(() => { + return new Promise(resolve => { const el = attachView( - this.resolver, this.injector, this.location, this.appRef, + this.zone, this.resolver, this.injector, this.location, this.appRef, this.elRefMap, this.elEventsMap, container, component, params, cssClasses ); @@ -47,8 +47,8 @@ export class AngularFrameworkDelegate implements FrameworkDelegate { } removeViewFromDom(_container: any, component: any): Promise { - return new Promise(resolve => { - this.zone.run(() => { + return this.zone.run(() => { + return new Promise(resolve => { const componentRef = this.elRefMap.get(component); if (componentRef) { componentRef.destroy(); @@ -66,6 +66,7 @@ export class AngularFrameworkDelegate implements FrameworkDelegate { } export function attachView( + zone: NgZone, resolver: ComponentFactoryResolver, injector: Injector, location: ViewContainerRef | undefined, @@ -93,7 +94,7 @@ export function attachView( hostElement.classList.add(clazz); } } - const unbindEvents = bindLifecycleEvents(instance, hostElement); + const unbindEvents = bindLifecycleEvents(zone, instance, hostElement); container.appendChild(hostElement); if (!location) { @@ -113,15 +114,17 @@ const LIFECYCLES = [ LIFECYCLE_WILL_UNLOAD ]; -export function bindLifecycleEvents(instance: any, element: HTMLElement) { - const unregisters = LIFECYCLES - .filter(eventName => typeof instance[eventName] === 'function') - .map(eventName => { - const handler = (ev: any) => instance[eventName](ev.detail); - element.addEventListener(eventName, handler); - return () => element.removeEventListener(eventName, handler); - }); - return () => unregisters.forEach(fn => fn()); +export function bindLifecycleEvents(zone: NgZone, instance: any, element: HTMLElement) { + return zone.run(() => { + const unregisters = LIFECYCLES + .filter(eventName => typeof instance[eventName] === 'function') + .map(eventName => { + const handler = (ev: any) => instance[eventName](ev.detail); + element.addEventListener(eventName, handler); + return () => element.removeEventListener(eventName, handler); + }); + return () => unregisters.forEach(fn => fn()); + }); } const NavParamsToken = new InjectionToken('NavParamsToken'); diff --git a/angular/test/test-app/e2e/src/router-link.e2e-spec.ts b/angular/test/test-app/e2e/src/router-link.e2e-spec.ts index 1f7f8b3904..20fcbb23cf 100644 --- a/angular/test/test-app/e2e/src/router-link.e2e-spec.ts +++ b/angular/test/test-app/e2e/src/router-link.e2e-spec.ts @@ -1,5 +1,5 @@ import { browser, element, by, protractor } from 'protractor'; -import { waitTime, testStack, testLifeCycle, handleErrorMessages } from './utils'; +import { waitTime, testStack, testLifeCycle, handleErrorMessages, getText } from './utils'; const EC = protractor.ExpectedConditions; @@ -23,12 +23,13 @@ describe('router-link params and fragments', () => { it('should return to a page with preserved query param and fragment', async () => { await browser.get('/router-link?ionic:_testing=true'); + await waitTime(30); await element(by.css('#queryParamsFragment')).click(); - await waitTime(200); + await waitTime(400); await element(by.css('#goToPage3')).click(); browser.wait(EC.urlContains('router-link-page3'), 5000); - await waitTime(200); + await waitTime(400); await element(by.css('#goBackFromPage3')).click(); @@ -38,6 +39,7 @@ describe('router-link params and fragments', () => { it('should preserve query param and fragment with defaultHref string', async () => { await browser.get('/router-link-page3?ionic:_testing=true'); + await waitTime(30); await element(by.css('#goBackFromPage3')).click(); @@ -71,18 +73,6 @@ describe('router-link', () => { it('should go forward with ion-button[routerLink]', async () => { await element(by.css('#routerLink')).click(); await testForward(); - - // test go back - await element(by.css('ion-back-button')).click(); - await waitTime(500); - - await testStack('ion-router-outlet', ['app-router-link']); - await testLifeCycle('app-router-link', { - ionViewWillEnter: 2, - ionViewDidEnter: 2, - ionViewWillLeave: 1, - ionViewDidLeave: 1, - }); }); it('should go forward with a[routerLink]', async () => { @@ -140,18 +130,23 @@ describe('router-link', () => { async function testForward() { await waitTime(2500); await testStack('ion-router-outlet', ['app-router-link', 'app-router-link-page']); - await testLifeCycle('app-router-link', { - ionViewWillEnter: 1, - ionViewDidEnter: 1, - ionViewWillLeave: 1, - ionViewDidLeave: 1, - }); await testLifeCycle('app-router-link-page', { ionViewWillEnter: 1, ionViewDidEnter: 1, ionViewWillLeave: 0, ionViewDidLeave: 0, }); + expect(await getText(`app-router-link-page #canGoBack`)).toEqual('true'); + + await browser.navigate().back(); + await waitTime(100); + await testStack('ion-router-outlet', ['app-router-link']); + await testLifeCycle('app-router-link', { + ionViewWillEnter: 2, + ionViewDidEnter: 2, + ionViewWillLeave: 1, + ionViewDidLeave: 1, + }); } async function testRoot() { @@ -163,6 +158,8 @@ async function testRoot() { ionViewWillLeave: 0, ionViewDidLeave: 0, }); + expect(await getText(`app-router-link-page #canGoBack`)).toEqual('false'); + await browser.navigate().back(); await waitTime(100); await testStack('ion-router-outlet', ['app-router-link']); @@ -183,4 +180,15 @@ async function testBack() { ionViewWillLeave: 0, ionViewDidLeave: 0, }); + expect(await getText(`app-router-link-page #canGoBack`)).toEqual('false'); + + await browser.navigate().back(); + await waitTime(100); + await testStack('ion-router-outlet', ['app-router-link']); + await testLifeCycle('app-router-link', { + ionViewWillEnter: 1, + ionViewDidEnter: 1, + ionViewWillLeave: 0, + ionViewDidLeave: 0, + }); } diff --git a/angular/test/test-app/e2e/src/utils.ts b/angular/test/test-app/e2e/src/utils.ts index eab27612cf..cc85e9ab6c 100644 --- a/angular/test/test-app/e2e/src/utils.ts +++ b/angular/test/test-app/e2e/src/utils.ts @@ -48,11 +48,19 @@ export function handleErrorMessages() { export async function testLifeCycle(selector: string, expected: LifeCycleCount) { await waitTime(50); - expect(await getText(`${selector} #ngOnInit`)).toEqual('1'); - expect(await getText(`${selector} #ionViewWillEnter`)).toEqual(expected.ionViewWillEnter.toString()); - expect(await getText(`${selector} #ionViewDidEnter`)).toEqual(expected.ionViewDidEnter.toString()); - expect(await getText(`${selector} #ionViewWillLeave`)).toEqual(expected.ionViewWillLeave.toString()); - expect(await getText(`${selector} #ionViewDidLeave`)).toEqual(expected.ionViewDidLeave.toString()); + const results = await Promise.all([ + getText(`${selector} #ngOnInit`), + getText(`${selector} #ionViewWillEnter`), + getText(`${selector} #ionViewDidEnter`), + getText(`${selector} #ionViewWillLeave`), + getText(`${selector} #ionViewDidLeave`), + ]); + + expect(results[0]).toEqual('1'); + expect(results[1]).toEqual(expected.ionViewWillEnter.toString()); + expect(results[2]).toEqual(expected.ionViewDidEnter.toString()); + expect(results[3]).toEqual(expected.ionViewWillLeave.toString()); + expect(results[4]).toEqual(expected.ionViewDidLeave.toString()); } export async function testStack(selector: string, expected: string[]) { diff --git a/angular/test/test-app/src/app/alert/alert.component.html b/angular/test/test-app/src/app/alert/alert.component.html index 68f6210040..ed8c16e3e6 100644 --- a/angular/test/test-app/src/app/alert/alert.component.html +++ b/angular/test/test-app/src/app/alert/alert.component.html @@ -1,10 +1,10 @@ - Modal test + Alert test - Open Alert +

Change Detections: {{counter()}}

 diff --git a/angular/test/test-app/src/app/alert/alert.component.ts b/angular/test/test-app/src/app/alert/alert.component.ts index 8ff209ab62..95c3d33433 100644 --- a/angular/test/test-app/src/app/alert/alert.component.ts +++ b/angular/test/test-app/src/app/alert/alert.component.ts @@ -8,10 +8,17 @@ import { NavComponent } from '../nav/nav.component'; }) export class AlertComponent { + changes = 0; + constructor( private alertCtrl: AlertController ) { } + counter() { + this.changes++; + return Math.floor(this.changes / 2); + } + async openAlert() { const alert = await this.alertCtrl.create({ header: 'Hello', diff --git a/angular/test/test-app/src/app/router-link-page/router-link-page.component.html b/angular/test/test-app/src/app/router-link-page/router-link-page.component.html index 8077d198b0..aa12296afb 100644 --- a/angular/test/test-app/src/app/router-link-page/router-link-page.component.html +++ b/angular/test/test-app/src/app/router-link-page/router-link-page.component.html @@ -9,6 +9,7 @@

ngOnInit: {{onInit}}

+

canGoBack: {{canGoBack}}

ionViewWillEnter: {{willEnter}}

ionViewDidEnter: {{didEnter}}

ionViewWillLeave: {{willLeave}}

diff --git a/angular/test/test-app/src/app/router-link-page/router-link-page.component.ts b/angular/test/test-app/src/app/router-link-page/router-link-page.component.ts index 882b0ffa70..74149e8a48 100644 --- a/angular/test/test-app/src/app/router-link-page/router-link-page.component.ts +++ b/angular/test/test-app/src/app/router-link-page/router-link-page.component.ts @@ -1,4 +1,5 @@ import { Component, OnInit, NgZone } from '@angular/core'; +import { IonRouterOutlet } from '@ionic/angular'; @Component({ selector: 'app-router-link-page', @@ -11,9 +12,15 @@ export class RouterLinkPageComponent implements OnInit { didEnter = 0; willLeave = 0; didLeave = 0; + canGoBack: boolean = null; + + constructor( + private ionRouterOutlet: IonRouterOutlet + ) {} ngOnInit() { NgZone.assertInAngularZone(); + this.canGoBack = this.ionRouterOutlet.canGoBack(); this.onInit++; } @@ -21,10 +28,16 @@ export class RouterLinkPageComponent implements OnInit { if (this.onInit !== 1) { throw new Error('ngOnInit was not called'); } + if (this.canGoBack !== this.ionRouterOutlet.canGoBack()) { + throw new Error('canGoBack() changed'); + } NgZone.assertInAngularZone(); this.willEnter++; } ionViewDidEnter() { + if (this.canGoBack !== this.ionRouterOutlet.canGoBack()) { + throw new Error('canGoBack() changed'); + } NgZone.assertInAngularZone(); this.didEnter++; } diff --git a/core/scripts/swiper.rollup.config.js b/core/scripts/swiper.rollup.config.js index a0974615f4..13b8e450db 100644 --- a/core/scripts/swiper.rollup.config.js +++ b/core/scripts/swiper.rollup.config.js @@ -7,8 +7,6 @@ export default { format: 'es' }, plugins: [ - resolve({ - module: true - }) + resolve() ] }; \ No newline at end of file