From a3e2b99fd4c79c8dd20c6ff1ba7f5e32435e4d47 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 09:13:51 -0400 Subject: [PATCH 01/13] remove timestamp from history --- packages/core/src/history.ts | 2 -- tests/history.spec.ts | 5 ----- 2 files changed, 7 deletions(-) diff --git a/packages/core/src/history.ts b/packages/core/src/history.ts index 1cd980de1..d51a2b24c 100644 --- a/packages/core/src/history.ts +++ b/packages/core/src/history.ts @@ -42,7 +42,6 @@ class History { window.history.pushState( { page: data, - timestamp: Date.now(), }, '', page.url, @@ -107,7 +106,6 @@ class History { window.history.replaceState( { page: data, - timestamp: Date.now(), }, '', page.url, diff --git a/tests/history.spec.ts b/tests/history.spec.ts index 1e0c481d5..691a87744 100644 --- a/tests/history.spec.ts +++ b/tests/history.spec.ts @@ -9,7 +9,6 @@ test('it will not encrypt history by default', async ({ page }) => { const historyState1 = await page.evaluate(() => window.history.state) await expect(historyState1.page.component).toBe('History/Page') await expect(historyState1.page.props.pageNumber).toBe('1') - await expect(historyState1.timestamp).toBeGreaterThan(0) await expect(page.getByText('This is page 1')).toBeVisible() await clickAndWaitForResponse(page, 'Page 2', '/history/2') @@ -17,7 +16,6 @@ test('it will not encrypt history by default', async ({ page }) => { await expect(historyState2.page.component).toBe('History/Page') await expect(historyState2.page.props.pageNumber).toBe('2') await expect(page.getByText('This is page 2')).toBeVisible() - await expect(historyState2.timestamp).toBeGreaterThan(0) requests.listen(page) @@ -39,7 +37,6 @@ test('it can encrypt history', async ({ page }) => { // but Playwright doesn't transfer it as such over the wire (page.evaluate), // so if the object is "empty" and the page check below works, it's working. await expect(historyState3.page).toEqual({}) - await expect(historyState3.timestamp).toBeGreaterThan(0) requests.listen(page) @@ -52,7 +49,6 @@ test('it can encrypt history', async ({ page }) => { const historyState1 = await page.evaluate(() => window.history.state) await expect(historyState1.page.component).toBe('History/Page') await expect(historyState1.page.props.pageNumber).toBe('1') - await expect(historyState1.timestamp).toBeGreaterThan(0) await page.goForward() await page.waitForURL('/history/3') @@ -117,7 +113,6 @@ test('multi byte strings can be encrypyed', async ({ page }) => { // but Playwright doesn't transfer it as such over the wire (page.evaluate), // so if the object is "empty" and the page check below works, it's working. await expect(historyState5.page).toEqual({}) - await expect(historyState5.timestamp).toBeGreaterThan(0) await expect(page.getByText('Multi byte character: 😃')).toBeVisible() await clickAndWaitForResponse(page, 'Page 1', '/history/1') From db17ff8a73709b3c88f93da233a18b34af2d0500 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 09:14:22 -0400 Subject: [PATCH 02/13] store scroll regions outside of encrypted page --- packages/core/src/eventHandler.ts | 2 +- packages/core/src/history.ts | 38 ++++++++++++++++++++++++++++--- packages/core/src/initialVisit.ts | 8 ++++--- packages/core/src/scroll.ts | 24 ++++++++----------- packages/core/src/types.ts | 7 ++++-- 5 files changed, 55 insertions(+), 24 deletions(-) diff --git a/packages/core/src/eventHandler.ts b/packages/core/src/eventHandler.ts index 1eea3b978..5efe3c069 100644 --- a/packages/core/src/eventHandler.ts +++ b/packages/core/src/eventHandler.ts @@ -81,7 +81,7 @@ class EventHandler { .decrypt(state.page) .then((data) => { currentPage.setQuietly(data, { preserveState: false }).then(() => { - Scroll.restore(currentPage.get()) + Scroll.restore(history.getScrollRegions()) fireNavigateEvent(currentPage.get()) }) }) diff --git a/packages/core/src/history.ts b/packages/core/src/history.ts index d51a2b24c..8975eb6d5 100644 --- a/packages/core/src/history.ts +++ b/packages/core/src/history.ts @@ -1,7 +1,7 @@ import { decryptHistory, encryptHistory, historySessionStorageKeys } from './encryption' import { page as currentPage } from './page' import { SessionStorage } from './sessionStorage' -import { Page } from './types' +import { Page, ScrollRegion } from './types' const isServer = typeof window === 'undefined' @@ -13,6 +13,7 @@ class History { protected queue: (() => Promise)[] = [] // We need initialState for `restore` protected initialState: Partial | null = null + protected processingQueue = false public remember(data: unknown, key: string): void { this.replaceState({ @@ -56,11 +57,23 @@ class History { }) } - public processQueue(): Promise { + public processQueue(): void { + if (this.processingQueue) { + return + } + + this.processingQueue = true + + this.processNext().then(() => { + this.processingQueue = false + }) + } + + protected processNext(): Promise { const next = this.queue.shift() if (next) { - return next().then(() => this.processQueue()) + return next().then(() => this.processNext()) } return Promise.resolve() @@ -92,6 +105,25 @@ class History { return pageData instanceof ArrayBuffer ? decryptHistory(pageData) : Promise.resolve(pageData) } + public saveScrollPositions(scrollRegions: ScrollRegion[]): void { + this.addToQueue(() => { + return Promise.resolve().then(() => { + window.history.replaceState( + { + page: window.history.state.page, + scrollRegions, + }, + '', + this.current.url, + ) + }) + }) + } + + public getScrollRegions(): ScrollRegion[] { + return window.history.state.scrollRegions || [] + } + public replaceState(page: Page): void { currentPage.merge(page) diff --git a/packages/core/src/initialVisit.ts b/packages/core/src/initialVisit.ts index 67d602748..9ea651818 100644 --- a/packages/core/src/initialVisit.ts +++ b/packages/core/src/initialVisit.ts @@ -27,11 +27,13 @@ export class InitialVisit { return false } + const scrollRegions = history.getScrollRegions() + history .decrypt() .then((data) => { currentPage.set(data, { preserveScroll: true, preserveState: true }).then(() => { - Scroll.restore(currentPage.get()) + Scroll.restore(scrollRegions) fireNavigateEvent(currentPage.get()) }) }) @@ -62,7 +64,7 @@ export class InitialVisit { .decrypt() .then(() => { const rememberedState = history.getState(history.rememberedState, {}) - const scrollRegions = history.getState(history.scrollRegions, []) + const scrollRegions = history.getScrollRegions() currentPage.remember(rememberedState) currentPage.scrollRegions(scrollRegions) @@ -73,7 +75,7 @@ export class InitialVisit { }) .then(() => { if (locationVisit.preserveScroll) { - Scroll.restore(currentPage.get()) + Scroll.restore(scrollRegions) } fireNavigateEvent(currentPage.get()) diff --git a/packages/core/src/scroll.ts b/packages/core/src/scroll.ts index 8e528bc34..74f1f0557 100644 --- a/packages/core/src/scroll.ts +++ b/packages/core/src/scroll.ts @@ -1,16 +1,14 @@ import { history } from './history' -import { page as currentPage } from './page' -import { Page } from './types' +import { Page, ScrollRegion } from './types' export class Scroll { - public static save(page: Page): void { - history.replaceState({ - ...page, - scrollRegions: Array.from(this.regions()).map((region) => ({ + public static save(): void { + history.saveScrollPositions( + Array.from(this.regions()).map((region) => ({ top: region.scrollTop, left: region.scrollLeft, })), - }) + ) } protected static regions(): NodeListOf { @@ -31,7 +29,7 @@ export class Scroll { } }) - this.save(page) + this.save() if (window.location.hash) { // We're using a setTimeout() here as a workaround for a bug in the React adapter where the @@ -40,13 +38,9 @@ export class Scroll { } } - public static restore(page: Page): void { - if (!page.scrollRegions) { - return - } - + public static restore(scrollRegions: ScrollRegion[]): void { this.regions().forEach((region: Element, index: number) => { - const scrollPosition = page.scrollRegions[index] + const scrollPosition = scrollRegions[index] if (!scrollPosition) { return @@ -65,7 +59,7 @@ export class Scroll { const target = event.target as Element if (typeof target.hasAttribute === 'function' && target.hasAttribute('scroll-region')) { - this.save(currentPage.get()) + this.save() } } } diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 8bec60465..e37c75ac3 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -43,12 +43,15 @@ export interface Page { deferredProps?: Record mergeProps?: string[] - /** @internal */ - scrollRegions: Array<{ top: number; left: number }> /** @internal */ rememberedState: Record } +export type ScrollRegion = { + top: number + left: number +} + export type PageResolver = (name: string) => Component export type PageHandler = ({ From 9e87e78d14dcd9ea58c3f051f208893671d3ad24 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 10:34:52 -0400 Subject: [PATCH 03/13] take over scroll restoration from browser --- packages/core/src/eventHandler.ts | 1 + packages/core/src/history.ts | 59 ++++++++++++++++++++++++++++--- packages/core/src/scroll.ts | 19 ++++++++-- 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/packages/core/src/eventHandler.ts b/packages/core/src/eventHandler.ts index 5efe3c069..094e1818c 100644 --- a/packages/core/src/eventHandler.ts +++ b/packages/core/src/eventHandler.ts @@ -82,6 +82,7 @@ class EventHandler { .then((data) => { currentPage.setQuietly(data, { preserveState: false }).then(() => { Scroll.restore(history.getScrollRegions()) + Scroll.restoreDocument() fireNavigateEvent(currentPage.get()) }) }) diff --git a/packages/core/src/history.ts b/packages/core/src/history.ts index 8975eb6d5..42362808a 100644 --- a/packages/core/src/history.ts +++ b/packages/core/src/history.ts @@ -108,13 +108,27 @@ class History { public saveScrollPositions(scrollRegions: ScrollRegion[]): void { this.addToQueue(() => { return Promise.resolve().then(() => { - window.history.replaceState( + this.doReplaceState( { page: window.history.state.page, scrollRegions, }, - '', - this.current.url, + this.current.url!, + ) + }) + }) + } + + public saveDocumentScrollPosition(scrollRegion: ScrollRegion): void { + console.log('saveDocumentScrollPosition', scrollRegion) + this.addToQueue(() => { + return Promise.resolve().then(() => { + this.doReplaceState( + { + page: window.history.state.page, + documentScrollPosition: scrollRegion, + }, + this.current.url!, ) }) }) @@ -124,6 +138,10 @@ class History { return window.history.state.scrollRegions || [] } + public getDocumentScrollPosition(): ScrollRegion { + return window.history.state.documentScrollPosition || { top: 0, left: 0 } + } + public replaceState(page: Page): void { currentPage.merge(page) @@ -135,17 +153,44 @@ class History { this.addToQueue(() => { return this.getPageData(page).then((data) => { - window.history.replaceState( + this.doReplaceState( { page: data, }, - '', page.url, ) }) }) } + protected doReplaceState( + data: { + page: Page | ArrayBuffer + scrollRegions?: ScrollRegion[] + documentScrollPosition?: ScrollRegion + }, + url: string, + ): void { + console.log( + 'doReplaceState', + { + ...data, + scrollRegions: data.scrollRegions ?? window.history.state?.scrollRegions, + documentScrollPosition: data.documentScrollPosition ?? window.history.state?.documentScrollPosition, + }, + url, + ) + window.history.replaceState( + { + ...data, + scrollRegions: data.scrollRegions ?? window.history.state?.scrollRegions, + documentScrollPosition: data.documentScrollPosition ?? window.history.state?.documentScrollPosition, + }, + '', + url, + ) + } + protected addToQueue(fn: () => Promise): void { this.queue.push(fn) this.processQueue() @@ -180,4 +225,8 @@ class History { } } +if (window.history.scrollRestoration) { + window.history.scrollRestoration = 'manual' +} + export const history = new History() diff --git a/packages/core/src/scroll.ts b/packages/core/src/scroll.ts index 74f1f0557..419a1f10f 100644 --- a/packages/core/src/scroll.ts +++ b/packages/core/src/scroll.ts @@ -39,6 +39,8 @@ export class Scroll { } public static restore(scrollRegions: ScrollRegion[]): void { + this.restoreDocument() + this.regions().forEach((region: Element, index: number) => { const scrollPosition = scrollRegions[index] @@ -55,10 +57,23 @@ export class Scroll { }) } + public static restoreDocument(): void { + const scrollPosition = history.getDocumentScrollPosition() + + if (typeof window !== 'undefined') { + window.scrollTo(scrollPosition.left, scrollPosition.top) + } + } + public static onScroll(event: Event): void { - const target = event.target as Element + const target = event.target as Element | Document - if (typeof target.hasAttribute === 'function' && target.hasAttribute('scroll-region')) { + if (target instanceof Document) { + history.saveDocumentScrollPosition({ + top: target.documentElement.scrollTop, + left: target.documentElement.scrollLeft, + }) + } else if (typeof target.hasAttribute === 'function' && target.hasAttribute('scroll-region')) { this.save() } } From fcf12fc549f94a9ee17b866c166c8326ba61a515 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 10:36:11 -0400 Subject: [PATCH 04/13] remove console logs --- packages/core/src/history.ts | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/packages/core/src/history.ts b/packages/core/src/history.ts index 42362808a..ca714ddee 100644 --- a/packages/core/src/history.ts +++ b/packages/core/src/history.ts @@ -120,7 +120,6 @@ class History { } public saveDocumentScrollPosition(scrollRegion: ScrollRegion): void { - console.log('saveDocumentScrollPosition', scrollRegion) this.addToQueue(() => { return Promise.resolve().then(() => { this.doReplaceState( @@ -171,15 +170,6 @@ class History { }, url: string, ): void { - console.log( - 'doReplaceState', - { - ...data, - scrollRegions: data.scrollRegions ?? window.history.state?.scrollRegions, - documentScrollPosition: data.documentScrollPosition ?? window.history.state?.documentScrollPosition, - }, - url, - ) window.history.replaceState( { ...data, From 5b3f943d7fb17c67e0b876a516f4da5bc9d79747 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 11:18:00 -0400 Subject: [PATCH 05/13] new generic queue helper --- packages/core/src/queue.ts | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 packages/core/src/queue.ts diff --git a/packages/core/src/queue.ts b/packages/core/src/queue.ts new file mode 100644 index 000000000..9c53b369f --- /dev/null +++ b/packages/core/src/queue.ts @@ -0,0 +1,31 @@ +export default class Queue { + protected items: (() => T)[] = [] + protected processing = false + + public add(item: () => T) { + this.items.push(item) + this.process() + } + + protected process() { + if (this.processing) { + return + } + + this.processing = true + + this.processNext().then(() => { + this.processing = false + }) + } + + protected processNext(): Promise { + const next = this.items.shift() + + if (next) { + return Promise.resolve(next()).then(() => this.processNext()) + } + + return Promise.resolve() + } +} From fcdf87b98f4803bec8edf75822a023a17ccd9da2 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 11:43:53 -0400 Subject: [PATCH 06/13] fixed fully processing queue --- packages/core/src/queue.ts | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/packages/core/src/queue.ts b/packages/core/src/queue.ts index 9c53b369f..999733baf 100644 --- a/packages/core/src/queue.ts +++ b/packages/core/src/queue.ts @@ -1,22 +1,18 @@ export default class Queue { protected items: (() => T)[] = [] - protected processing = false + protected processingPromise: Promise | null = null public add(item: () => T) { this.items.push(item) - this.process() + return this.process() } - protected process() { - if (this.processing) { - return - } - - this.processing = true - - this.processNext().then(() => { - this.processing = false + public process() { + this.processingPromise ??= this.processNext().then(() => { + this.processingPromise = null }) + + return this.processingPromise } protected processNext(): Promise { From cb112dbdc0d3c415686b74ae1ae3edf098f109c3 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 11:44:07 -0400 Subject: [PATCH 07/13] use new queue --- packages/core/src/history.ts | 40 ++++++++--------------------------- packages/core/src/response.ts | 38 +++------------------------------ 2 files changed, 12 insertions(+), 66 deletions(-) diff --git a/packages/core/src/history.ts b/packages/core/src/history.ts index ca714ddee..658a3be65 100644 --- a/packages/core/src/history.ts +++ b/packages/core/src/history.ts @@ -1,19 +1,20 @@ import { decryptHistory, encryptHistory, historySessionStorageKeys } from './encryption' import { page as currentPage } from './page' +import Queue from './queue' import { SessionStorage } from './sessionStorage' import { Page, ScrollRegion } from './types' const isServer = typeof window === 'undefined' +const queue = new Queue>() + class History { public rememberedState = 'rememberedState' as const public scrollRegions = 'scrollRegions' as const public preserveUrl = false protected current: Partial = {} - protected queue: (() => Promise)[] = [] // We need initialState for `restore` protected initialState: Partial | null = null - protected processingQueue = false public remember(data: unknown, key: string): void { this.replaceState({ @@ -38,7 +39,7 @@ class History { this.current = page - this.addToQueue(() => { + queue.add(() => { return this.getPageData(page).then((data) => { window.history.pushState( { @@ -57,26 +58,8 @@ class History { }) } - public processQueue(): void { - if (this.processingQueue) { - return - } - - this.processingQueue = true - - this.processNext().then(() => { - this.processingQueue = false - }) - } - - protected processNext(): Promise { - const next = this.queue.shift() - - if (next) { - return next().then(() => this.processNext()) - } - - return Promise.resolve() + public processQueue(): Promise { + return queue.process() } public decrypt(page: Page | null = null): Promise { @@ -106,7 +89,7 @@ class History { } public saveScrollPositions(scrollRegions: ScrollRegion[]): void { - this.addToQueue(() => { + queue.add(() => { return Promise.resolve().then(() => { this.doReplaceState( { @@ -120,7 +103,7 @@ class History { } public saveDocumentScrollPosition(scrollRegion: ScrollRegion): void { - this.addToQueue(() => { + queue.add(() => { return Promise.resolve().then(() => { this.doReplaceState( { @@ -150,7 +133,7 @@ class History { this.current = page - this.addToQueue(() => { + queue.add(() => { return this.getPageData(page).then((data) => { this.doReplaceState( { @@ -181,11 +164,6 @@ class History { ) } - protected addToQueue(fn: () => Promise): void { - this.queue.push(fn) - this.processQueue() - } - public getState(key: keyof Page, defaultValue?: T): any { return this.current?.[key] ?? defaultValue } diff --git a/packages/core/src/response.ts b/packages/core/src/response.ts index 4b905b666..4b2ffe1bf 100644 --- a/packages/core/src/response.ts +++ b/packages/core/src/response.ts @@ -3,44 +3,13 @@ import { fireErrorEvent, fireInvalidEvent, firePrefetchedEvent, fireSuccessEvent import { history } from './history' import modal from './modal' import { page as currentPage } from './page' +import Queue from './queue' import { RequestParams } from './requestParams' import { SessionStorage } from './sessionStorage' import { ActiveVisit, ErrorBag, Errors, Page } from './types' import { hrefToUrl, isSameUrlWithoutHash, setHashIfSameUrl } from './url' -class ResponseQueue { - protected queue: Response[] = [] - protected processing = false - - public add(response: Response) { - this.queue.push(response) - } - - public async process(): Promise { - if (this.processing) { - return Promise.resolve() - } - - this.processing = true - await this.processQueue() - this.processing = false - - return Promise.resolve() - } - - protected async processQueue(): Promise { - const nextResponse = this.queue.shift() - - if (nextResponse) { - await nextResponse.process() - return this.processQueue() - } - - return Promise.resolve() - } -} - -const queue = new ResponseQueue() +const queue = new Queue>() export class Response { constructor( @@ -60,8 +29,7 @@ export class Response { } public async handle() { - queue.add(this) - return queue.process() + return queue.add(() => this.process()) } public async process() { From 17b56e56360009283986dcbcc8e9714d43b0bf38 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 12:13:51 -0400 Subject: [PATCH 08/13] separate out window scroll --- packages/core/src/eventHandler.ts | 1 + packages/core/src/scroll.ts | 16 +++++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/core/src/eventHandler.ts b/packages/core/src/eventHandler.ts index 094e1818c..a60467383 100644 --- a/packages/core/src/eventHandler.ts +++ b/packages/core/src/eventHandler.ts @@ -15,6 +15,7 @@ class EventHandler { public init() { if (typeof window !== 'undefined') { window.addEventListener('popstate', this.handlePopstateEvent.bind(this)) + window.addEventListener('scroll', debounce(Scroll.onWindowScroll.bind(Scroll), 100), true) } if (typeof document !== 'undefined') { diff --git a/packages/core/src/scroll.ts b/packages/core/src/scroll.ts index 419a1f10f..b810fe000 100644 --- a/packages/core/src/scroll.ts +++ b/packages/core/src/scroll.ts @@ -66,15 +66,17 @@ export class Scroll { } public static onScroll(event: Event): void { - const target = event.target as Element | Document + const target = event.target as Element - if (target instanceof Document) { - history.saveDocumentScrollPosition({ - top: target.documentElement.scrollTop, - left: target.documentElement.scrollLeft, - }) - } else if (typeof target.hasAttribute === 'function' && target.hasAttribute('scroll-region')) { + if (typeof target.hasAttribute === 'function' && target.hasAttribute('scroll-region')) { this.save() } } + + public static onWindowScroll(): void { + history.saveDocumentScrollPosition({ + top: window.scrollY, + left: window.scrollX, + }) + } } From e3feea11828a7be337f3c2783f658824d7b4cc66 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 12:14:03 -0400 Subject: [PATCH 09/13] fix scroll based tests to account for debounce --- tests/links.spec.ts | 42 +++++++++++++++++-------- tests/manual-visits.spec.ts | 63 ++++++++++++++++++++++++++----------- tests/support.ts | 6 ++++ 3 files changed, 79 insertions(+), 32 deletions(-) diff --git a/tests/links.spec.ts b/tests/links.spec.ts index a45217788..5a6475159 100644 --- a/tests/links.spec.ts +++ b/tests/links.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test' -import { consoleMessages, pageLoads, requests, shouldBeDumpPage } from './support' +import { consoleMessages, pageLoads, requests, scrollElementTo, shouldBeDumpPage } from './support' test.beforeEach(async ({ page }) => {}) @@ -363,9 +363,14 @@ test.describe('preserve scroll', () => { await page.goto('/links/preserve-scroll-false') await expect(page.getByText('Foo is now default')).toBeVisible() - await page.evaluate(() => window.scrollTo(5, 7)) - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(10, 15)) + await scrollElementTo( + page, + page.evaluate(() => window.scrollTo(5, 7)), + ) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(10, 15)), + ) await expect(page.getByText('Document scroll position is 5 & 7')).toBeVisible() await expect(page.getByText('Slot scroll position is 10 & 15')).toBeVisible() @@ -406,8 +411,10 @@ test.describe('preserve scroll', () => { await expect(page).toHaveURL('/links/preserve-scroll-false-page-two') await expect(page.getByText('Foo is now bar')).toBeVisible() - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(0, 0)) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(0, 0)), + ) await expect(page.getByText('Slot scroll position is 0 & 0')).toBeVisible() await page.goBack() @@ -429,8 +436,10 @@ test.describe('preserve scroll', () => { await expect(page).toHaveURL('/links/preserve-scroll-false-page-two') await expect(page.getByText('Foo is now baz')).toBeVisible() - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(0, 0)) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(0, 0)), + ) await expect(page.getByText('Slot scroll position is 0 & 0')).toBeVisible() const message = JSON.parse(consoleMessages.messages[0]) @@ -469,10 +478,14 @@ test.describe('enabled', () => { await page.goto('/links/preserve-scroll') await expect(page.getByText('Foo is now default')).toBeVisible() - await page.evaluate(() => window.scrollTo(5, 7)) - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(10, 15)) - + await scrollElementTo( + page, + page.evaluate(() => window.scrollTo(5, 7)), + ) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(10, 15)), + ) await expect(page.getByText('Document scroll position is 5 & 7')).toBeVisible() await expect(page.getByText('Slot scroll position is 10 & 15')).toBeVisible() }) @@ -534,7 +547,10 @@ test.describe('enabled', () => { await expect(page).toHaveURL('/links/preserve-scroll-page-two') - await page.evaluate(() => (document as any).querySelector('#slot').scrollTo(0, 0)) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(0, 0)), + ) await expect(page.getByText('Slot scroll position is 0 & 0')).toBeVisible() diff --git a/tests/manual-visits.spec.ts b/tests/manual-visits.spec.ts index d3beb6ca4..e2d82efe1 100644 --- a/tests/manual-visits.spec.ts +++ b/tests/manual-visits.spec.ts @@ -1,5 +1,12 @@ import test, { expect } from '@playwright/test' -import { clickAndWaitForResponse, consoleMessages, pageLoads, requests, shouldBeDumpPage } from './support' +import { + clickAndWaitForResponse, + consoleMessages, + pageLoads, + requests, + scrollElementTo, + shouldBeDumpPage, +} from './support' test('visits a different page', async ({ page }) => { pageLoads.watch(page) @@ -429,10 +436,14 @@ test.describe('Preserve scroll', () => { await page.goto('/visits/preserve-scroll-false') await expect(page.getByText('Foo is now default')).toBeVisible() - await page.evaluate(() => window.scrollTo(5, 7)) - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(10, 15)) - + await scrollElementTo( + page, + page.evaluate(() => window.scrollTo(5, 7)), + ) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(10, 15)), + ) await expect(page.getByText('Document scroll position is 5 & 7')).toBeVisible() await expect(page.getByText('Slot scroll position is 10 & 15')).toBeVisible() }) @@ -482,8 +493,10 @@ test.describe('Preserve scroll', () => { await expect(page).toHaveURL('/visits/preserve-scroll-false-page-two') await expect(page.getByText('Foo is now bar')).toBeVisible() - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(0, 0)) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(0, 0)), + ) await expect(page.getByText('Slot scroll position is 0 & 0')).toBeVisible() await page.goBack() @@ -500,8 +513,10 @@ test.describe('Preserve scroll', () => { await expect(page).toHaveURL('/visits/preserve-scroll-false-page-two') await expect(page.getByText('Foo is now baz')).toBeVisible() - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(0, 0)) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(0, 0)), + ) await expect(page.getByText('Slot scroll position is 0 & 0')).toBeVisible() await page.goBack() @@ -522,8 +537,10 @@ test.describe('Preserve scroll', () => { await expect(page).toHaveURL('/visits/preserve-scroll-false-page-two') await expect(page.getByText('Foo is now baz')).toBeVisible() - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(0, 0)) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(0, 0)), + ) await expect(page.getByText('Slot scroll position is 0 & 0')).toBeVisible() const message = JSON.parse(consoleMessages.messages[0]) @@ -562,10 +579,14 @@ test.describe('Preserve scroll', () => { await page.goto('/visits/preserve-scroll') await expect(page.getByText('Foo is now default')).toBeVisible() - await page.evaluate(() => window.scrollTo(5, 7)) - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(10, 15)) - + await scrollElementTo( + page, + page.evaluate(() => window.scrollTo(5, 7)), + ) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(10, 15)), + ) await expect(page.getByText('Document scroll position is 5 & 7')).toBeVisible() await expect(page.getByText('Slot scroll position is 10 & 15')).toBeVisible() }) @@ -648,8 +669,10 @@ test.describe('Preserve scroll', () => { await expect(page).toHaveURL('/visits/preserve-scroll-page-two') - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(0, 0)) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(0, 0)), + ) await expect(page.getByText('Slot scroll position is 0 & 0')).toBeVisible() @@ -666,8 +689,10 @@ test.describe('Preserve scroll', () => { await expect(page).toHaveURL('/visits/preserve-scroll-page-two') - // @ts-ignore - await page.evaluate(() => document.querySelector('#slot').scrollTo(0, 0)) + await scrollElementTo( + page, + page.evaluate(() => document.querySelector('#slot')?.scrollTo(0, 0)), + ) await expect(page.getByText('Slot scroll position is 0 & 0')).toBeVisible() diff --git a/tests/support.ts b/tests/support.ts index 8e9135a8c..34bfe05bb 100644 --- a/tests/support.ts +++ b/tests/support.ts @@ -60,3 +60,9 @@ export const shouldBeDumpPage = async (page: Page, method: 'get' | 'post' | 'pat return dump } + +export const scrollElementTo = async (page: Page, promise: Promise) => { + await promise + // Wait for scroll listener debounce + await page.waitForTimeout(100) +} From 33b154248305feccffedaef552c0aa4904509b84 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 12:20:01 -0400 Subject: [PATCH 10/13] remove scroll regions prop from page as it does not exist anymore --- packages/core/src/initialVisit.ts | 1 - packages/core/src/page.ts | 6 +----- packages/vue3/src/app.ts | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/core/src/initialVisit.ts b/packages/core/src/initialVisit.ts index 9ea651818..d4689801f 100644 --- a/packages/core/src/initialVisit.ts +++ b/packages/core/src/initialVisit.ts @@ -66,7 +66,6 @@ export class InitialVisit { const rememberedState = history.getState(history.rememberedState, {}) const scrollRegions = history.getScrollRegions() currentPage.remember(rememberedState) - currentPage.scrollRegions(scrollRegions) currentPage .set(currentPage.get(), { diff --git a/packages/core/src/page.ts b/packages/core/src/page.ts index aa110aa3b..c87c17fea 100644 --- a/packages/core/src/page.ts +++ b/packages/core/src/page.ts @@ -56,8 +56,8 @@ class CurrentPage { return } - page.scrollRegions ??= [] page.rememberedState ??= {} + const location = typeof window !== 'undefined' ? window.location : new URL(page.url) replace = replace || isSameUrlWithoutHash(hrefToUrl(page.url), location) @@ -131,10 +131,6 @@ class CurrentPage { this.page.rememberedState = data } - public scrollRegions(regions: Page['scrollRegions']): void { - this.page.scrollRegions = regions - } - public swap({ component, page, diff --git a/packages/vue3/src/app.ts b/packages/vue3/src/app.ts index ea40fbb11..365465083 100755 --- a/packages/vue3/src/app.ts +++ b/packages/vue3/src/app.ts @@ -135,7 +135,6 @@ export function usePage(): Page { clearHistory: computed(() => page.value?.clearHistory), deferredProps: computed(() => page.value?.deferredProps), mergeProps: computed(() => page.value?.mergeProps), - scrollRegions: computed(() => page.value?.scrollRegions), rememberedState: computed(() => page.value?.rememberedState), encryptHistory: computed(() => page.value?.encryptHistory), }) From 5378ff9c0fe2b7e75673be3bfd6af47a507f51b7 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Mon, 21 Oct 2024 12:23:26 -0400 Subject: [PATCH 11/13] fixed method definitions --- packages/core/src/eventHandler.ts | 2 +- packages/core/src/page.ts | 2 +- packages/core/src/router.ts | 2 +- packages/core/src/scroll.ts | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/core/src/eventHandler.ts b/packages/core/src/eventHandler.ts index a60467383..0c8972723 100644 --- a/packages/core/src/eventHandler.ts +++ b/packages/core/src/eventHandler.ts @@ -72,7 +72,7 @@ class EventHandler { url.hash = window.location.hash history.replaceState({ ...currentPage.get(), url: url.href }) - Scroll.reset(currentPage.get()) + Scroll.reset() return } diff --git a/packages/core/src/page.ts b/packages/core/src/page.ts index c87c17fea..f87e01911 100644 --- a/packages/core/src/page.ts +++ b/packages/core/src/page.ts @@ -80,7 +80,7 @@ class CurrentPage { return this.swap({ component, page, preserveState }).then(() => { if (!preserveScroll) { - Scroll.reset(page) + Scroll.reset() } eventHandler.fireInternalEvent('loadDeferredProps') diff --git a/packages/core/src/router.ts b/packages/core/src/router.ts index 991a32820..00ccd7bfc 100644 --- a/packages/core/src/router.ts +++ b/packages/core/src/router.ts @@ -151,7 +151,7 @@ export class Router { if (!currentPage.isCleared() && !visit.preserveUrl) { // Save scroll regions for the current page - Scroll.save(currentPage.get()) + Scroll.save() } const requestParams: PendingVisit & VisitCallbacks = { diff --git a/packages/core/src/scroll.ts b/packages/core/src/scroll.ts index b810fe000..df1bb51be 100644 --- a/packages/core/src/scroll.ts +++ b/packages/core/src/scroll.ts @@ -1,5 +1,5 @@ import { history } from './history' -import { Page, ScrollRegion } from './types' +import { ScrollRegion } from './types' export class Scroll { public static save(): void { @@ -15,7 +15,7 @@ export class Scroll { return document.querySelectorAll('[scroll-region]') } - public static reset(page: Page): void { + public static reset(): void { if (typeof window !== 'undefined') { window.scrollTo(0, 0) } From b138d04f61b28bfc2d3b5b49ae031af3d67e5046 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Thu, 9 Jan 2025 15:04:54 -0500 Subject: [PATCH 12/13] fix merge error --- packages/core/src/page.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/page.ts b/packages/core/src/page.ts index 25621c0f1..1b2ea0d1c 100644 --- a/packages/core/src/page.ts +++ b/packages/core/src/page.ts @@ -95,7 +95,7 @@ class CurrentPage { return this.swap({ component, page, preserveState }).then(() => { if (!preserveScroll) { - Scroll.reset(page) + Scroll.reset() } eventHandler.fireInternalEvent('loadDeferredProps') From b9695b41f0078aafd3a95ba4b58a2e22856c07f2 Mon Sep 17 00:00:00 2001 From: Joe Tannenbaum Date: Thu, 9 Jan 2025 15:24:48 -0500 Subject: [PATCH 13/13] removed unused import --- .../svelte/test-app/Pages/Svelte/PropsAndPageStore.svelte | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/svelte/test-app/Pages/Svelte/PropsAndPageStore.svelte b/packages/svelte/test-app/Pages/Svelte/PropsAndPageStore.svelte index 17eb623bb..b3c57900e 100644 --- a/packages/svelte/test-app/Pages/Svelte/PropsAndPageStore.svelte +++ b/packages/svelte/test-app/Pages/Svelte/PropsAndPageStore.svelte @@ -1,9 +1,9 @@