From 5064dc75acbd86c0afe23d6ef46117ca65470b47 Mon Sep 17 00:00:00 2001
From: Alison Gale <agale753@gmail.com>
Date: Wed, 7 Aug 2019 06:52:04 -0700
Subject: [PATCH] fix(common): update $locationShim to notify onChange
 listeners before emitting AngularJS events (#32037)

The $locationShim has onChange listeners to allow for synchronization logic between
AngularJS and Angular. When the AngularJS routing events are emitted first, this can
cause Angular code to be out of sync. Notifying the listeners earlier solves the
problem.

PR Close #32037
---
 packages/common/upgrade/src/location_shim.ts |  5 ++-
 packages/common/upgrade/test/upgrade.spec.ts | 47 +++++++++++++++-----
 2 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/packages/common/upgrade/src/location_shim.ts b/packages/common/upgrade/src/location_shim.ts
index b9e7cda766..7f03b46c0b 100644
--- a/packages/common/upgrade/src/location_shim.ts
+++ b/packages/common/upgrade/src/location_shim.ts
@@ -144,6 +144,7 @@ export class $locationShim {
         this.$$parse(oldUrl);
         this.state(oldState);
         this.setBrowserUrlWithFallback(oldUrl, false, oldState);
+        this.$$notifyChangeListeners(this.url(), this.$$state, oldUrl, oldState);
       } else {
         this.initalizing = false;
         $rootScope.$broadcast('$locationChangeSuccess', newUrl, oldUrl, newState, oldState);
@@ -199,6 +200,9 @@ export class $locationShim {
               }
               $rootScope.$broadcast(
                   '$locationChangeSuccess', newUrl, oldUrl, this.$$state, oldState);
+              if (urlOrStateChanged) {
+                this.$$notifyChangeListeners(this.url(), this.$$state, oldUrl, oldState);
+              }
             }
           });
         }
@@ -415,7 +419,6 @@ export class $locationShim {
       // state object; this makes possible quick checking if the state changed in the digest
       // loop. Checking deep equality would be too expensive.
       this.$$state = this.browserState();
-      this.$$notifyChangeListeners(url, state, oldUrl, oldState);
     } catch (e) {
       // Restore old values if pushState fails
       this.url(oldUrl);
diff --git a/packages/common/upgrade/test/upgrade.spec.ts b/packages/common/upgrade/test/upgrade.spec.ts
index 14d422fa2b..919450d8e6 100644
--- a/packages/common/upgrade/test/upgrade.spec.ts
+++ b/packages/common/upgrade/test/upgrade.spec.ts
@@ -628,6 +628,7 @@ describe('$location.onChange()', () => {
 
   let $location: $locationShim;
   let upgradeModule: UpgradeModule;
+  let mock$rootScope: $rootScopeMock;
 
   beforeEach(() => {
     TestBed.configureTestingModule({
@@ -640,6 +641,7 @@ describe('$location.onChange()', () => {
 
     upgradeModule = TestBed.get(UpgradeModule);
     upgradeModule.$injector = {get: injectorFactory()};
+    mock$rootScope = upgradeModule.$injector.get('$rootScope');
   });
 
   beforeEach(inject([$locationShim], (loc: $locationShim) => { $location = loc; }));
@@ -674,17 +676,44 @@ describe('$location.onChange()', () => {
 
     $location.onChange(changeListener);
 
-    // Mock out setting browserUrl
-    ($location as any).browserUrl = (url: string, replace: boolean, state: unknown) => {};
-
     const newState = {foo: 'bar'};
-    ($location as any).setBrowserUrlWithFallback('/newUrl', false, newState);
+    $location.state(newState);
+    $location.path('/newUrl');
+    mock$rootScope.runWatchers();
+
     expect(onChangeVals.url).toBe('/newUrl');
-    expect(onChangeVals.state).toBe(newState);
-    expect(onChangeVals.oldUrl).toBe('/');
+    expect(onChangeVals.state).toEqual(newState);
+    expect(onChangeVals.oldUrl).toBe('http://host.com');
     expect(onChangeVals.oldState).toBe(null);
   });
 
+  it('should call changeListeners after $locationChangeSuccess', () => {
+
+    let changeListenerCalled = false;
+    let locationChangeSuccessEmitted = false;
+
+    function changeListener(url: string, state: unknown, oldUrl: string, oldState: unknown) {
+      changeListenerCalled = true;
+    }
+
+    $location.onChange(changeListener);
+
+    mock$rootScope.$on('$locationChangeSuccess', () => {
+      // Ensure that the changeListener hasn't been called yet
+      expect(changeListenerCalled).toBe(false);
+      locationChangeSuccessEmitted = true;
+    });
+
+    // Update state and run watchers
+    const stateValue = {foo: 'bar'};
+    $location.state(stateValue);
+    mock$rootScope.runWatchers();
+
+    // Ensure that change listeners are called and location events are emitted
+    expect(changeListenerCalled).toBe(true);
+    expect(locationChangeSuccessEmitted).toBe(true);
+  });
+
   it('should call forward errors to error handler', () => {
 
     let error !: Error;
@@ -696,10 +725,8 @@ describe('$location.onChange()', () => {
 
     $location.onChange(changeListener, errorHandler);
 
-    // Mock out setting browserUrl
-    ($location as any).browserUrl = (url: string, replace: boolean, state: unknown) => {};
-
-    ($location as any).setBrowserUrlWithFallback('/newUrl');
+    $location.url('/newUrl');
+    mock$rootScope.runWatchers();
     expect(error.message).toBe('Handle error');
   });