diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 6ccfd0bf9f..7e9bb3f2d3 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -1127,11 +1127,20 @@ export class Router { extras: NavigationExtras, priorPromise?: {resolve: any, reject: any, promise: Promise}): Promise { const lastNavigation = this.getTransition(); - // If the user triggers a navigation imperatively (e.g., by using navigateByUrl), - // and that navigation results in 'replaceState' that leads to the same URL, - // we should skip those. - if (lastNavigation && source !== 'imperative' && lastNavigation.source === 'imperative' && - lastNavigation.rawUrl.toString() === rawUrl.toString()) { + // * Imperative navigations (router.navigate) might trigger additional navigations to the same + // URL via a popstate event and the locationChangeListener. We should skip these duplicate + // navs. + // * Imperative navigations can be cancelled by router guards, meaning the URL won't change. If + // the user follows that with a navigation using the back/forward button or manual URL change, + // the destination may be the same as the previous imperative attempt. We should not skip + // these navigations because it's a separate case from the one above -- it's not a duplicate + // navigation. + // Note that imperative navs might only trigger a popstate in tests because the + // SpyLocation triggers it on replaceState. Real browsers don't; see #27059. + const navigationToSameUrl = lastNavigation.urlAfterRedirects.toString() === rawUrl.toString(); + const browserNavPrecededByRouterNav = + lastNavigation && source !== 'imperative' && lastNavigation.source === 'imperative'; + if (browserNavPrecededByRouterNav && navigationToSameUrl) { return Promise.resolve(true); // return value is not used } diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 626498c1b2..7ffe138188 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -2539,6 +2539,161 @@ describe('Integration', () => { }))); }); + describe('should not break the history', () => { + @Injectable({providedIn: 'root'}) + class MyGuard implements CanDeactivate { + allow: boolean = true; + canDeactivate(): boolean { + return this.allow; + } + } + + @Component({selector: 'parent', template: ''}) + class Parent { + } + + @Component({selector: 'home', template: 'home'}) + class Home { + } + + @Component({selector: 'child1', template: 'child1'}) + class Child1 { + } + + @Component({selector: 'child2', template: 'child2'}) + class Child2 { + } + + @Component({selector: 'child3', template: 'child3'}) + class Child3 { + } + + @Component({selector: 'child4', template: 'child4'}) + class Child4 { + } + + @Component({selector: 'child5', template: 'child5'}) + class Child5 { + } + + @NgModule({ + declarations: [Parent, Home, Child1, Child2, Child3, Child4, Child5], + entryComponents: [Child1, Child2, Child3, Child4, Child5], + imports: [RouterModule] + }) + class TestModule { + } + + let fixture: ComponentFixture; + + beforeEach(fakeAsync(() => { + TestBed.configureTestingModule({imports: [TestModule]}); + const router = TestBed.get(Router); + const location = TestBed.get(Location); + fixture = createRoot(router, Parent); + + router.resetConfig([ + {path: '', component: Home}, + {path: 'first', component: Child1}, + {path: 'second', component: Child2}, + {path: 'third', component: Child3, canDeactivate: [MyGuard]}, + {path: 'fourth', component: Child4}, + {path: 'fifth', component: Child5}, + ]); + + // Create a navigation history of pages 1-5, and go back to 3 so that there is both + // back and forward history. + router.navigateByUrl('/first'); + advance(fixture); + router.navigateByUrl('/second'); + advance(fixture); + router.navigateByUrl('/third'); + advance(fixture); + router.navigateByUrl('/fourth'); + advance(fixture); + router.navigateByUrl('/fifth'); + advance(fixture); + location.back(); + advance(fixture); + location.back(); + advance(fixture); + })); + + // TODO(https://github.com/angular/angular/issues/13586) + // A fix to this requires much more design + xit('when navigate back using Back button', fakeAsync(() => { + const location = TestBed.get(Location); + expect(location.path()).toEqual('/third'); + + TestBed.get(MyGuard).allow = false; + location.back(); + advance(fixture); + expect(location.path()).toEqual('/third'); + expect(fixture.nativeElement).toHaveText('child3'); + + TestBed.get(MyGuard).allow = true; + location.back(); + advance(fixture); + expect(location.path()).toEqual('/second'); + expect(fixture.nativeElement).toHaveText('child2'); + })); + + it('when navigate back imperatively', fakeAsync(() => { + const router = TestBed.get(Router); + const location = TestBed.get(Location); + expect(location.path()).toEqual('/third'); + + TestBed.get(MyGuard).allow = false; + router.navigateByUrl('/second'); + advance(fixture); + expect(location.path()).toEqual('/third'); + expect(fixture.nativeElement).toHaveText('child3'); + + TestBed.get(MyGuard).allow = true; + location.back(); + advance(fixture); + expect(location.path()).toEqual('/second'); + expect(fixture.nativeElement).toHaveText('child2'); + })); + + // TODO(https://github.com/angular/angular/issues/13586) + // A fix to this requires much more design + xit('when navigate back using Foward button', fakeAsync(() => { + const location = TestBed.get(Location); + expect(location.path()).toEqual('/third'); + + TestBed.get(MyGuard).allow = false; + location.forward(); + advance(fixture); + expect(location.path()).toEqual('/third'); + expect(fixture.nativeElement).toHaveText('child3'); + + TestBed.get(MyGuard).allow = true; + location.forward(); + advance(fixture); + expect(location.path()).toEqual('/fourth'); + expect(fixture.nativeElement).toHaveText('child4'); + })); + + it('when navigate forward imperatively', fakeAsync(() => { + const router = TestBed.get(Router); + const location = TestBed.get(Location); + expect(location.path()).toEqual('/third'); + + TestBed.get(MyGuard).allow = false; + router.navigateByUrl('/fourth'); + advance(fixture); + expect(location.path()).toEqual('/third'); + expect(fixture.nativeElement).toHaveText('child3'); + + TestBed.get(MyGuard).allow = true; + location.forward(); + advance(fixture); + expect(location.path()).toEqual('/fourth'); + expect(fixture.nativeElement).toHaveText('child4'); + })); + }); + describe('should redirect when guard returns UrlTree', () => { beforeEach(() => TestBed.configureTestingModule({ providers: [