Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(router): don't break history when CanDeactivate cancel back navigation #13922

Closed
wants to merge 1 commit into from
Closed

fix(router): don't break history when CanDeactivate cancel back navigation #13922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 29 additions & 18 deletions packages/router/src/router.ts
View file
Open in desktop
Expand Up @@ -530,14 +530,6 @@ export class Router {
Promise<boolean> {
const lastNavigation = this.navigations.value;

// 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()) {
return null; // return value is not used
}

// Because of a bug in IE and Edge, the location class fires two events (popstate and
// hashchange) every single time. The second one should be ignored. Otherwise, the URL will
// flicker.
Expand All @@ -562,8 +554,8 @@ export class Router {
return promise.catch((e: any) => Promise.reject(e));
}

private executeScheduledNavigation({id, rawUrl, extras, resolve, reject}: NavigationParams):
void {
private executeScheduledNavigation({id, source, rawUrl, extras, resolve,
reject}: NavigationParams): void {
const url = this.urlHandlingStrategy.extract(rawUrl);
const urlTransition = !this.navigated || url.toString() !== this.currentUrlTree.toString();

Expand All @@ -572,7 +564,7 @@ export class Router {
Promise.resolve()
.then(
(_) => this.runNavigate(
url, rawUrl, extras.skipLocationChange, extras.replaceUrl, id, null))
url, rawUrl, extras.skipLocationChange, extras.replaceUrl, id, null, source))
.then(resolve, reject);

// we cannot process the current URL, but we could process the previous one =>
Expand All @@ -585,7 +577,7 @@ export class Router {
.then(
(_) => this.runNavigate(
url, rawUrl, false, false, id,
createEmptyState(url, this.rootComponentType).snapshot))
createEmptyState(url, this.rootComponentType).snapshot, source))
.then(resolve, reject);

} else {
Expand All @@ -596,7 +588,8 @@ export class Router {

private runNavigate(
url: UrlTree, rawUrl: UrlTree, shouldPreventPushState: boolean, shouldReplaceUrl: boolean,
id: number, precreatedState: RouterStateSnapshot): Promise<boolean> {
id: number, precreatedState: RouterStateSnapshot,
source: NavigationSource): Promise<boolean> {
if (id !== this.navigationId) {
this.location.go(this.urlSerializer.serialize(this.currentUrlTree));
this.routerEvents.next(new NavigationCancel(
Expand Down Expand Up @@ -726,8 +719,22 @@ export class Router {
id, this.serializeUrl(url), this.serializeUrl(this.currentUrlTree)));
resolvePromise(true);
} else {
this.resetUrlToCurrentUrlTree();
this.routerEvents.next(new NavigationCancel(id, this.serializeUrl(url), ''));
const navigatedUrl: string = this.serializeUrl(url);
// When navigation is canceled and it was initiated by the click on Back button,
// browser history is 1 item above the actual state. So we need to call pushState
// instead of replaceState to restore the correct history. We can detect click
// on Back button when:
// 1. Source of navigation is browser event
// 2. Url is equal to the previous url
if (source === 'imperative') {
this.resetUrlToCurrentUrlTree();
} else {
const prevUrl: string =
this.urlSerializer.serialize(this.navigations.value.rawUrl);
const pushState: boolean = navigatedUrl === prevUrl;
this.resetUrlToCurrentUrlTree(pushState);
}
this.routerEvents.next(new NavigationCancel(id, navigatedUrl, ''));
resolvePromise(false);
}
},
Expand Down Expand Up @@ -755,9 +762,13 @@ export class Router {
});
}

private resetUrlToCurrentUrlTree(): void {
const path = this.urlSerializer.serialize(this.rawUrlTree);
this.location.replaceState(path);
private resetUrlToCurrentUrlTree(pushState: boolean = false): void {
const currentUrl: string = this.urlSerializer.serialize(this.rawUrlTree);
if (pushState) {
this.location.go(currentUrl);
} else {
this.location.replaceState(currentUrl);
}
}
}

Expand Down
127 changes: 124 additions & 3 deletions packages/router/test/integration.spec.ts
View file
Open in desktop
Expand Up @@ -241,8 +241,7 @@ describe('Integration', () => {

let recordedError: any = null;
router.navigateByUrl('/blank').catch(e => recordedError = e);
advance(fixture);

expect(() => advance(fixture)).toThrow();
expect(recordedError.message).toEqual('Cannot find primary outlet to load \'BlankCmp\'');
}));

Expand Down Expand Up @@ -2018,7 +2017,6 @@ describe('Integration', () => {
})));
});


describe('should work when returns an observable', () => {
beforeEach(() => {
TestBed.configureTestingModule({
Expand Down Expand Up @@ -2046,6 +2044,129 @@ describe('Integration', () => {
expect(location.path()).toEqual('/team/22');
})));
});

describe('should not break the history', () => {

class CancelFirstNavigation implements CanDeactivate<any> {
private firstTime: boolean = true;
canDeactivate(): boolean {
if (this.firstTime) {
this.firstTime = false;
return false;
}
return true;
}
}

@Component({selector: 'parent', template: '<router-outlet></router-outlet>'})
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 {
}

@NgModule({
declarations: [Parent, Home, Child1, Child2, Child3],
entryComponents: [Child1, Child2, Child3],
imports: [RouterModule]
})
class TestModule {
}

beforeEach(() => {
TestBed.configureTestingModule(
{providers: [CancelFirstNavigation], imports: [TestModule]});
});

it('when navigate back using Back button',
fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = createRoot(router, Parent);

router.resetConfig([
{path: '', component: Home}, {path: 'first', component: Child1},
{path: 'second', component: Child2},
{path: 'third', component: Child3, canDeactivate: [CancelFirstNavigation]}
]);

router.navigateByUrl('/first');
advance(fixture);
expect(location.path()).toEqual('/first');
expect(fixture.nativeElement).toHaveText('child1');

router.navigateByUrl('/second');
advance(fixture);
expect(location.path()).toEqual('/second');
expect(fixture.nativeElement).toHaveText('child2');

router.navigateByUrl('/third');
advance(fixture);
expect(location.path()).toEqual('/third');
expect(fixture.nativeElement).toHaveText('child3');

// CanDeactivate false
location.back();
advance(fixture);
expect(location.path()).toEqual('/third');
expect(fixture.nativeElement).toHaveText('child3');

// CanDeactivate true
location.back();
advance(fixture);
expect(location.path()).toEqual('/second');
expect(fixture.nativeElement).toHaveText('child2');
})));

it('when navigate back imperatively',
fakeAsync(inject([Router, Location], (router: Router, location: Location) => {
const fixture = createRoot(router, Parent);

router.resetConfig([
{path: '', component: Home}, {path: 'first', component: Child1},
{path: 'second', component: Child2},
{path: 'third', component: Child3, canDeactivate: [CancelFirstNavigation]}
]);

router.navigateByUrl('/first');
advance(fixture);
expect(location.path()).toEqual('/first');
expect(fixture.nativeElement).toHaveText('child1');

router.navigateByUrl('/second');
advance(fixture);
expect(location.path()).toEqual('/second');
expect(fixture.nativeElement).toHaveText('child2');

router.navigateByUrl('/third');
advance(fixture);
expect(location.path()).toEqual('/third');
expect(fixture.nativeElement).toHaveText('child3');

// CanDeactivate false
router.navigateByUrl('/second');
advance(fixture);
expect(location.path()).toEqual('/third');
expect(fixture.nativeElement).toHaveText('child3');

// CanDeactivate true
location.back();
advance(fixture);
expect(location.path()).toEqual('/second');
expect(fixture.nativeElement).toHaveText('child2');
})));
});
});

describe('CanActivateChild', () => {
Expand Down