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
This commit is contained in:
George Kalpakas 2019-10-23 11:47:38 +03:00 committed by Andrew Kushnir
parent ed4d96f858
commit 43ac02e566
8 changed files with 133 additions and 19 deletions

View File

@ -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', (() => {

View File

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

View File

@ -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 {

View File

@ -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);

View File

@ -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');
}

View File

@ -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));

View File

@ -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) {

View File

@ -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);
});
});