From 43ac02e56640fe6d7f73c993f41947726e7a0a6f Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Wed, 23 Oct 2019 11:47:38 +0300 Subject: [PATCH] fix(docs-infra): scroll to top when navigating to new page via address bar (#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 #33260 PR Close #33344 --- aio/src/app/app.component.spec.ts | 8 +- aio/src/app/app.component.ts | 2 +- aio/src/app/shared/location.service.spec.ts | 14 ++-- aio/src/app/shared/location.service.ts | 2 +- aio/src/app/shared/scroll.service.ts | 23 +++++- aio/tests/e2e/src/app.e2e-spec.ts | 4 +- aio/tests/e2e/src/app.po.ts | 9 ++- aio/tests/e2e/src/scroll.e2e-spec.ts | 90 +++++++++++++++++++++ 8 files changed, 133 insertions(+), 19 deletions(-) create mode 100644 aio/tests/e2e/src/scroll.e2e-spec.ts diff --git a/aio/src/app/app.component.spec.ts b/aio/src/app/app.component.spec.ts index 1c630d6f01..c1cf20877b 100644 --- a/aio/src/app/app.component.spec.ts +++ b/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); 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', (() => { diff --git a/aio/src/app/app.component.ts b/aio/src/app/app.component.ts index a4caa5d5e7..f4d65b60a8 100644 --- a/aio/src/app/app.component.ts +++ b/aio/src/app/app.component.ts @@ -202,7 +202,7 @@ export class AppComponent implements OnInit { } onDocRemoved() { - this.scrollService.removeStoredScrollPosition(); + this.scrollService.removeStoredScrollInfo(); } onDocInserted() { diff --git a/aio/src/app/shared/location.service.spec.ts b/aio/src/app/shared/location.service.spec.ts index 4d6d8ba616..a098d9bf31 100644 --- a/aio/src/app/shared/location.service.spec.ts +++ b/aio/src/app/shared/location.service.spec.ts @@ -296,11 +296,11 @@ 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'); @@ -308,24 +308,24 @@ describe('LocationService', () => { 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 { diff --git a/aio/src/app/shared/location.service.ts b/aio/src/app/shared/location.service.ts index ec13fa8a6e..d09d2cee06 100644 --- a/aio/src/app/shared/location.service.ts +++ b/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); diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts index aa08a517ee..690ea3fc63 100644 --- a/aio/src/app/shared/scroll.service.ts +++ b/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'); } diff --git a/aio/tests/e2e/src/app.e2e-spec.ts b/aio/tests/e2e/src/app.e2e-spec.ts index 4f81ed1b62..4876cc9525 100644 --- a/aio/tests/e2e/src/app.e2e-spec.ts +++ b/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)); diff --git a/aio/tests/e2e/src/app.po.ts b/aio/tests/e2e/src/app.po.ts index 76390c1f6a..95bca28562 100644 --- a/aio/tests/e2e/src/app.po.ts +++ b/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) { diff --git a/aio/tests/e2e/src/scroll.e2e-spec.ts b/aio/tests/e2e/src/scroll.e2e-spec.ts new file mode 100644 index 0000000000..b638008a59 --- /dev/null +++ b/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[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); + }); +});