fix(router): eagerly update internal state on browser-triggered navig…

…ations (#43102)

The management of `browserUrlTree` currently has several problems with
correctly tracking the actual state of the browser.

This change makes the Router eagerly update the `browserUrlTree` when
handling navigations triggered by browser events (i.e., not 'imperative'). This
is because with those types of navigations, the browser URL bar is
_already_ updated. If we do not update the internal tracking of the
`browserUrlTree`, we will be out of sync with the real URL if the
navigation is rejected.

It would be best if we could remove `browserUrlTree` completely, but doing that
would require a lot more investigation and is blocked by #27059 because
the SpyLocation used in tests does not emulate real browser behavior.

fixes #43101

PR Close #43102

packages/core/test/bundling/router/bundle.golden_symbols.json

@@ -1526,6 +1526,9 @@
   {
     "name": "isArrayLike"
   },
+  {
+    "name": "isBrowserTriggeredNavigation"
+  },
   {
     "name": "isCommandWithOutlets"
   },

packages/router/src/router.ts

@@ -636,6 +636,12 @@ export class Router {
                            (this.onSameUrlNavigation === 'reload' ? true : urlTransition) &&
                            this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl);
 
+                       // If the source of the navigation is from a browser event, the URL is
+                       // already updated. We already need to sync the internal state.
+                       if (isBrowserTriggeredNavigation(t.source)) {
+                         this.browserUrlTree = t.rawUrl;
+                       }
+
                        if (processCurrentUrl) {
                          return of(t).pipe(
                              // Fire NavigationStart event
@@ -954,7 +960,12 @@ export class Router {
                                  this.urlHandlingStrategy.merge(e.url, this.rawUrlTree);
                              const extras = {
                                skipLocationChange: t.extras.skipLocationChange,
-                               replaceUrl: this.urlUpdateStrategy === 'eager'
+                               // The URL is already updated at this point if we have 'eager' URL
+                               // updates or if the navigation was triggered by the browser (back
+                               // button, URL bar, etc). We want to replace that item in history if
+                               // the navigation is rejected.
+                               replaceUrl: this.urlUpdateStrategy === 'eager' ||
+                                   isBrowserTriggeredNavigation(t.source)
                              };
 
                              this.scheduleNavigation(
@@ -1385,8 +1396,8 @@ export class Router {
     const lastNavigation = this.getTransition();
     // We don't want to skip duplicate successful navs if they're imperative because
     // onSameUrlNavigation could be 'reload' (so the duplicate is intended).
-    const browserNavPrecededByRouterNav =
-        source !== 'imperative' && lastNavigation?.source === 'imperative';
+    const browserNavPrecededByRouterNav = isBrowserTriggeredNavigation(source) && lastNavigation &&
+        !isBrowserTriggeredNavigation(lastNavigation.source);
     const lastNavigationSucceeded = this.lastSuccessfulId === lastNavigation.id;
     // If the last navigation succeeded or is in flight, we can use the rawUrl as the comparison.
     // However, if it failed, we should compare to the final result (urlAfterRedirects).
@@ -1549,3 +1560,7 @@ function validateCommands(commands: string[]): void {
     }
   }
 }
+
+function isBrowserTriggeredNavigation(source: 'imperative'|'popstate'|'hashchange') {
+  return source !== 'imperative';
+}

packages/router/test/integration.spec.ts

@@ -1230,7 +1230,11 @@ describe('Integration', () => {
          location.simulateHashChange('/blocked');
 
          advance(fixture);
-         expectEvents(recordedEvents, [[NavigationStart, '/blocked']]);
+         expectEvents(recordedEvents, [
+           [NavigationStart, '/blocked'],
+           [NavigationStart, '/simple'],
+           [NavigationEnd, '/simple'],
+         ]);
        }));
   });
 

packages/router/test/regression_integration.spec.ts

@@ -266,14 +266,9 @@ describe('Integration', () => {
          location.simulateHashChange('/one');
          advance(fixture);
 
-         const BASE_ERROR_MESSAGE =
-             'This asserts current behavior, which is incorrect. When #28561 is fixed, it should be: ';
-
-         expect(location.path()).toEqual('/one', BASE_ERROR_MESSAGE + '/');
-         const urlChanges = ['replace: /', 'hash: /one'];
-         expect(location.urlChanges)
-             .toEqual(
-                 urlChanges, BASE_ERROR_MESSAGE + JSON.stringify(urlChanges.concat('replace: /')));
+         expect(location.path()).toEqual('/');
+         const urlChanges = ['replace: /', 'hash: /one', 'replace: /'];
+         expect(location.urlChanges).toEqual(urlChanges);
        }));
   });
 });