fix(aio): AppComponent should scroll only once when location changes

aio/src/app/app.component.spec.ts

@@ -265,19 +265,49 @@ describe('AppComponent', () => {
     });
   });
 
-  describe('autoScrolling', () => {
+  describe('autoScrolling with AutoScrollService', () => {
-    it('should AutoScrollService.scroll when the url changes', () => {
+    let scrollService: AutoScrollService;
-      const scrollService: AutoScrollService = fixture.debugElement.injector.get(AutoScrollService);
+    let scrollSpy: jasmine.Spy;
-      spyOn(scrollService, 'scroll');
+
-      locationService.go('some/url#fragment');
+    beforeEach(() => {
-      expect(scrollService.scroll).toHaveBeenCalledWith();
+      scrollService = fixture.debugElement.injector.get(AutoScrollService);
+      scrollSpy = spyOn(scrollService, 'scroll');
     });
 
-    it('should be called when a document has been rendered', () => {
+    it('should not scroll immediately when the docId (path) changes', () => {
-      const scrollService: AutoScrollService = fixture.debugElement.injector.get(AutoScrollService);
+      locationService.go('guide/pipes');
-      spyOn(scrollService, 'scroll');
+      // deliberately not calling `fixture.detectChanges` because don't want `onDocRendered`
+      expect(scrollSpy).not.toHaveBeenCalled();
+    });
+
+    it('should scroll when just the hash changes (# alone)', () => {
+      locationService.go('guide/pipes');
+      locationService.go('guide/pipes#somewhere');
+      expect(scrollSpy).toHaveBeenCalled();
+    });
+
+    it('should scroll when just the hash changes (/#)', () => {
+      locationService.go('guide/pipes');
+      locationService.go('guide/pipes/#somewhere');
+      expect(scrollSpy).toHaveBeenCalled();
+    });
+
+    it('should scroll again when nav to the same hash twice in succession', () => {
+      locationService.go('guide/pipes');
+      locationService.go('guide/pipes#somewhere');
+      locationService.go('guide/pipes#somewhere');
+      expect(scrollSpy.calls.count()).toBe(2);
+    });
+
+    it('should scroll when call onDocRendered directly', () => {
       component.onDocRendered();
-      expect(scrollService.scroll).toHaveBeenCalledWith();
+      expect(scrollSpy).toHaveBeenCalled();
+    });
+
+    it('should scroll (via onDocRendered) when finish navigating to a new doc', () => {
+      locationService.go('guide/pipes');
+      fixture.detectChanges(); // triggers the event that calls onDocRendered
+      expect(scrollSpy).toHaveBeenCalled();
     });
   });
 
@@ -464,7 +494,7 @@ class TestHttp {
       const match = /content\/docs\/(.+)\.json/.exec(url);
       const id = match[1];
       const title = id.split('/').pop().replace(/^([a-z])/, (_, letter) => letter.toUpperCase());
-      const contents = `<h1>${title} Doc</h1>`;
+      const contents = `<h1>${title} Doc</h1><h2 id="#somewhere">Some heading</h2>`;
       data = { id, title, contents };
       if (id === 'no-title') {
         data.title = '';

aio/src/app/app.component.ts

@@ -21,6 +21,7 @@ const sideNavView = 'SideNav';
 export class AppComponent implements OnInit {
 
   currentNode: CurrentNode;
+  currentPath: string;
   dtOn = false;
   pageId: string;
   currentDocument: DocumentContents;
@@ -76,8 +77,15 @@ export class AppComponent implements OnInit {
       this.setPageId(doc.id);
     });
 
-    // scroll even if only the hash fragment changed
+    this.locationService.currentPath.subscribe(path => {
-    this.locationService.currentUrl.subscribe(() => this.autoScroll());
+        if (this.currentPath && path === this.currentPath) {
+          // scroll only if on same page (most likely a change to the hash)
+          this.autoScroll();
+        } else {
+          // don't scroll; leave that to `onDocRendered`
+          this.currentPath = path;
+        }
+      });
 
     this.navigationService.currentNode.subscribe(currentNode => {
       this.currentNode = currentNode;
@@ -100,7 +108,7 @@ export class AppComponent implements OnInit {
     this.swUpdateNotifications.enable();
   }
 
-  // Scroll to the anchor in the hash fragment.
+  // Scroll to the anchor in the hash fragment or top of doc.
   autoScroll() {
     this.autoScrollService.scroll();
   }

aio/src/app/shared/location.service.ts

@@ -17,8 +17,9 @@ export class LocationService {
     .map(url => this.stripSlashes(url))
     .do(url => this.gaService.locationChanged(url))
     .publishReplay(1);
+
   currentPath = this.currentUrl
-    .map(url => url.match(/[^?#]*/)[0]);   // strip off query and hash
+    .map(url => url.match(/[^?#]*/)[0]); // strip query and hash
 
   constructor(
     private gaService: GaService,

aio/src/testing/location.service.ts

@@ -2,7 +2,8 @@ import { BehaviorSubject } from 'rxjs/BehaviorSubject';
 
 export class MockLocationService {
   urlSubject = new BehaviorSubject<string>(this.initialUrl);
-  currentUrl = this.urlSubject.asObservable();
+  currentUrl = this.urlSubject.asObservable().map(url => this.stripSlashes(url));
+  // strip off query and hash
   currentPath = this.currentUrl.map(url => url.match(/[^?#]*/)[0]);
   search = jasmine.createSpy('search').and.returnValue({});
   setSearch = jasmine.createSpy('setSearch');
@@ -12,5 +13,9 @@ export class MockLocationService {
       .and.returnValue(false); // prevent click from causing a browser navigation
 
   constructor(private initialUrl) {}
+
+  private stripSlashes(url: string) {
+    return url.replace(/^\/+/, '').replace(/\/+(\?|#|$)/, '$1');
+  }
 }