fix(docs-infra): scroll to top when navigating to new page via addres…

…s bar (angular#33344)

Previously, when navigating to a new page via a link, the scroll
position was correctly restored to 0, but navigating to a new page via
typing the URL in the browser address bar keeps the old scroll position.

This commit ensures that the scroll position is restored to 0 whenever
the `ScrollService` is instantiated anew (i.e. new page navigation). The
old behavior of retaining the scroll position on reload is kept by
storing the old URL when leaving a page and only applying the stored
scroll position if the new URL matches the stored one.

Fixes angular#33260

PR Close angular#33344

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

@@ -464,14 +464,14 @@ describe('AppComponent', () => {
       let scrollSpy: jasmine.Spy;
       let scrollToTopSpy: jasmine.Spy;
       let scrollAfterRenderSpy: jasmine.Spy;
-      let removeStoredScrollPositionSpy: jasmine.Spy;
+      let removeStoredScrollInfoSpy: jasmine.Spy;
 
       beforeEach(() => {
         scrollService = fixture.debugElement.injector.get<ScrollService>(ScrollService);
         scrollSpy = spyOn(scrollService, 'scroll');
         scrollToTopSpy = spyOn(scrollService, 'scrollToTop');
         scrollAfterRenderSpy = spyOn(scrollService, 'scrollAfterRender');
-        removeStoredScrollPositionSpy = spyOn(scrollService, 'removeStoredScrollPosition');
+        removeStoredScrollInfoSpy = spyOn(scrollService, 'removeStoredScrollInfo');
       });
 
       it('should not scroll immediately when the docId (path) changes', () => {
@@ -516,9 +516,9 @@ describe('AppComponent', () => {
         expect(scrollSpy).toHaveBeenCalledTimes(1);
       });
 
-      it('should call `removeStoredScrollPosition` when call `onDocRemoved` directly', () => {
+      it('should call `removeStoredScrollInfo` when call `onDocRemoved` directly', () => {
         component.onDocRemoved();
-        expect(removeStoredScrollPositionSpy).toHaveBeenCalled();
+        expect(removeStoredScrollInfoSpy).toHaveBeenCalled();
       });
 
       it('should call `scrollAfterRender` when call `onDocInserted` directly', (() => {

aio/src/app/app.component.ts

@@ -202,7 +202,7 @@ export class AppComponent implements OnInit {
   }
 
   onDocRemoved() {
-    this.scrollService.removeStoredScrollPosition();
+    this.scrollService.removeStoredScrollInfo();
   }
 
   onDocInserted() {

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

@@ -296,36 +296,36 @@ describe('LocationService', () => {
     it('should do a "full page navigation" and remove the stored scroll position when navigating to ' +
       'internal URLs only if a ServiceWorker update has been activated', () => {
       const goExternalSpy = spyOn(service, 'goExternal');
-      const removeStoredScrollPositionSpy = spyOn(scrollService, 'removeStoredScrollPosition');
+      const removeStoredScrollInfoSpy = spyOn(scrollService, 'removeStoredScrollInfo');
 
       // Internal URL - No ServiceWorker update
       service.go('some-internal-url');
-      expect(removeStoredScrollPositionSpy).not.toHaveBeenCalled();
+      expect(removeStoredScrollInfoSpy).not.toHaveBeenCalled();
       expect(goExternalSpy).not.toHaveBeenCalled();
       expect(location.path(true)).toEqual('some-internal-url');
 
       // Internal URL - ServiceWorker update
       swUpdates.updateActivated.next('foo');
       service.go('other-internal-url');
       expect(goExternalSpy).toHaveBeenCalledWith('other-internal-url');
-      expect(removeStoredScrollPositionSpy).toHaveBeenCalled();
+      expect(removeStoredScrollInfoSpy).toHaveBeenCalled();
     });
 
     it('should not remove the stored scroll position when navigating to external URLs', () => {
-      const removeStoredScrollPositionSpy = spyOn(scrollService, 'removeStoredScrollPosition');
+      const removeStoredScrollInfoSpy = spyOn(scrollService, 'removeStoredScrollInfo');
       const goExternalSpy = spyOn(service, 'goExternal');
       const externalUrl = 'http://some/far/away/land';
       const otherExternalUrl = 'http://some/far/far/away/land';
 
       // External URL - No ServiceWorker update
       service.go(externalUrl);
-      expect(removeStoredScrollPositionSpy).not.toHaveBeenCalled();
+      expect(removeStoredScrollInfoSpy).not.toHaveBeenCalled();
       expect(goExternalSpy).toHaveBeenCalledWith(externalUrl);
 
       // External URL - ServiceWorker update
       swUpdates.updateActivated.next('foo');
       service.go(otherExternalUrl);
-      expect(removeStoredScrollPositionSpy).not.toHaveBeenCalled();
+      expect(removeStoredScrollInfoSpy).not.toHaveBeenCalled();
       expect(goExternalSpy).toHaveBeenCalledWith(otherExternalUrl);
     });
 
@@ -633,7 +633,7 @@ class MockSwUpdatesService {
 }
 
 class MockScrollService {
-  removeStoredScrollPosition() { }
+  removeStoredScrollInfo() { }
 }
 
 class TestGaService {

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

@@ -49,7 +49,7 @@ export class LocationService {
     } else if (this.swUpdateActivated) {
       // (Do a "full page navigation" if a ServiceWorker update has been activated)
       // We need to remove stored Position in order to be sure to scroll to the Top position
-      this.scrollService.removeStoredScrollPosition();
+      this.scrollService.removeStoredScrollInfo();
       this.goExternal(url);
     } else {
       this.location.go(url);

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

@@ -58,6 +58,10 @@ export class ScrollService implements OnDestroy {
         .pipe(debounceTime(250), takeUntil(this.onDestroy))
         .subscribe(() => this.updateScrollPositionInHistory());
 
+    fromEvent(window, 'beforeunload')
+        .pipe(takeUntil(this.onDestroy))
+        .subscribe(() => this.updateScrollLocationHref());
+
     // Change scroll restoration strategy to `manual` if it's supported
     if (this.supportManualScrollRestoration) {
       history.scrollRestoration = 'manual';
@@ -70,13 +74,18 @@ export class ScrollService implements OnDestroy {
         } else {
           // Navigating with the forward/back button, we have to remove the position from the
           // session storage in order to avoid a race-condition.
-          this.removeStoredScrollPosition();
+          this.removeStoredScrollInfo();
           // The `popstate` event is always triggered by a browser action such as clicking the
           // forward/back button. It can be followed by a `hashchange` event.
           this.poppedStateScrollPosition = event.state ? event.state.scrollPosition : null;
         }
       });
     }
+
+    // If this was not a reload, discard the stored scroll info.
+    if (window.location.href !== this.getStoredScrollLocationHref()) {
+      this.removeStoredScrollInfo();
+    }
   }
 
   ngOnDestroy() {
@@ -170,6 +179,10 @@ export class ScrollService implements OnDestroy {
     }
   }
 
+  updateScrollLocationHref(): void {
+    window.sessionStorage.setItem('scrollLocationHref', window.location.href);
+  }
+
   /**
    * Update the state with scroll position into history.
    */
@@ -181,6 +194,11 @@ export class ScrollService implements OnDestroy {
     }
   }
 
+  getStoredScrollLocationHref(): string | null {
+    const href = window.sessionStorage.getItem('scrollLocationHref');
+    return href || null;
+  }
+
   getStoredScrollPosition(): ScrollPosition | null {
     const position = window.sessionStorage.getItem('scrollPosition');
     if (!position) { return null; }
@@ -189,7 +207,8 @@ export class ScrollService implements OnDestroy {
     return [+x, +y];
   }
 
-  removeStoredScrollPosition() {
+  removeStoredScrollInfo() {
+    window.sessionStorage.removeItem('scrollLocationHref');
     window.sessionStorage.removeItem('scrollPosition');
   }
 

aio/tests/e2e/src/app.e2e-spec.ts

@@ -100,7 +100,7 @@ describe('site App', function() {
     it('should scroll to the top when navigating to another page', () => {
       page.navigateTo('guide/security');
 
-      page.scrollToBottom();
+      page.scrollTo('bottom');
       expect(page.getScrollTop()).toBeGreaterThan(0);
 
       page.click(page.getNavItem(/api/i));
@@ -111,7 +111,7 @@ describe('site App', function() {
     it('should scroll to the top when navigating to the same page', () => {
       page.navigateTo('guide/security');
 
-      page.scrollToBottom();
+      page.scrollTo('bottom');
       expect(page.getScrollTop()).toBeGreaterThan(0);
 
       page.click(page.getNavItem(/security/i));

aio/tests/e2e/src/app.po.ts

@@ -59,8 +59,13 @@ export class SitePage {
     return browser.executeScript('return window.pageYOffset');
   }
 
-  scrollToBottom() {
-    return browser.executeScript('window.scrollTo(0, document.body.scrollHeight)');
+  scrollTo(y: 'top' | 'bottom' | number) {
+    const yExpr = (y === 'top') ? '0' : (y === 'bottom') ? 'document.body.scrollHeight' : y;
+
+    return browser.executeScript(`
+      window.scrollTo(0, ${yExpr});
+      window.dispatchEvent(new Event('scroll'));
+    `);
   }
 
   click(elementFinder: ElementFinder) {

aio/tests/e2e/src/scroll.e2e-spec.ts

@@ -0,0 +1,90 @@
+import { browser } from 'protractor';
+import { SitePage } from './app.po';
+
+describe('site auto-scrolling', () => {
+  let page: SitePage;
+
+  // Helpers
+  const scrollAndWait = async (y: Parameters<SitePage['scrollTo']>[0] = 'bottom') => {
+    await page.scrollTo(y);
+    await browser.sleep(500);  // Scroll position is stored every 250ms for performance reasons.
+  };
+
+  beforeEach(async () => {
+    page = new SitePage();
+    await page.navigateTo('');
+  });
+
+  it('should be initially scrolled to top', async () => {
+    expect(await page.getScrollTop()).toBe(0);
+  });
+
+  it('should scroll to top when navigating to a different page', async () => {
+    await scrollAndWait();
+    expect(await page.getScrollTop()).not.toBe(0);
+
+    await page.navigateTo('docs');
+    expect(await page.getScrollTop()).toBe(0);
+  });
+
+  it('should retain the scroll position on reload', async () => {
+    await scrollAndWait();
+    expect(await page.getScrollTop()).not.toBe(0);
+
+    await browser.refresh();
+    expect(await page.getScrollTop()).not.toBe(0);
+  });
+
+  it('should scroll to top when navigating to a different page via a link', async () => {
+    await scrollAndWait();
+    expect(await page.getScrollTop()).not.toBe(0);
+
+    await page.docsMenuLink.click();
+    expect(await page.getScrollTop()).toBe(0);
+  });
+
+  it('should scroll to top when navigating to the same page via a link', async () => {
+    await scrollAndWait();
+    expect(await page.getScrollTop()).not.toBe(0);
+
+    await page.homeLink.click();
+    expect(await page.getScrollTop()).toBe(0);
+  });
+
+  // TODO: Find a way to accurately emulate clicking the browser back/forward button. Apparently,
+  // both `browser.navigate().back()` and `browser.executeScript('history.back()')` cause a full
+  // page load, which behaves differently than clicking the browser back button (and triggering a
+  // `popstate` event instead of a navigation). Same for `forward()`.
+  xit('should retain the scroll position when navigating back/forward', async () => {
+    await scrollAndWait(100);
+    expect(await page.getScrollTop()).toBeCloseTo(100, -1);
+
+    await page.navigateTo('docs');
+    await scrollAndWait(50);
+    expect(await page.getScrollTop()).toBeCloseTo(50, -1);
+
+    await page.navigateTo('features');
+    await scrollAndWait(75);
+    expect(await page.getScrollTop()).toBeCloseTo(75, -1);
+
+    // Go back.
+    await browser.navigate().back();
+    expect(await page.locationPath()).toBe('/docs');
+    expect(await page.getScrollTop()).toBeCloseTo(50, -1);
+
+    // Go back.
+    await browser.navigate().back();
+    expect(await page.locationPath()).toBe('/');
+    expect(await page.getScrollTop()).toBeCloseTo(100, -1);
+
+    // Go forward.
+    await browser.navigate().forward();
+    expect(await page.locationPath()).toBe('/docs');
+    expect(await page.getScrollTop()).toBeCloseTo(50, -1);
+
+    // Go forward.
+    await browser.navigate().forward();
+    expect(await page.locationPath()).toBe('/features');
+    expect(await page.getScrollTop()).toBeCloseTo(75, -1);
+  });
+});