From f66053a1ea0a4e3bdb0b0df12bb1bf56e1ea2618 Mon Sep 17 00:00:00 2001 From: Martin Trapp <94928215+martrapp@users.noreply.github.com> Date: Thu, 7 Sep 2023 20:21:57 +0200 Subject: [PATCH] Transition only between pages where both have ViewTransitions enabled (#8441) * added e2e test regarding loss of router * only navigate to pages from which we can navigate back * location does not change before deferred pushState * initialize history state * test cases adapted to new semantics (only traverse to pages w/ ViewTransigs) * type URL instead of Location * + changeset --- .changeset/curvy-dolls-thank.md | 5 ++ .../astro/components/ViewTransitions.astro | 72 ++++++++++-------- .../view-transitions/src/pages/five.astro | 11 +++ .../view-transitions/src/pages/three.astro | 2 + packages/astro/e2e/view-transitions.test.js | 76 ++++++++++++------- 5 files changed, 106 insertions(+), 60 deletions(-) create mode 100644 .changeset/curvy-dolls-thank.md create mode 100644 packages/astro/e2e/fixtures/view-transitions/src/pages/five.astro diff --git a/.changeset/curvy-dolls-thank.md b/.changeset/curvy-dolls-thank.md new file mode 100644 index 000000000..47ea8ad32 --- /dev/null +++ b/.changeset/curvy-dolls-thank.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Only transition between pages where both have ViewTransitions enabled diff --git a/packages/astro/components/ViewTransitions.astro b/packages/astro/components/ViewTransitions.astro index 74c338333..049355461 100644 --- a/packages/astro/components/ViewTransitions.astro +++ b/packages/astro/components/ViewTransitions.astro @@ -19,7 +19,9 @@ const { fallback = 'animate' } = Astro.props as Props; }; type Events = 'astro:page-load' | 'astro:after-swap'; - const persistState = (state: State) => history.replaceState(state, ''); + // only update history entries that are managed by us + // leave other entries alone and do not accidently add state. + const persistState = (state: State) => history.state && history.replaceState(state, ''); const supportsViewTransitions = !!document.startViewTransition; const transitionEnabledOnThisPage = () => !!document.querySelector('[name="astro-view-transitions-enabled"]'); @@ -32,11 +34,13 @@ const { fallback = 'animate' } = Astro.props as Props; // can use that to determine popstate if going forward or back. let currentHistoryIndex = 0; if (history.state) { - // we reloaded a page with history state (e.g. back button or browser reload) + // we reloaded a page with history state + // (e.g. history navigation from non-transition page or browser reload) currentHistoryIndex = history.state.index; scrollTo({ left: 0, top: history.state.scrollY }); + } else if (transitionEnabledOnThisPage()) { + history.replaceState({index: currentHistoryIndex, scrollY}, ''); } - const throttle = (cb: (...args: any[]) => any, delay: number) => { let wait = false; // During the waiting time additional events are lost. @@ -109,9 +113,7 @@ const { fallback = 'animate' } = Astro.props as Props; const parser = new DOMParser(); - async function updateDOM(html: string, state?: State, fallback?: Fallback) { - const doc = parser.parseFromString(html, 'text/html'); - + async function updateDOM(doc: Document, loc: URL, state?: State, fallback?: Fallback) { // Check for a head element that should persist, either because it has the data // attribute or is a link el. const persistedHeadElement = (el: Element): Element | null => { @@ -189,19 +191,21 @@ const { fallback = 'animate' } = Astro.props as Props; // Chromium based browsers (Chrome, Edge, Opera, ...) scrollTo({ left: 0, top: 0, behavior: 'instant' }); - if (state?.scrollY === 0 && location.hash) { - const id = decodeURIComponent(location.hash.slice(1)); + let initialScrollY = 0; + if (!state && loc.hash) { + const id = decodeURIComponent(loc.hash.slice(1)); const elem = document.getElementById(id); // prefer scrollIntoView() over scrollTo() because it takes scroll-padding into account - if (elem) { - state.scrollY = elem.offsetTop; - persistState(state); // first guess, later updated by scroll handler - elem.scrollIntoView(); // for Firefox, this should better be {behavior: 'instant'} - } + elem && (initialScrollY = elem.offsetTop) && elem.scrollIntoView(); } else if (state && state.scrollY !== 0) { scrollTo(0, state.scrollY); // usings default scrollBehavior } - + !state && + history.pushState( + { index: ++currentHistoryIndex, scrollY: initialScrollY }, + '', + loc.href + ); triggerEvent('astro:after-swap'); }; @@ -247,19 +251,26 @@ const { fallback = 'animate' } = Astro.props as Props; } } - async function navigate(dir: Direction, href: string, state?: State) { + async function navigate(dir: Direction, loc: URL, state?: State) { let finished: Promise; + const href=loc.href; const { html, ok } = await getHTML(href); // If there is a problem fetching the new page, just do an MPA navigation to it. if (!ok) { location.href = href; return; } + const doc = parser.parseFromString(html, 'text/html'); + if (!doc.querySelector('[name="astro-view-transitions-enabled"]')) { + location.href = href; + return; + } + document.documentElement.dataset.astroTransition = dir; if (supportsViewTransitions) { - finished = document.startViewTransition(() => updateDOM(html, state)).finished; + finished = document.startViewTransition(() => updateDOM(doc, loc, state)).finished; } else { - finished = updateDOM(html, state, getFallback()); + finished = updateDOM(doc, loc, state, getFallback()); } try { await finished; @@ -311,11 +322,11 @@ const { fallback = 'animate' } = Astro.props as Props; ev.shiftKey || // new window ev.defaultPrevented || !transitionEnabledOnThisPage() - ) + ) { // No page transitions in these cases, // Let the browser standard action handle this return; - + } // We do not need to handle same page links because there are no page transitions // Same page means same path and same query params (but different hash) if (location.pathname === link.pathname && location.search === link.search) { @@ -341,10 +352,8 @@ const { fallback = 'animate' } = Astro.props as Props; // these are the cases we will handle: same origin, different page ev.preventDefault(); - navigate('forward', link.href, { index: ++currentHistoryIndex, scrollY: 0 }); - const newState: State = { index: currentHistoryIndex, scrollY }; - persistState({ index: currentHistoryIndex - 1, scrollY }); - history.pushState(newState, '', link.href); + persistState({ index: currentHistoryIndex, scrollY }); + navigate('forward', new URL(link.href)); }); addEventListener('popstate', (ev) => { @@ -374,11 +383,11 @@ const { fallback = 'animate' } = Astro.props as Props; history.scrollRestoration = 'manual'; } - const state: State | undefined = history.state; - const nextIndex = state?.index ?? currentHistoryIndex + 1; + const state: State = history.state; + const nextIndex = state.index; const direction: Direction = nextIndex > currentHistoryIndex ? 'forward' : 'back'; - navigate(direction, location.href, state); currentHistoryIndex = nextIndex; + navigate(direction, new URL(location.href), state); }); ['mouseenter', 'touchstart', 'focus'].forEach((evName) => { @@ -402,13 +411,10 @@ const { fallback = 'animate' } = Astro.props as Props; addEventListener('load', onPageLoad); // There's not a good way to record scroll position before a back button. // So the way we do it is by listening to scrollend if supported, and if not continuously record the scroll position. - const updateState = () => { - // only update history entries that are managed by us - // leave other entries alone and do not accidently add state. - if (history.state) { - persistState({ ...history.state, scrollY }); - } - }; + const updateState = () => { + persistState({ ...history.state, scrollY }); + } + if ('onscrollend' in window) addEventListener('scrollend', updateState); else addEventListener('scroll', throttle(updateState, 300)); } diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/five.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/five.astro new file mode 100644 index 000000000..f17dd42b3 --- /dev/null +++ b/packages/astro/e2e/fixtures/view-transitions/src/pages/five.astro @@ -0,0 +1,11 @@ + + + Page 5 + + +
+

Page 5

+ go to 3 +
+ + diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro index eddc049a8..8324a9c72 100644 --- a/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro +++ b/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro @@ -7,6 +7,8 @@

Page 3

go to 2
+ go to 5 +
hash target

Long paragraph

diff --git a/packages/astro/e2e/view-transitions.test.js b/packages/astro/e2e/view-transitions.test.js index 4b4d64915..f5a63c67c 100644 --- a/packages/astro/e2e/view-transitions.test.js +++ b/packages/astro/e2e/view-transitions.test.js @@ -83,7 +83,7 @@ test.describe('View Transitions', () => { expect(loads.length, 'There should only be 1 page load').toEqual(1); }); - test('Moving from a page without ViewTransitions triggers a full page navigation', async ({ + test('Moving to a page without ViewTransitions triggers a full page navigation', async ({ page, astro, }) => { @@ -102,10 +102,6 @@ test.describe('View Transitions', () => { p = page.locator('#three'); await expect(p, 'should have content').toHaveText('Page 3'); - await page.click('#click-two'); - p = page.locator('#two'); - await expect(p, 'should have content').toHaveText('Page 2'); - expect( loads.length, 'There should be 2 page loads. The original, then going from 3 to 2' @@ -142,8 +138,8 @@ test.describe('View Transitions', () => { expect( loads.length, - 'There should be only 1 page load. The original, but no additional loads for the hash change' - ).toEqual(1); + 'There should be only 2 page loads (for page one & three), but no additional loads for the hash change' + ).toEqual(2); }); test('Moving from a page without ViewTransitions w/ back button', async ({ page, astro }) => { @@ -501,25 +497,51 @@ test.describe('View Transitions', () => { await page.click('#click-logo'); await downloadPromise; }); + + test('Scroll position is restored on back navigation from page w/o ViewTransitions', async ({ + page, + astro, + }) => { + // Go to middle of long page + await page.goto(astro.resolveUrl('/long-page#click-external')); + + let locator = page.locator('#click-external'); + await expect(locator).toBeInViewport(); + + // Go to a page that has not enabled ViewTransistions + await page.click('#click-external'); + locator = page.locator('#three'); + await expect(locator).toHaveText('Page 3'); + + // Scroll back to long page + await page.goBack(); + locator = page.locator('#click-external'); + await expect(locator).toBeInViewport(); + }); + + test("Non transition navigation doesn't loose handlers", async ({ page, astro }) => { + // Go to page 1 + await page.goto(astro.resolveUrl('/one')); + let p = page.locator('#one'); + await expect(p, 'should have content').toHaveText('Page 1'); + + // go to page 3 + await page.click('#click-three'); + p = page.locator('#three'); + await expect(p, 'should have content').toHaveText('Page 3'); + + // go to page 5 + await page.click('#click-five'); + p = page.locator('#five'); + await expect(p, 'should have content').toHaveText('Page 5'); + + await page.goBack(); + p = page.locator('#three'); + await expect(p, 'should have content').toHaveText('Page 3'); + + await page.goBack(); + p = page.locator('#one'); + await expect(p, 'should have content').toHaveText('Page 1'); + }); }); -test('Scroll position is restored on back navigation from page w/o ViewTransitions', async ({ - page, - astro, -}) => { - // Go to middle of long page - await page.goto(astro.resolveUrl('/long-page#click-external')); - - let locator = page.locator('#click-external'); - await expect(locator).toBeInViewport(); - - // Go to a page that has not enabled ViewTransistions - await page.click('#click-external'); - locator = page.locator('#three'); - await expect(locator).toHaveText('Page 3'); - - // Scroll back to long page - await page.goBack(); - locator = page.locator('#click-external'); - await expect(locator).toBeInViewport(); -});