From 7bf7ec6d9c8296a206864bbb7310cebe8e4d7259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Tue, 28 Jul 2015 01:46:09 -0400 Subject: [PATCH] fix(router): ensure navigation via back button works The router will now navigate and respect the current address value accordingly whenever a popState event is handled. Closes #2201 --- modules/angular2/src/router/location.ts | 4 +- modules/angular2/src/router/router.ts | 21 +++--- modules/angular2/test/router/location_spec.ts | 4 +- .../test/router/router_integration_spec.ts | 70 ++++++++++++++++++- modules/angular2/test/router/router_spec.ts | 14 ++++ 5 files changed, 99 insertions(+), 14 deletions(-) diff --git a/modules/angular2/src/router/location.ts b/modules/angular2/src/router/location.ts index 8447b14a57..0757dd43a1 100644 --- a/modules/angular2/src/router/location.ts +++ b/modules/angular2/src/router/location.ts @@ -34,7 +34,9 @@ export class Location { this._platformStrategy.onPopState((_) => this._onPopState(_)); } - _onPopState(_): void { ObservableWrapper.callNext(this._subject, {'url': this.path()}); } + _onPopState(_): void { + ObservableWrapper.callNext(this._subject, {'url': this.path(), 'pop': true}); + } path(): string { return this.normalize(this._platformStrategy.path()); } diff --git a/modules/angular2/src/router/router.ts b/modules/angular2/src/router/router.ts index e3caae9269..ee6cdb1c12 100644 --- a/modules/angular2/src/router/router.ts +++ b/modules/angular2/src/router/router.ts @@ -100,7 +100,7 @@ export class Router { * If the given URL begins with a `/`, router will navigate absolutely. * If the given URL does not begin with `/`, the router will navigate relative to this component. */ - navigate(url: string): Promise { + navigate(url: string, _skipLocationChange: boolean = false): Promise { return this._currentNavigation = this._currentNavigation.then((_) => { this.lastNavigationAttempt = url; this._startNavigating(); @@ -117,7 +117,7 @@ export class Router { return this._canDeactivate(matchedInstruction) .then((result) => { if (result) { - return this.commit(matchedInstruction) + return this.commit(matchedInstruction, _skipLocationChange) .then((_) => { this._emitNavigationFinish(matchedInstruction.accumulatedUrl); return true; @@ -180,7 +180,7 @@ export class Router { /** * Updates this router and all descendant routers according to the given instruction */ - commit(instruction: Instruction): Promise { + commit(instruction: Instruction, _skipLocationChange: boolean = false): Promise { this._currentInstruction = instruction; if (isPresent(this._outlet)) { return this._outlet.commit(instruction); @@ -290,14 +290,17 @@ export class RootRouter extends Router { hostComponent: Type) { super(registry, pipeline, null, hostComponent); this._location = location; - this._location.subscribe((change) => this.navigate(change['url'])); + this._location.subscribe((change) => this.navigate(change['url'], isPresent(change['pop']))); this.registry.configFromComponent(hostComponent, true); this.navigate(location.path()); } - commit(instruction: Instruction): Promise { - return super.commit(instruction) - .then((_) => { this._location.go(instruction.accumulatedUrl); }); + commit(instruction: Instruction, _skipLocationChange: boolean = false): Promise { + var promise = super.commit(instruction); + if (!_skipLocationChange) { + promise = promise.then((_) => { this._location.go(instruction.accumulatedUrl); }); + } + return promise; } } @@ -308,9 +311,9 @@ class ChildRouter extends Router { } - navigate(url: string): Promise { + navigate(url: string, _skipLocationChange: boolean = false): Promise { // Delegate navigation to the root router - return this.parent.navigate(url); + return this.parent.navigate(url, _skipLocationChange); } } diff --git a/modules/angular2/test/router/location_spec.ts b/modules/angular2/test/router/location_spec.ts index 2273386a24..260a5e7d2a 100644 --- a/modules/angular2/test/router/location_spec.ts +++ b/modules/angular2/test/router/location_spec.ts @@ -88,6 +88,8 @@ export function main() { var locationStrategy = new MockLocationStrategy(); var location = new Location(locationStrategy); + function assertUrl(path) { expect(location.path()).toEqual(path); } + location.go('/ready'); assertUrl('/ready'); @@ -102,8 +104,6 @@ export function main() { location.back(); assertUrl('/ready'); - - function assertUrl(path) { expect(location.path()).toEqual(path); } }); }); } diff --git a/modules/angular2/test/router/router_integration_spec.ts b/modules/angular2/test/router/router_integration_spec.ts index b588f47db8..7b4402de40 100644 --- a/modules/angular2/test/router/router_integration_spec.ts +++ b/modules/angular2/test/router/router_integration_spec.ts @@ -27,7 +27,8 @@ import { RouteParams, Router, appBaseHrefToken, - routerDirectives + routerDirectives, + HashLocationStrategy } from 'angular2/router'; import {LocationStrategy} from 'angular2/src/router/location_strategy'; @@ -81,6 +82,57 @@ export function main() { })); }); + describe('back button app', () => { + beforeEachBindings(() => { return [bind(appComponentTypeToken).toValue(HierarchyAppCmp)]; }); + + it('should change the url without pushing a new history state for back navigations', + inject([AsyncTestCompleter, TestComponentBuilder], (async, tcb: TestComponentBuilder) => { + + tcb.createAsync(HierarchyAppCmp) + .then((rootTC) => { + var router = rootTC.componentInstance.router; + var position = 0; + var flipped = false; + var history = + [ + ['/parent/child', 'root { parent { hello } }', '/super-parent/child'], + ['/super-parent/child', 'root { super-parent { hello2 } }', '/parent/child'], + ['/parent/child', 'root { parent { hello } }', false] + ] + + router.subscribe((_) => { + var location = rootTC.componentInstance.location; + var element = rootTC.nativeElement; + var path = location.path(); + + var entry = history[position]; + + expect(path).toEqual(entry[0]); + expect(element).toHaveText(entry[1]); + + var nextUrl = entry[2]; + if (nextUrl == false) { + flipped = true; + } + + if (flipped && position == 0) { + async.done(); + return; + } + + position = position + (flipped ? -1 : 1); + if (flipped) { + location.back(); + } else { + router.navigate(nextUrl); + } + }); + + router.navigate(history[0][0]); + }); + })); + }); + describe('hierarchical app', () => { beforeEachBindings(() => { return [bind(appComponentTypeToken).toValue(HierarchyAppCmp)]; }); @@ -153,6 +205,11 @@ export function main() { class HelloCmp { } +@Component({selector: 'hello2-cmp'}) +@View({template: 'hello2'}) +class Hello2Cmp { +} + @Component({selector: 'app-cmp'}) @View({template: "outer { }", directives: routerDirectives}) @RouteConfig([new Route({path: '/', component: HelloCmp})]) @@ -166,9 +223,18 @@ class AppCmp { class ParentCmp { } +@Component({selector: 'super-parent-cmp'}) +@View({template: `super-parent { }`, directives: routerDirectives}) +@RouteConfig([new Route({path: '/child', component: Hello2Cmp})]) +class SuperParentCmp { +} + @Component({selector: 'app-cmp'}) @View({template: `root { }`, directives: routerDirectives}) -@RouteConfig([new Route({path: '/parent/...', component: ParentCmp})]) +@RouteConfig([ + new Route({path: '/parent/...', component: ParentCmp}), + new Route({path: '/super-parent/...', component: SuperParentCmp}) +]) class HierarchyAppCmp { constructor(public router: Router, public location: LocationStrategy) {} } diff --git a/modules/angular2/test/router/router_spec.ts b/modules/angular2/test/router/router_spec.ts index bacf063268..950e952ee2 100644 --- a/modules/angular2/test/router/router_spec.ts +++ b/modules/angular2/test/router/router_spec.ts @@ -76,6 +76,20 @@ export function main() { }); })); + it('should not push a history change on when navigate is called with skipUrlChange', + inject([AsyncTestCompleter], (async) => { + var outlet = makeDummyOutlet(); + + router.registerOutlet(outlet) + .then((_) => router.config([new Route({path: '/b', component: DummyComponent})])) + .then((_) => router.navigate('/b', true)) + .then((_) => { + expect(outlet.spy('commit')).toHaveBeenCalled(); + expect(location.urlChanges).toEqual([]); + async.done(); + }); + })); + it('should navigate after being configured', inject([AsyncTestCompleter], (async) => { var outlet = makeDummyOutlet();