From a655cd78acba2e2102d6ac02153275ecb832f8cc Mon Sep 17 00:00:00 2001 From: Dan Bucholtz Date: Tue, 14 Jun 2016 15:52:24 -0500 Subject: [PATCH] Feature/child nav fixes (#6907) * fix(navigation): fixes bug where navigating to page with child-nav immediately re-enables app fixes bug where navigating to page with child-nav immediately re-enables app closes #6752 * test(navigation): added some additional tests added some additional tests * test(tabs): added multiple nested children test added multiple nested children test --- src/components/nav/nav-controller.ts | 78 ++- src/components/nav/test/child-navs/e2e.ts | 3 + src/components/nav/test/child-navs/index.ts | 113 ++++ .../nav/test/nav-controller.spec.ts | 488 +++++++++++++++--- src/components/tabs/test/child-navs/e2e.ts | 0 src/components/tabs/test/child-navs/index.ts | 140 +++++ 6 files changed, 732 insertions(+), 90 deletions(-) create mode 100644 src/components/nav/test/child-navs/e2e.ts create mode 100644 src/components/nav/test/child-navs/index.ts create mode 100644 src/components/tabs/test/child-navs/e2e.ts create mode 100644 src/components/tabs/test/child-navs/index.ts diff --git a/src/components/nav/nav-controller.ts b/src/components/nav/nav-controller.ts index b0039b7716..dd1a6e18ec 100644 --- a/src/components/nav/nav-controller.ts +++ b/src/components/nav/nav-controller.ts @@ -169,7 +169,6 @@ export class NavController extends Ion { protected _sbEnabled: boolean; protected _ids: number = -1; protected _trnsDelay: any; - protected _trnsTime: number = 0; protected _views: ViewController[] = []; viewDidLoad: EventEmitter; @@ -205,6 +204,11 @@ export class NavController extends Ion { */ isPortal: boolean = false; + /** + * @private + */ + _trnsTime: number = 0; + constructor( parent: any, protected _app: App, @@ -1167,9 +1171,7 @@ export class NavController extends Ion { ev: opts.ev, }; - let transAnimation = Transition.createTransition(enteringView, - leavingView, - transitionOpts); + let transAnimation = this.createTransitionWrapper(enteringView, leavingView, transitionOpts); this._trans && this._trans.destroy(); this._trans = transAnimation; @@ -1179,17 +1181,25 @@ export class NavController extends Ion { transAnimation.duration(0); } - let keyboardDurationPadding = 0; - if ( this._keyboard.isOpen() ) { - // add XXms to the duration the app is disabled when the keyboard is open - keyboardDurationPadding = 600; + // check if a parent is transitioning and get the time that it ends + let parentTransitionEndTime = this._getLongestTrans(Date.now()); + if (parentTransitionEndTime > 0) { + // the parent is already transitioning and has disabled the app + // so just update the local transitioning information + let duration = parentTransitionEndTime - Date.now(); + this.setTransitioning(true, duration); + } else { + // this is the only active transition (for now), so disable the app + let keyboardDurationPadding = 0; + if (this._keyboard.isOpen()) { + // add XXms to the duration the app is disabled when the keyboard is open + keyboardDurationPadding = 600; + } + let duration = transAnimation.getDuration() + keyboardDurationPadding; + let enableApp = (duration < 64); + this._app.setEnabled(enableApp, duration); + this.setTransitioning(!enableApp, duration); } - let duration = transAnimation.getDuration() + keyboardDurationPadding; - let enableApp = (duration < 64); - // block any clicks during the transition and provide a - // fallback to remove the clickblock if something goes wrong - this._app.setEnabled(enableApp, duration); - this.setTransitioning(!enableApp, duration); if (enteringView.viewType) { transAnimation.before.addClass(enteringView.viewType); @@ -1333,8 +1343,14 @@ export class NavController extends Ion { enteringView.state = STATE_INACTIVE; } - // allow clicks and enable the app again - this._app && this._app.setEnabled(true); + // check if there is a parent actively transitioning + let transitionEndTime = this._getLongestTrans(Date.now()); + // if transitionEndTime is greater than 0, there is a parent transition occurring + // so delegate enabling the app to the parent. If it <= 0, go ahead and enable the app + if (transitionEndTime <= 0) { + this._app && this._app.setEnabled(true); + } + this.setTransitioning(false); if (direction !== null && hasCompleted && !this.isPortal) { @@ -1372,6 +1388,16 @@ export class NavController extends Ion { } } + /** + *@private + * This method is just a wrapper to the Transition function of same name + * to make it easy/possible to mock the method call by overriding the function. + * In testing we don't want to actually do the animation, we want to return a stub instead + */ + private createTransitionWrapper(enteringView: ViewController, leavingView: ViewController, transitionOpts: any) { + return Transition.createTransition(enteringView, leavingView, transitionOpts); + } + private _cleanup() { // ok, cleanup time!! Destroy all of the views that are // INACTIVE and come after the active view @@ -1648,6 +1674,26 @@ export class NavController extends Ion { this._trnsTime = (isTransitioning ? Date.now() + fallback : 0); } + /** + * @private + * This method traverses the tree of parents upwards + * and looks at the time the transition ends (if it's transitioning) + * and returns the value that is the furthest into the future + * thus giving us the longest transition duration + */ + private _getLongestTrans(now: number) { + let parentNav: NavController = this.parent; + let transitionEndTime: number = -1; + while (parentNav) { + if (parentNav._trnsTime > transitionEndTime) { + transitionEndTime = parentNav._trnsTime; + } + parentNav = parentNav.parent; + } + // only check if the transitionTime is greater than the current time once + return transitionEndTime > 0 && transitionEndTime > now ? transitionEndTime : 0; + } + /** * @private */ diff --git a/src/components/nav/test/child-navs/e2e.ts b/src/components/nav/test/child-navs/e2e.ts new file mode 100644 index 0000000000..e1141e2d68 --- /dev/null +++ b/src/components/nav/test/child-navs/e2e.ts @@ -0,0 +1,3 @@ +it('should go to page and open child navs', function() { + element(by.css('.nested-children-test')).click(); +}); diff --git a/src/components/nav/test/child-navs/index.ts b/src/components/nav/test/child-navs/index.ts new file mode 100644 index 0000000000..e6c7376a13 --- /dev/null +++ b/src/components/nav/test/child-navs/index.ts @@ -0,0 +1,113 @@ +import {Component} from '@angular/core'; +import {ionicBootstrap, NavController} from '../../../../../src'; + +@Component({ + template: ``, +}) +class E2EApp { + root = LandingPage; +} + +ionicBootstrap(E2EApp); + +@Component({ + template: ` + + + Landing Page Comp + + + + + + + ` +}) +class LandingPage{ + + constructor(private _navController: NavController){ + } + + goToPage(){ + this._navController.push(FirstPage); + } +} + +@Component({ + template: ` + + + First Page Comp + + + + +

Sub Header First Page

+ + + ` +}) +class FirstPage{ + root = SecondPage; +} + +@Component({ + template: ` + + + Second Page Comp + + + + +

Sub Header Second Page

+ + + ` +}) +class SecondPage{ + root = ThirdPage; +} + +@Component({ + template: ` + + + Third Page Comp + + + + +

Sub Header Third Page

+ + + ` +}) +class ThirdPage{ + root = FourthPage; +} + +@Component({ + template: ` + + + + {{item}} + + + + ` +}) +class FourthPage{ + private items: string[]; + + ionViewWillEnter(){ + let items: string[] = []; + for ( let i = 0 ; i < 500; i++ ){ + items.push(`Item ${(i + 1)}`); + } + this.items = items; + } +} diff --git a/src/components/nav/test/nav-controller.spec.ts b/src/components/nav/test/nav-controller.spec.ts index 662df71e65..cda4e631d5 100644 --- a/src/components/nav/test/nav-controller.spec.ts +++ b/src/components/nav/test/nav-controller.spec.ts @@ -978,6 +978,52 @@ export function run() { expect(view4.domShow).toHaveBeenCalled(); }); + it('should re-enable the app when transition time <= 0', () => { + // arrange + let enteringView = new ViewController(Page1); + enteringView.state = 'somethingelse'; + let leavingView = new ViewController(Page2); + leavingView.state = 'somethingelse'; + nav._transIds = 1; + nav._app = { + setEnabled: () => {} + }; + + spyOn(nav._app, 'setEnabled'); + spyOn(nav, 'setTransitioning'); + + // act + nav._transFinish(nav._transIds, enteringView, leavingView, 'forward', true); + + // assert + expect(nav._app.setEnabled).toHaveBeenCalledWith(true); + expect(nav.setTransitioning).toHaveBeenCalledWith(false); + }); + + it('should not re-enable app when transition time > 0', () => { + // arrange + let enteringView = new ViewController(Page1); + enteringView.state = 'somethingelse'; + let leavingView = new ViewController(Page2); + leavingView.state = 'somethingelse'; + nav._transIds = 1; + nav._app = { + setEnabled: () => {} + }; + + spyOn(nav._app, 'setEnabled'); + spyOn(nav, 'setTransitioning'); + + nav._getLongestTrans = () => { return 50 }; + + // act + nav._transFinish(nav._transIds, enteringView, leavingView, 'forward', true); + + // assert + expect(nav._app.setEnabled).not.toHaveBeenCalled(); + expect(nav.setTransitioning).toHaveBeenCalledWith(false); + }); + }); describe('_insert', () => { @@ -1198,108 +1244,402 @@ export function run() { expect(nav._portal.length()).toBe(0); expect(nav.length()).toBe(1); }); - }); - it('should getActive()', () => { - expect(nav.getActive()).toBe(null); - let view1 = new ViewController(Page1); - view1.state = STATE_INIT_ENTER; - nav.views = [view1]; - expect(nav.getActive()).toBe(null); - view1.state = STATE_ACTIVE; - expect(nav.getActive()).toBe(view1); + describe('getActive', () => { + it('should getActive()', () => { + expect(nav.getActive()).toBe(null); + let view1 = new ViewController(Page1); + view1.state = STATE_INIT_ENTER; + nav.views = [view1]; + expect(nav.getActive()).toBe(null); + view1.state = STATE_ACTIVE; + expect(nav.getActive()).toBe(view1); + }); }); - it('should getByState()', () => { - expect(nav.getByState(null)).toBe(null); + describe('getByState', () => { + it('should getByState()', () => { + expect(nav.getByState(null)).toBe(null); - let view1 = new ViewController(Page1); - view1.state = STATE_INIT_ENTER; - let view2 = new ViewController(Page2); - view2.state = STATE_INIT_ENTER; - nav.views = [view1, view2]; + let view1 = new ViewController(Page1); + view1.state = STATE_INIT_ENTER; + let view2 = new ViewController(Page2); + view2.state = STATE_INIT_ENTER; + nav.views = [view1, view2]; - expect(nav.getByState('whatever')).toBe(null); - expect(nav.getByState(STATE_INIT_ENTER)).toBe(view2); + expect(nav.getByState('whatever')).toBe(null); + expect(nav.getByState(STATE_INIT_ENTER)).toBe(view2); - view2.state = STATE_INACTIVE; - expect(nav.getByState(STATE_INIT_ENTER)).toBe(view1); + view2.state = STATE_INACTIVE; + expect(nav.getByState(STATE_INIT_ENTER)).toBe(view1); - view2.state = STATE_ACTIVE; - expect(nav.getActive()).toBe(view2); + view2.state = STATE_ACTIVE; + expect(nav.getActive()).toBe(view2); + }); }); - it('should getPrevious()', () => { - expect(nav.getPrevious(null)).toBe(null); + describe('getPrevious', () => { + it('should getPrevious()', () => { + expect(nav.getPrevious(null)).toBe(null); - let view1 = new ViewController(Page1); - let view2 = new ViewController(Page2); - nav.views = [view1, view2]; + let view1 = new ViewController(Page1); + let view2 = new ViewController(Page2); + nav.views = [view1, view2]; - expect(nav.getPrevious(view1)).toBe(null); - expect(nav.getPrevious(view2)).toBe(view1); + expect(nav.getPrevious(view1)).toBe(null); + expect(nav.getPrevious(view2)).toBe(view1); + }); }); - it('should get first()', () => { - expect(nav.first()).toBe(null); - let view1 = new ViewController(Page1); - view1.setNav(nav); - let view2 = new ViewController(Page2); - view2.setNav(nav); - nav.views = [view1]; + describe('first', () => { + it('should get first()', () => { + expect(nav.first()).toBe(null); + let view1 = new ViewController(Page1); + view1.setNav(nav); + let view2 = new ViewController(Page2); + view2.setNav(nav); + nav.views = [view1]; - expect(nav.first()).toBe(view1); - expect(view1.isFirst()).toBe(true); + expect(nav.first()).toBe(view1); + expect(view1.isFirst()).toBe(true); - nav.views = [view1, view2]; - expect(nav.first()).toBe(view1); - expect(view1.isFirst()).toBe(true); - expect(view2.isFirst()).toBe(false); + nav.views = [view1, view2]; + expect(nav.first()).toBe(view1); + expect(view1.isFirst()).toBe(true); + expect(view2.isFirst()).toBe(false); + }); }); - it('should get last()', () => { - expect(nav.last()).toBe(null); - let view1 = new ViewController(Page1); - view1.setNav(nav); - let view2 = new ViewController(Page2); - view2.setNav(nav); - nav.views = [view1]; + describe('last', () => { + it('should get last()', () => { + expect(nav.last()).toBe(null); + let view1 = new ViewController(Page1); + view1.setNav(nav); + let view2 = new ViewController(Page2); + view2.setNav(nav); + nav.views = [view1]; - expect(nav.last()).toBe(view1); - expect(view1.isLast()).toBe(true); + expect(nav.last()).toBe(view1); + expect(view1.isLast()).toBe(true); - nav.views = [view1, view2]; - expect(nav.last()).toBe(view2); - expect(view1.isLast()).toBe(false); - expect(view2.isLast()).toBe(true); + nav.views = [view1, view2]; + expect(nav.last()).toBe(view2); + expect(view1.isLast()).toBe(false); + expect(view2.isLast()).toBe(true); + }); }); - it('should get indexOf()', () => { - let view1 = new ViewController(Page1); - let view2 = new ViewController(Page2); + describe('indexOf', () => { + it('should get indexOf()', () => { + let view1 = new ViewController(Page1); + let view2 = new ViewController(Page2); - expect(nav.length()).toBe(0); - expect(nav.indexOf(view1)).toBe(-1); + expect(nav.length()).toBe(0); + expect(nav.indexOf(view1)).toBe(-1); - nav.views = [view1, view2]; - expect(nav.indexOf(view1)).toBe(0); - expect(nav.indexOf(view2)).toBe(1); - expect(nav.length()).toBe(2); + nav.views = [view1, view2]; + expect(nav.indexOf(view1)).toBe(0); + expect(nav.indexOf(view2)).toBe(1); + expect(nav.length()).toBe(2); + }); }); - it('should get getByIndex()', () => { - expect(nav.getByIndex(-99)).toBe(null); - expect(nav.getByIndex(99)).toBe(null); + describe('getByIndex', () => { + it('should get getByIndex()', () => { + expect(nav.getByIndex(-99)).toBe(null); + expect(nav.getByIndex(99)).toBe(null); - let view1 = new ViewController(Page1); - let view2 = new ViewController(Page2); - nav.views = [view1, view2]; + let view1 = new ViewController(Page1); + let view2 = new ViewController(Page2); + nav.views = [view1, view2]; - expect(nav.getByIndex(-1)).toBe(null); - expect(nav.getByIndex(0)).toBe(view1); - expect(nav.getByIndex(1)).toBe(view2); - expect(nav.getByIndex(2)).toBe(null); + expect(nav.getByIndex(-1)).toBe(null); + expect(nav.getByIndex(0)).toBe(view1); + expect(nav.getByIndex(1)).toBe(view2); + expect(nav.getByIndex(2)).toBe(null); + }); + }); + + /* private method */ + describe('_beforeTrans', () => { + + it('shouldnt disable app on short transition', () => { + // arrange + let executeAssertions = () => { + // assertions triggerd by callbacks + expect(app.setEnabled).toHaveBeenCalledWith(true, 50); + expect(nav.setTransitioning).toHaveBeenCalledWith(false, 50); + }; + let mockTransition = { + play: () => { + executeAssertions(); + }, + getDuration: () => { return 50}, + onFinish: () => {} + }; + nav.createTransitionWrapper = () => { + return mockTransition; + }; + nav.config = { + platform : { + isRTL: () => {} + } + }; + let app = { + setEnabled: () => {} + }; + nav._app = app; + + spyOn(app, 'setEnabled'); + spyOn(nav, 'setTransitioning'); + + let view1 = new ViewController(Page1); + let view2 = new ViewController(Page2); + + // act + nav._beforeTrans(view1, view2, {}, () => {}); + }); + + it('should disable app on longer transition', () => { + // arrange + let executeAssertions = () => { + // assertions triggerd by callbacks + expect(app.setEnabled).toHaveBeenCalledWith(false, 200); + expect(nav.setTransitioning).toHaveBeenCalledWith(true, 200); + }; + let mockTransition = { + play: () => { + executeAssertions(); + }, + getDuration: () => { return 200}, + onFinish: () => {} + }; + nav.createTransitionWrapper = () => { + return mockTransition; + }; + nav.config = { + platform : { + isRTL: () => {} + } + }; + let app = { + setEnabled: () => {} + }; + nav._app = app; + + spyOn(app, 'setEnabled'); + spyOn(nav, 'setTransitioning'); + + let view1 = new ViewController(Page1); + let view2 = new ViewController(Page2); + + // act + nav._beforeTrans(view1, view2, {}, () => {}); + }); + + it('should disable app w/ padding when keyboard is open', () => { + // arrange + let executeAssertions = () => { + // assertions triggerd by callbacks + expect(app.setEnabled.calls.mostRecent().args[0]).toEqual(false); + expect(app.setEnabled.calls.mostRecent().args[1]).toBeGreaterThan(200); + + expect(nav.setTransitioning.calls.mostRecent().args[0]).toEqual(true); + expect(nav.setTransitioning.calls.mostRecent().args[1]).toBeGreaterThan(200); + }; + let mockTransition = { + play: () => { + executeAssertions(); + }, + getDuration: () => { return 200}, + onFinish: () => {} + }; + nav.createTransitionWrapper = () => { + return mockTransition; + }; + nav.config = { + platform : { + isRTL: () => {} + } + }; + let app = { + setEnabled: () => {} + }; + nav._app = app; + nav._keyboard = { + isOpen: () => true + }; + + spyOn(app, 'setEnabled'); + spyOn(nav, 'setTransitioning'); + + let view1 = new ViewController(Page1); + let view2 = new ViewController(Page2); + + // act + nav._beforeTrans(view1, view2, {}, () => {}); + }); + + it('shouldnt update app enabled when parent transition is occurring', () => { + // arrange + let executeAssertions = () => { + // assertions triggerd by callbacks + expect(app.setEnabled).not.toHaveBeenCalled(); + expect(nav.setTransitioning.calls.mostRecent().args[0]).toEqual(true); + }; + let mockTransition = { + play: () => { + executeAssertions(); + }, + getDuration: () => { return 200}, + onFinish: () => {} + }; + nav.createTransitionWrapper = () => { + return mockTransition; + }; + nav.config = { + platform : { + isRTL: () => {} + } + }; + let app = { + setEnabled: () => {} + }; + nav._app = app; + + spyOn(app, 'setEnabled'); + spyOn(nav, 'setTransitioning'); + + nav._getLongestTrans = () => { return Date.now() + 100 }; + + let view1 = new ViewController(Page1); + let view2 = new ViewController(Page2); + + // act + nav._beforeTrans(view1, view2, {}, () => {}); + }); + }); + + /* private method */ + describe('_getLongestTrans', () => { + it('should return 0 when transition end time is less than 0', () => { + // arrange + nav.parent = null; + // act + let returnedValue = nav._getLongestTrans(Date.now()); + // asssert + expect(returnedValue).toEqual(0); + }); + + it('should return 0 when transition end time is less than now', () => { + // arrange + nav.parent = { + _trnsTime: Date.now() - 5 + }; + // act + let returnedValue = nav._getLongestTrans(Date.now()); + // asssert + expect(returnedValue).toEqual(0); + }); + + it('should return 0 when parent transition time not set', () => { + // arrange + nav.parent = { + _trnsTime: undefined + }; + // act + let returnedValue = nav._getLongestTrans(Date.now()); + // asssert + expect(returnedValue).toEqual(0); + }); + + it('should return transitionEndTime when transition end time is greater than now', () => { + // arrange + let expectedReturnValue = Date.now() + 100; + nav.parent = { + _trnsTime: expectedReturnValue + }; + // act + let returnedValue = nav._getLongestTrans(Date.now()); + // asssert + expect(returnedValue).toEqual(expectedReturnValue); + }); + + it('should return the greatest end of transition time if found on first parent', () => { + // arrange + let expectedReturnValue = Date.now() + 100; + let firstParent = { + _trnsTime: expectedReturnValue + }; + let secondParent = { + _trnsTime: Date.now() + 50 + }; + let thirdParent = { + _trnsTime: Date.now() + }; + let fourthParent = { + _trnsTime: Date.now() + 20 + }; + firstParent.parent = secondParent; + secondParent.parent = thirdParent; + thirdParent.parent = fourthParent; + nav.parent = firstParent; + // act + let returnedValue = nav._getLongestTrans(Date.now()); + // asssert + expect(returnedValue).toEqual(expectedReturnValue); + }); + + it('should return the greatest end of transition time if found on middle parent', () => { + // arrange + let expectedReturnValue = Date.now() + 100; + let firstParent = { + _trnsTime: Date.now() + }; + let secondParent = { + _trnsTime: Date.now() + 50 + }; + let thirdParent = { + _trnsTime: expectedReturnValue + }; + let fourthParent = { + _trnsTime: Date.now() + 20 + }; + firstParent.parent = secondParent; + secondParent.parent = thirdParent; + thirdParent.parent = fourthParent; + nav.parent = firstParent; + // act + let returnedValue = nav._getLongestTrans(Date.now()); + // asssert + expect(returnedValue).toEqual(expectedReturnValue); + }); + + it('should return the greatest end of transition time if found on last parent', () => { + // arrange + let expectedReturnValue = Date.now() + 100; + let firstParent = { + _trnsTime: Date.now() + }; + let secondParent = { + _trnsTime: Date.now() + 50 + }; + let thirdParent = { + _trnsTime: Date.now() + 20 + }; + let fourthParent = { + _trnsTime: expectedReturnValue + }; + firstParent.parent = secondParent; + secondParent.parent = thirdParent; + thirdParent.parent = fourthParent; + nav.parent = firstParent; + // act + let returnedValue = nav._getLongestTrans(Date.now()); + // asssert + expect(returnedValue).toEqual(expectedReturnValue); + }); }); // setup stuff diff --git a/src/components/tabs/test/child-navs/e2e.ts b/src/components/tabs/test/child-navs/e2e.ts new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/components/tabs/test/child-navs/index.ts b/src/components/tabs/test/child-navs/index.ts new file mode 100644 index 0000000000..09c93a0f69 --- /dev/null +++ b/src/components/tabs/test/child-navs/index.ts @@ -0,0 +1,140 @@ +import {Component} from '@angular/core'; +import {ionicBootstrap, NavController, App, Alert, Modal, ViewController, Tab, Tabs} from '../../../../../src'; + + +// +// Tab 1 +// +@Component({ + template: ` + + + Tab 1 + + + + + + + ` +}) +export class Tab1 { + root = SecondPage; +} + +// +// Tab 2 +// +@Component({ + template: ` + + + Tab 2 + + + + + + + ` +}) +export class Tab2 { + root = SecondPage; +} + +// +// Tab 3 +// +@Component({ + template: ` + + + Tab 3 + + + + + + + ` +}) +export class Tab3 { + root = SecondPage; +} + +@Component({ + template: ` + +
SecondPage Cmp
+ + + ` +}) +class SecondPage{ + root = ThirdPage; +} + +@Component({ + template: ` + +
ThirdPage Cmp
+ + + ` +}) +class ThirdPage{ + root = FourthPage; +} + +@Component({ + template: ` + + + Fourth Page Comp + + + + + + {{item}} + + + + ` +}) +class FourthPage{ + private items: string[]; + + ionViewWillEnter(){ + let items: string[] = []; + for ( let i = 0 ; i < 500; i++ ){ + items.push(`Item ${(i + 1)}`); + } + this.items = items; + } +} + + +@Component({ + template: ` + + + + + + ` +}) +export class TabsPage { + root1 = Tab1; + root2 = Tab2; + root3 = Tab3; +} + +@Component({ + template: `` +}) +export class E2EApp { + root = TabsPage; +} + +ionicBootstrap(E2EApp);