Chores: Tidying up the view transition router

packages/astro/e2e/fixtures/view-transitions/src/components/listener-layout.astro

@@ -10,25 +10,20 @@ import { ViewTransitions } from 'astro:transitions';
 		<slot/>
 		<script>
 			document.addEventListener("astro:after-swap", () => {
-				document.querySelector("p").addEventListener("transitionstart", () => {
-					console.info("transitionstart");
-				});
 				document.documentElement.setAttribute("class", "blue");
+				document.documentElement.setAttribute("animations", "" + document.getAnimations().length);
 			});
-			document.dispatchEvent(new Event("astro:after-swap"));
 		</script>
 	</body>
 	<style>
 		p {
-			transition: background-color 1s;
+			transition: color 1s;
 		}
 		p {
-			background-color: #0ee;
 			color: red;
 		}
 		.blue p {
-			background-color: #ee0;
-			color: blue;
+			color: #0000ff;
 		}
 	</style>
 </html>

packages/astro/e2e/view-transitions.test.js

@@ -1,5 +1,5 @@
 import { expect } from '@playwright/test';
-import { testFactory } from './test-utils.js';
+import { testFactory, waitForHydrate } from './test-utils.js';
 
 const test = testFactory({ root: './fixtures/view-transitions/' });
 
@@ -231,25 +231,37 @@ test.describe('View Transitions', () => {
 	});
 
 	test('No page rendering during swap()', async ({ page, astro }) => {
-		let transitions = 0;
-		page.on('console', (msg) => {
-			if (msg.type() === 'info' && msg.text() === 'transitionstart') ++transitions;
-		});
+		// This has been a problem with theme switchers (e.g. for drakmode)
+		// Swap() should not trigger any page renders and give users the chance to
+		// correct attributes in the astro:after-swap handler before they become visible
+
+		// This test uses a CSS animation to detect page rendering
+		// The test succeeds if no additional animation beside those of the
+		// view transition is triggered during swap()
 
-		// Go to page 1
 		await page.goto(astro.resolveUrl('/listener-one'));
 		let p = page.locator('#totwo');
 		await expect(p, 'should have content').toHaveText('Go to listener two');
-		// on load a CSS transition is started triggered by a class on the html element
-		expect(transitions).toBeLessThanOrEqual(1);
-		const transitionsBefore = transitions;
+
+		// setting the blue class on the html element triggers a CSS animation
+		let animations = await page.evaluate(async () => {
+			document.documentElement.classList.add('blue');
+			return document.getAnimations();
+		});
+		expect(animations.length).toEqual(1);
+
 		// go to page 2
 		await page.click('#totwo');
 		p = page.locator('#toone');
 		await expect(p, 'should have content').toHaveText('Go to listener one');
-		// swap() resets that class, the after-swap listener sets it again.
-		// the temporarily missing class must not trigger page rendering
-		expect(transitions).toEqual(transitionsBefore);
+		// swap() resets the "blue" class, as it is not set in the static html of page 2
+		// The astro:after-swap listener (defined in the layout) sets it to "blue" again.
+		// The temporarily missing class must not trigger page rendering.
+
+		// When the after-swap listener starts, no animations should be running
+		// after-swap listener sets animations to document.getAnimations().length
+		// and we expect this to be zero
+		await expect(page.locator('html')).toHaveAttribute('animations', '0');
 	});
 
 	test('click hash links does not do navigation', async ({ page, astro }) => {
@@ -663,10 +675,11 @@ test.describe('View Transitions', () => {
 		// go to external page
 		await page.click('#click-redirect-external');
 		// doesn't work for playwright when we are too fast
-		await page.waitForTimeout(1000);
 		p = page.locator('h1');
-		await expect(p, 'should have content').toBeVisible();
 
+		await expect(p, 'should have content').toBeVisible();
+		await page.waitForURL('http://example.com');
+		await page.waitForFunction((arr) => arr.length === 2, loads);
 		expect(loads.length, 'There should be 2 page loads').toEqual(2);
 	});
 
@@ -696,7 +709,8 @@ test.describe('View Transitions', () => {
 		await page.goto(astro.resolveUrl('/client-only-three'));
 		let msg = page.locator('#name');
 		await expect(msg).toHaveText('client-only-three');
-		await page.waitForTimeout(400); // await hydration
+		await waitForHydrate(page, page.getByText('Vue'));
+		await waitForHydrate(page, page.getByText('Svelte'));
 
 		let styles = await page.locator('style').all();
 		expect(styles.length).toEqual(totalExpectedStyles_page_three);

packages/astro/src/transitions/router.ts

@@ -12,7 +12,8 @@ type Events = 'astro:page-load' | 'astro:after-swap';
 
 // 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 updateScrollPosition = (positions: { scrollX: number; scrollY: number }) =>
+	history.state && history.replaceState({ ...history.state, ...positions }, '');
 
 const inBrowser = import.meta.env.SSR === false;
 
@@ -58,11 +59,13 @@ let currentHistoryIndex = 0;
 
 if (inBrowser) {
 	if (history.state) {
-		// we reloaded a page with history state
+		// Here we reloaded a page with history state
 		// (e.g. history navigation from non-transition page or browser reload)
 		currentHistoryIndex = history.state.index;
 		scrollTo({ left: history.state.scrollX, top: history.state.scrollY });
 	} else if (transitionEnabledOnThisPage()) {
+		// This page is loaded from the browser addressbar or via a link from extern,
+		// it needs a state in the history
 		history.replaceState({ index: currentHistoryIndex, scrollX, scrollY, intraPage: false }, '');
 	}
 }
@@ -122,12 +125,6 @@ function getFallback(): Fallback {
 	return 'animate';
 }
 
-function markScriptsExec() {
-	for (const script of document.scripts) {
-		script.dataset.astroExec = '';
-	}
-}
-
 function runScripts() {
 	let wait = Promise.resolve();
 	for (const script of Array.from(document.scripts)) {
@@ -156,7 +153,9 @@ function isInfinite(animation: Animation) {
 	return style.animationIterationCount === 'infinite';
 }
 
-const updateHistoryAndScrollPosition = (toLocation: URL, replace: boolean, intraPage: boolean) => {
+// Add a new entry to the browser history. This also sets the new page in the browser addressbar.
+// Sets the scroll position according to the hash fragment of the new location.
+const moveToLocation = (toLocation: URL, replace: boolean, intraPage: boolean) => {
 	const fresh = !samePage(toLocation);
 	let scrolledToTop = false;
 	if (toLocation.href !== location.href) {
@@ -190,6 +189,32 @@ const updateHistoryAndScrollPosition = (toLocation: URL, replace: boolean, intra
 	}
 };
 
+function stylePreloadLinks(newDocument: Document) {
+	const links: Promise<any>[] = [];
+	for (const el of newDocument.querySelectorAll('head link[rel=stylesheet]')) {
+		// Do not preload links that are already on the page.
+		if (
+			!document.querySelector(
+				`[${PERSIST_ATTR}="${el.getAttribute(
+					PERSIST_ATTR
+				)}"], link[rel=stylesheet][href="${el.getAttribute('href')}"]`
+			)
+		) {
+			const c = document.createElement('link');
+			c.setAttribute('rel', 'preload');
+			c.setAttribute('as', 'style');
+			c.setAttribute('href', el.getAttribute('href')!);
+			links.push(
+				new Promise<any>((resolve) => {
+					['load', 'error'].forEach((evName) => c.addEventListener(evName, resolve));
+					document.head.append(c);
+				})
+			);
+		}
+	}
+	return links;
+}
+
 // replace head and body of the windows document with contents from newDocument
 // if !popstate, update the history entry and scroll position according to toLocation
 // if popState is given, this holds the scroll position for history navigation
@@ -204,9 +229,8 @@ async function updateDOM(
 	// Check for a head element that should persist and returns it,
 	// either because it has the data attribute or is a link el.
 	// Returns null if the element is not part of the new head, undefined if it should be left alone.
-	const persistedHeadElement = (el: HTMLElement): Element | null | undefined => {
+	const persistedHeadElement = (el: HTMLElement): Element | null => {
 		const id = el.getAttribute(PERSIST_ATTR);
-		if (id === '') return undefined;
 		const newEl = id && newDocument.head.querySelector(`[${PERSIST_ATTR}="${id}"]`);
 		if (newEl) {
 			return newEl;
@@ -293,7 +317,7 @@ async function updateDOM(
 			// from the new document and leave the current node alone
 			if (newEl) {
 				newEl.remove();
-			} else if (newEl === null) {
+			} else {
 				// Otherwise remove the element in the head. It doesn't exist in the new page.
 				el.remove();
 			}
@@ -324,35 +348,13 @@ async function updateDOM(
 		if (popState) {
 			scrollTo(popState.scrollX, popState.scrollY); // usings 'auto' scrollBehavior
 		} else {
-			updateHistoryAndScrollPosition(toLocation, options.history === 'replace', false);
+			moveToLocation(toLocation, options.history === 'replace', false);
 		}
 
 		triggerEvent('astro:after-swap');
 	};
 
-	// Wait on links to finish, to prevent FOUC
-	const links: Promise<any>[] = [];
-	for (const el of newDocument.querySelectorAll('head link[rel=stylesheet]')) {
-		// Do not preload links that are already on the page.
-		if (
-			!document.querySelector(
-				`[${PERSIST_ATTR}="${el.getAttribute(
-					PERSIST_ATTR
-				)}"], link[rel=stylesheet][href="${el.getAttribute('href')}"]`
-			)
-		) {
-			const c = document.createElement('link');
-			c.setAttribute('rel', 'preload');
-			c.setAttribute('as', 'style');
-			c.setAttribute('href', el.getAttribute('href')!);
-			links.push(
-				new Promise<any>((resolve) => {
-					['load', 'error'].forEach((evName) => c.addEventListener(evName, resolve));
-					document.head.append(c);
-				})
-			);
-		}
-	}
+	const links = stylePreloadLinks(newDocument);
 	links.length && (await Promise.all(links));
 
 	if (fallback === 'animate') {
@@ -363,12 +365,9 @@ async function updateDOM(
 			.getAnimations()
 			.filter((a) => !currentAnimations.includes(a) && !isInfinite(a));
 		const finished = Promise.all(newAnimations.map((a) => a.finished));
-		const fallbackSwap = () => {
-			swap();
-			document.documentElement.dataset.astroTransitionFallback = 'new';
-		};
 		await finished;
-		fallbackSwap();
+		swap();
+		document.documentElement.dataset.astroTransitionFallback = 'new';
 	} else {
 		swap();
 	}
@@ -427,7 +426,6 @@ async function transition(
 		// skip this for the moment as it tends to stop fallback animations
 		// document.documentElement.removeAttribute('data-astro-transition');
 		await runScripts();
-		markScriptsExec();
 		onPageLoad();
 		announce();
 	}
@@ -460,7 +458,7 @@ export function navigate(href: string, options?: Options) {
 	// but we want to handle prevent reload on navigation to the same page
 	// Same page means same origin, path and query params (but maybe different hash)
 	if (location.origin === toLocation.origin && samePage(toLocation)) {
-		updateHistoryAndScrollPosition(toLocation, options?.history === 'replace', true);
+		moveToLocation(toLocation, options?.history === 'replace', true);
 	} else {
 		// different origin will be detected by fetch
 		transition('forward', toLocation, options ?? {});
@@ -509,20 +507,21 @@ function onPopState(ev: PopStateEvent) {
 	}
 }
 
+// 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 onScroll = () => {
+	updateScrollPosition({ scrollX, scrollY });
+};
+
 if (inBrowser) {
 	if (supportsViewTransitions || getFallback() !== 'none') {
 		addEventListener('popstate', onPopState);
 		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 = () => {
-			persistState({ ...history.state, scrollX, scrollY });
-		};
-
-		if ('onscrollend' in window) addEventListener('scrollend', updateState);
-		else addEventListener('scroll', throttle(updateState, 300));
-
-		markScriptsExec();
+		if ('onscrollend' in window) addEventListener('scrollend', onScroll);
+		else addEventListener('scroll', throttle(onScroll, 300));
+	}
+	for (const script of document.scripts) {
+		script.dataset.astroExec = '';
 	}
 }
 
@@ -549,12 +548,11 @@ async function prepareForClientOnlyComponents(newDocument: Document, toLocation:
 			const viteIds = [...nextHead.querySelectorAll(`style[${VITE_ID}]`)].map((style) =>
 				style.getAttribute(VITE_ID)
 			);
-			// Mark styles of the current head as persistent
-			// if they come from hydration and not from the newDocument
+			// Copy required styles to the new document if they are from hydration.
 			viteIds.forEach((id) => {
 				const style = document.head.querySelector(`style[${VITE_ID}="${id}"]`);
 				if (style && !newDocument.head.querySelector(`style[${VITE_ID}="${id}"]`)) {
-					style.setAttribute(PERSIST_ATTR, '');
+					newDocument.head.appendChild(style);
 				}
 			});
 		}