Fixes in the client-side router (#8166)

* Fixes in the client-side router * reverted function declaration after review (#8166) --------- Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
withastro · Aug 22, 2023 · fddd4dc · fddd4dc
1 parent cfc465d
commit fddd4dc
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 9 deletions.
diff --git a/.changeset/chilled-shoes-fail.md b/.changeset/chilled-shoes-fail.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+ViewTransitions: Fixes in the client-side router
diff --git a/packages/astro/components/ViewTransitions.astro b/packages/astro/components/ViewTransitions.astro
@@ -20,22 +20,21 @@ const { fallback = 'animate' } = Astro.props as Props;
 	type Events = 'astro:load' | 'astro:beforeload';
 
 	const persistState = (state: State) => history.replaceState(state, '');
+	const supportsViewTransitions = !!document.startViewTransition;
+	const transitionEnabledOnThisPage = () =>
+		!!document.querySelector('[name="astro-view-transitions-enabled"]');
+	const triggerEvent = (name: Events) => document.dispatchEvent(new Event(name));
+	const onload = () => triggerEvent('astro:load');
+	const PERSIST_ATTR = 'data-astro-transition-persist';
 
 	// The History API does not tell you if navigation is forward or back, so
 	// you can figure it using an index. On pushState the index is incremented so you
 	// can use that to determine popstate if going forward or back.
 	let currentHistoryIndex = history.state?.index || 0;
-	if (!history.state) {
+	if (!history.state && transitionEnabledOnThisPage()) {
 		persistState({ index: currentHistoryIndex, scrollY: 0 });
 	}
 
-	const supportsViewTransitions = !!document.startViewTransition;
-	const transitionEnabledOnThisPage = () =>
-		!!document.querySelector('[name="astro-view-transitions-enabled"]');
-	const triggerEvent = (name: Events) => document.dispatchEvent(new Event(name));
-	const onload = () => triggerEvent('astro:load');
-	const PERSIST_ATTR = 'data-astro-transition-persist';
-
 	const throttle = (cb: (...args: any[]) => any, delay: number) => {
 		let wait = false;
 		// During the waiting time additional events are lost.
@@ -323,9 +322,10 @@ const { fallback = 'animate' } = Astro.props as Props;
 		});
 
 		addEventListener('popstate', (ev) => {
-			if (!transitionEnabledOnThisPage()) {
+			if (!transitionEnabledOnThisPage() && ev.state) {
 				// The current page doesn't haven't View Transitions,
 				// respect that with a full page reload
+				// -- but only for transition managed by us (ev.state is set)
 				location.reload();
 				return;
 			}

diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/half-baked.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/half-baked.astro
@@ -0,0 +1,24 @@
+---
+import { ViewTransitions } from 'astro:transitions';
+
+// For the test fixture, we import the script but we don't use the <ViewTransitions /> component
+// While this seems to be some strange mistake,
+// it might be realistic, e.g. in a configurable CommenHead component
+
+interface Props {
+	transitions?: string;
+}
+const { transitions } = Astro.props;
+---
+<html>
+	<head>
+		<title>Half-Baked</title>
+		{transitions && <ViewTransitions />}
+	</head>
+	<body>
+		<main>
+			<p id="half-baked">Half Baked</p>
+			<a id="click-hash" href="#click-hash">hash target</a>
+		</main>
+	</body>
+</html>
diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro
@@ -6,6 +6,9 @@
 		<main>
 			<p id="three">Page 3</p>
 			<a id="click-two" href="/two">go to 2</a>
+			<br/>
+			<a id="click-hash" href="#click-hash">hash target</a>
+			<p style="height: 150vh">Long paragraph</p>
 		</main>
 	</body>
 </html>
diff --git a/packages/astro/e2e/view-transitions.test.js b/packages/astro/e2e/view-transitions.test.js
@@ -112,6 +112,40 @@ test.describe('View Transitions', () => {
 		).toEqual(2);
 	});
 
+	test('Moving within a page without ViewTransitions does not trigger a full page navigation', async ({
+		page,
+		astro,
+	}) => {
+		const loads = [];
+		page.addListener('load', async (p) => {
+			loads.push(p.title());
+		});
+		// 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 which does *not* have ViewTransitions enabled
+		await page.click('#click-three');
+		p = page.locator('#three');
+		await expect(p, 'should have content').toHaveText('Page 3');
+
+		// click a hash link to navigate further down the page
+		await page.click('#click-hash');
+		// still on page 3
+		p = page.locator('#three');
+		await expect(p, 'should have content').toHaveText('Page 3');
+
+		// check that we are further down the page
+		const Y = await page.evaluate(() => window.scrollY);
+		expect(Y, 'The target is further down the page').toBeGreaterThan(0);
+
+		expect(
+			loads.length,
+			'There should be only 1 page load. The original, but no additional loads for the hash change'
+		).toEqual(1);
+	});
+
 	test('Moving from a page without ViewTransitions w/ back button', async ({ page, astro }) => {
 		const loads = [];
 		page.addListener('load', (p) => {
@@ -332,4 +366,34 @@ test.describe('View Transitions', () => {
 
 		await expect(loads.length, 'There should only be 1 page load').toEqual(1);
 	});
+
+	test('Importing ViewTransitions w/o using the component must not mess with history', async ({
+		page,
+		astro,
+	}) => {
+		const loads = [];
+		page.addListener('load', async (p) => {
+			loads.push(p);
+		});
+		// Go to the half bakeed page
+		await page.goto(astro.resolveUrl('/half-baked'));
+		let p = page.locator('#half-baked');
+		await expect(p, 'should have content').toHaveText('Half Baked');
+
+		// click a hash link to navigate further down the page
+		await page.click('#click-hash');
+		// still on page
+		p = page.locator('#half-baked');
+		await expect(p, 'should have content').toHaveText('Half Baked');
+
+		// go back within same page without reloading
+		await page.goBack();
+		p = page.locator('#half-baked');
+		await expect(p, 'should have content').toHaveText('Half Baked');
+
+		expect(
+			loads.length,
+			'There should be only 1 page load. No additional loads for going back on same page'
+		).toEqual(1);
+	});
 });