diff --git a/package.json b/package.json index 83d005fa..99fead62 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "impro", - "version": "0.14.119", + "version": "0.14.120", "type": "module", "scripts": { "start": "rm -rf build && NODE_ENV=development eleventy --serve", diff --git a/src/js/router.js b/src/js/router.js index 63c5ad8e..6c721d66 100644 --- a/src/js/router.js +++ b/src/js/router.js @@ -35,6 +35,7 @@ export class Router extends EventEmitter { this.renderFunc = () => {}; this.container = null; this.currentPage = null; + this.currentPath = null; this.pages = new Map(); this.scrollStates = new Map(); // Disable scroll restoration @@ -42,7 +43,9 @@ export class Router extends EventEmitter { // on back button, go back to the previous page window.addEventListener("popstate", async (e) => { this.emit("navigate"); - await this.load(window.location.pathname, { isBack: true }); + await this.load(window.location.pathname + window.location.search, { + isBack: true, + }); }); } @@ -101,6 +104,11 @@ export class Router extends EventEmitter { } async load(path, { isBack = false } = {}) { + // Save the scroll position of the page we're leaving before swapping it out + if (this.currentPath != null) { + this.scrollStates.set(this.currentPath, window.scrollY); + } + this.currentPath = path; // used to pause videos on page exit, among other things window.dispatchEvent(new CustomEvent("page-transition")); // Strip query parameters for route matching (but keep full path for caching) @@ -162,7 +170,6 @@ export class Router extends EventEmitter { } async go(path) { - this.scrollStates.set(window.location.pathname, window.scrollY); window.history.pushState( { previousRoute: window.location.pathname }, "", @@ -173,7 +180,6 @@ export class Router extends EventEmitter { } async back() { - this.scrollStates.set(window.location.pathname, window.scrollY); if (!!window.history.state?.previousRoute) { window.history.back(); } else { diff --git a/src/js/views/postThread.view.js b/src/js/views/postThread.view.js index 8e6ee795..e8855887 100644 --- a/src/js/views/postThread.view.js +++ b/src/js/views/postThread.view.js @@ -570,7 +570,8 @@ class PostThreadView extends View { if (largePost) { const headerHeight = root.querySelector("header").offsetHeight; const largePostTop = largePost.offsetTop; - window.scrollTo(0, largePostTop - headerHeight); + // Preserve any scrolling the user did while the skeleton was loading + window.scrollTo(0, largePostTop - headerHeight + window.scrollY); } } @@ -585,11 +586,7 @@ class PostThreadView extends View { root.addEventListener("page-restore", async (e) => { const scrollY = e.detail?.scrollY ?? 0; - if (scrollY > 0) { - window.scrollTo(0, scrollY); - } else { - scrollToLargePost(); - } + window.scrollTo(0, scrollY); // Revalidate await dataLayer.requests.loadPostThread(postUri); }); diff --git a/tests/e2e/mockServer.js b/tests/e2e/mockServer.js index 4e75961c..b3779c58 100644 --- a/tests/e2e/mockServer.js +++ b/tests/e2e/mockServer.js @@ -38,6 +38,7 @@ export class MockServer { this.postReposts = new Map(); this.postThreadOthers = new Map(); this.postThreads = new Map(); + this.postThreadDelays = new Map(); this.profileFollowers = new Map(); this.profileFollows = new Map(); this.profiles = new Map(); @@ -141,8 +142,11 @@ export class MockServer { this.postReposts.set(postUri, reposts); } - setPostThread(postUri, thread) { + setPostThread(postUri, thread, { delayMs = 0 } = {}) { this.postThreads.set(postUri, thread); + if (delayMs > 0) { + this.postThreadDelays.set(postUri, delayMs); + } } setPostThreadOther(postUri, threadOther) { @@ -1005,11 +1009,15 @@ export class MockServer { }); }); - await page.route("**/xrpc/app.bsky.feed.getPostThread*", (route) => { + await page.route("**/xrpc/app.bsky.feed.getPostThread*", async (route) => { const url = new URL(route.request().url()); const uri = url.searchParams.get("uri"); const customThread = this.postThreads.get(uri); if (customThread) { + const delayMs = this.postThreadDelays.get(uri); + if (delayMs) { + await new Promise((resolve) => setTimeout(resolve, delayMs)); + } return route.fulfill({ status: 200, contentType: "application/json", diff --git a/tests/e2e/specs/views/postThread.view.test.js b/tests/e2e/specs/views/postThread.view.test.js index 8e3d5942..2e663772 100644 --- a/tests/e2e/specs/views/postThread.view.test.js +++ b/tests/e2e/specs/views/postThread.view.test.js @@ -153,6 +153,99 @@ test.describe("Post thread view", () => { expect(postTop).toBeLessThanOrEqual(headerHeight + 8); }); + test("should add the user's pre-load scroll to the scroll-to-main-post offset", async ({ + page, + }) => { + // Build a deep parent chain so the main post starts below the fold. + const NUM_PARENTS = 12; + const parents = Array.from({ length: NUM_PARENTS }).map((_, index) => + createPost({ + uri: `at://did:plc:parent${index}/app.bsky.feed.post/parent${index}`, + text: `Parent post number ${index}`, + authorHandle: `parent${index}.bsky.social`, + authorDisplayName: `Parent ${index}`, + }), + ); + const rootParent = parents[0]; + const immediateParent = parents[NUM_PARENTS - 1]; + + const childPost = createPost({ + uri: postUri, + text: "This is the main post we should scroll to", + authorHandle: "author1.bsky.social", + authorDisplayName: "Author One", + reply: { + parent: { uri: immediateParent.uri, cid: immediateParent.cid }, + root: { uri: rootParent.uri, cid: rootParent.cid }, + }, + }); + + let parentNode = null; + for (const parentPost of parents) { + parentNode = { + $type: "app.bsky.feed.defs#threadViewPost", + post: parentPost, + parent: parentNode, + replies: [], + }; + } + + const mockServer = new MockServer(); + mockServer.addPosts([childPost, ...parents]); + // Delay the thread response so the skeleton is shown long enough for the + // user to scroll before the full thread (with parents) loads. + mockServer.setPostThread( + postUri, + { + $type: "app.bsky.feed.defs#threadViewPost", + post: childPost, + parent: parentNode, + replies: [], + }, + { delayMs: 1500 }, + ); + await mockServer.setup(page); + + await login(page); + // A short viewport guarantees the loading skeleton is tall enough to scroll. + await page.setViewportSize({ width: 400, height: 200 }); + await page.goto("/profile/author1.bsky.social/post/abc123"); + + const view = page.locator("#post-detail-view"); + + // Wait until the loading skeleton has rendered (the router resets scroll to + // the top as part of navigation, so we must scroll after that happens), then + // scroll down while the full thread is still loading. + await expect( + view.locator('[data-testid="post-skeleton"]').first(), + ).toBeVisible(); + await expect(view.locator('[data-testid="large-post"]')).toHaveCount(0); + const extraScroll = 80; + await page.evaluate((y) => window.scrollTo(0, y), extraScroll); + const actualExtraScroll = await page.evaluate(() => window.scrollY); + expect(actualExtraScroll).toBeGreaterThan(0); + + const largePost = view.locator('[data-testid="large-post"]'); + await expect(largePost).toBeVisible({ timeout: 10000 }); + + // The page should settle so the main post sits the user's extra scroll + // *past* the just-below-header position, rather than jumping back to it. + const headerHeight = await view + .locator("header") + .evaluate((el) => el.offsetHeight); + await expect + .poll(() => largePost.evaluate((el) => el.getBoundingClientRect().top), { + timeout: 10000, + }) + .toBeLessThanOrEqual(headerHeight - actualExtraScroll + 8); + const postTop = await largePost.evaluate( + (el) => el.getBoundingClientRect().top, + ); + expect(postTop).toBeGreaterThanOrEqual( + headerHeight - actualExtraScroll - 8, + ); + }); + test("should show 'Load parent post' link when reply ref is broken", async ({ page, }) => { diff --git a/tests/unit/specs/router.test.js b/tests/unit/specs/router.test.js index 01846463..eb33b507 100644 --- a/tests/unit/specs/router.test.js +++ b/tests/unit/specs/router.test.js @@ -220,6 +220,41 @@ t.describe("popstate", (it) => { assertEquals(order[0], "navigate"); }); + + it("restores a query-bearing page from cache on back navigation", async () => { + const originalPath = + window.location.pathname + window.location.search + window.location.hash; + const originalState = window.history.state; + const { router, popstateHandler } = createRouterWithPopstateHandler(); + const container = document.createElement("div"); + router.mount(container); + router.addRoute("/search", () => Promise.resolve({})); + router.addRoute("/other", () => Promise.resolve({})); + router.renderRoute(() => {}); + + try { + await router.load("/search?q=alice"); + const searchPage = router.pages.get("/search?q=alice"); + assert(searchPage, "page should be cached under its full path"); + await router.load("/other"); + + // Simulate the back button landing on the query-bearing URL. + window.history.replaceState({}, "", "/search?q=alice"); + await popstateHandler(new Event("popstate")); + + // The cached page is reused rather than rebuilt under the query-less path. + assert( + router.currentPage === searchPage, + "should reuse the cached query-bearing page", + ); + assert( + !router.pages.has("/search"), + "should not create a query-less duplicate page", + ); + } finally { + window.history.replaceState(originalState, "", originalPath); + } + }); }); t.describe("load", (it) => { @@ -407,4 +442,72 @@ t.describe("back", (it) => { }); }); +t.describe("scroll position persistence", (it) => { + // JSDOM's window.scrollY is a read-only getter, so temporarily override it to + // simulate the page being scrolled before we navigate away. + function withScrollY(value, callback) { + const original = Object.getOwnPropertyDescriptor(window, "scrollY"); + Object.defineProperty(window, "scrollY", { + value, + configurable: true, + }); + return (async () => { + try { + return await callback(); + } finally { + if (original) { + Object.defineProperty(window, "scrollY", original); + } else { + delete window.scrollY; + } + } + })(); + } + + function createRouter() { + const router = new Router(); + const container = document.createElement("div"); + router.mount(container); + router.addRoute("/a", () => Promise.resolve({})); + router.addRoute("/b", () => Promise.resolve({})); + router.renderRoute(() => {}); + return router; + } + + it("saves the scroll position of the page being navigated away from", async () => { + const router = createRouter(); + await router.load("/a"); + + await withScrollY(250, () => router.load("/b")); + + assertEquals(router.scrollStates.get("/a"), 250); + }); + + it("does not record a scroll position on the very first load", async () => { + const router = createRouter(); + + await withScrollY(250, () => router.load("/a")); + + assertEquals(router.scrollStates.has("/a"), false); + }); + + it("restores the saved scroll position via the page-restore event", async () => { + const router = createRouter(); + await router.load("/a"); + + const pageA = router.pages.get("/a"); + let restoredScrollY = null; + pageA.addEventListener("page-restore", (event) => { + restoredScrollY = event.detail.scrollY; + }); + + await withScrollY(175, async () => { + await router.load("/b"); // leaving /a saves 175 under /a + await router.load("/a"); // returning to cached /a restores it + }); + + assertEquals(restoredScrollY, 175); + }); +}); + await t.run();