From 1f055b90b65cce2d0d063ed44cb0f8fbecb9b1f6 Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Thu, 10 Nov 2022 15:53:29 -0800
Subject: [PATCH] fix(router): Ensure anchor scrolling happens on ignored same
 URL navigations (#48025)

The Router scroller only listens for NavigationEnd events. However, the
default behavior of the Router is to ignore navigations to the same URL.
This breaks the anchor scrolling when clicking on an anchor whose
fragment is already in the URL.

fixes #29099

BREAKING CHANGE: The `Scroll` event's `routerEvent` property may also be
a `NavigationSkipped` event. Previously, it was only a `NavigationEnd`
event.

PR Close #48025
---
 goldens/public-api/router/index.md     |  4 ++--
 packages/router/src/events.ts          |  2 +-
 packages/router/src/router_scroller.ts | 12 +++++++++---
 packages/router/test/bootstrap.spec.ts | 15 +++++++++------
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/goldens/public-api/router/index.md b/goldens/public-api/router/index.md
index 452fca2902e..d688e7a3076 100644
--- a/goldens/public-api/router/index.md
+++ b/goldens/public-api/router/index.md
@@ -950,7 +950,7 @@ export type RunGuardsAndResolvers = 'pathParamsChange' | 'pathParamsOrQueryParam
 // @public
 export class Scroll {
     constructor(
-    routerEvent: NavigationEnd,
+    routerEvent: NavigationEnd | NavigationSkipped,
     position: [number, number] | null,
     anchor: string | null);
     // (undocumented)
@@ -958,7 +958,7 @@ export class Scroll {
     // (undocumented)
     readonly position: [number, number] | null;
     // (undocumented)
-    readonly routerEvent: NavigationEnd;
+    readonly routerEvent: NavigationEnd | NavigationSkipped;
     // (undocumented)
     toString(): string;
     // (undocumented)
diff --git a/packages/router/src/events.ts b/packages/router/src/events.ts
index 4f4062073b9..37b3d7d9a77 100644
--- a/packages/router/src/events.ts
+++ b/packages/router/src/events.ts
@@ -582,7 +582,7 @@ export class Scroll {
 
   constructor(
       /** @docsNotRequired */
-      readonly routerEvent: NavigationEnd,
+      readonly routerEvent: NavigationEnd|NavigationSkipped,
 
       /** @docsNotRequired */
       readonly position: [number, number]|null,
diff --git a/packages/router/src/router_scroller.ts b/packages/router/src/router_scroller.ts
index aaeb6e0c6bd..7a6428f0231 100644
--- a/packages/router/src/router_scroller.ts
+++ b/packages/router/src/router_scroller.ts
@@ -10,9 +10,8 @@ import {ViewportScroller} from '@angular/common';
 import {Injectable, InjectionToken, NgZone, OnDestroy} from '@angular/core';
 import {Unsubscribable} from 'rxjs';
 
-import {NavigationEnd, NavigationStart, Scroll} from './events';
+import {NavigationEnd, NavigationSkipped, NavigationSkippedCode, NavigationStart, Scroll} from './events';
 import {NavigationTransitions} from './navigation_transition';
-import {Router} from './router';
 import {UrlSerializer} from './url_tree';
 
 export const ROUTER_SCROLLER = new InjectionToken<RouterScroller>('');
@@ -61,6 +60,12 @@ export class RouterScroller implements OnDestroy {
       } else if (e instanceof NavigationEnd) {
         this.lastId = e.id;
         this.scheduleScrollEvent(e, this.urlSerializer.parse(e.urlAfterRedirects).fragment);
+      } else if (
+          e instanceof NavigationSkipped &&
+          e.code === NavigationSkippedCode.IgnoredSameUrlNavigation) {
+        this.lastSource = undefined;
+        this.restoredId = 0;
+        this.scheduleScrollEvent(e, this.urlSerializer.parse(e.url).fragment);
       }
     });
   }
@@ -86,7 +91,8 @@ export class RouterScroller implements OnDestroy {
     });
   }
 
-  private scheduleScrollEvent(routerEvent: NavigationEnd, anchor: string|null): void {
+  private scheduleScrollEvent(routerEvent: NavigationEnd|NavigationSkipped, anchor: string|null):
+      void {
     this.zone.runOutsideAngular(() => {
       // The scroll event needs to be delayed until after change detection. Otherwise, we may
       // attempt to restore the scroll position before the router outlet has fully rendered the
diff --git a/packages/router/test/bootstrap.spec.ts b/packages/router/test/bootstrap.spec.ts
index e628264b852..9a023ed367f 100644
--- a/packages/router/test/bootstrap.spec.ts
+++ b/packages/router/test/bootstrap.spec.ts
@@ -455,7 +455,7 @@ describe('bootstrap', () => {
               scrollPositionRestoration: 'enabled',
               anchorScrolling: 'enabled',
               scrollOffset: [0, 100],
-              onSameUrlNavigation: 'reload'
+              onSameUrlNavigation: 'ignore',
             })
       ],
       declarations: [TallComponent, RootCmp],
@@ -494,13 +494,16 @@ describe('bootstrap', () => {
 
     await router.navigateByUrl('/aa#marker2');
     await resolveAfter(100);
-    expect(window.pageYOffset).toBeGreaterThanOrEqual(5900);
-    expect(window.pageYOffset).toBeLessThan(6000);  // offset
+    expect(window.scrollY).toBeGreaterThanOrEqual(5900);
+    expect(window.scrollY).toBeLessThan(6000);  // offset
 
-    await router.navigateByUrl('/aa#marker3');
+    // Scroll somewhere else, then navigate to the hash again. Even though the same url navigation
+    // is ignored by the Router, we should still scroll.
+    window.scrollTo(0, 3000);
+    await router.navigateByUrl('/aa#marker2');
     await resolveAfter(100);
-    expect(window.pageYOffset).toBeGreaterThanOrEqual(8900);
-    expect(window.pageYOffset).toBeLessThan(9000);
+    expect(window.scrollY).toBeGreaterThanOrEqual(5900);
+    expect(window.scrollY).toBeLessThan(6000);  // offset
   });
 
   it('should cleanup "popstate" and "hashchange" listeners', async () => {