Clean-up router implementation (#8617)

* Update regarding review comments from #8571

* Update regarding review comments from #8571 (2)

* Update regarding review comments from #8571 (3)

* Update regarding review comments from #8571 (4)

2 files changed, 149 insertions, 104 deletions

diff --git a/packages/astro/components/ViewTransitions.astro b/packages/astro/components/ViewTransitions.astro
index 39b9996ce..aa266af13 100644
--- a/packages/astro/components/ViewTransitions.astro
+++ b/packages/astro/components/ViewTransitions.astro
@@ -17,18 +17,26 @@ const { fallback = 'animate' } = Astro.props as Props;
 		index: number;
 		scrollX: number;
 		scrollY: number;
+		intraPage?: boolean;
 	};
 	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, '');
+	// @ts-expect-error: startViewTransition might exist
 	const supportsViewTransitions = !!document.startViewTransition;
 	const transitionEnabledOnThisPage = () =>
 		!!document.querySelector('[name="astro-view-transitions-enabled"]');
 	const triggerEvent = (name: Events) => document.dispatchEvent(new Event(name));
 	const onPageLoad = () => triggerEvent('astro:page-load');
 	const PERSIST_ATTR = 'data-astro-transition-persist';
+	const parser = new DOMParser();
+	// explained at its usage
+	let noopEl: HTMLDivElement;
+	if (import.meta.env.DEV) {
+		noopEl = document.createElement('div');
+	}
 
 	// 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
@@ -40,7 +48,7 @@ const { fallback = 'animate' } = Astro.props as Props;
 		currentHistoryIndex = history.state.index;
 		scrollTo({ left: history.state.scrollX, top: history.state.scrollY });
 	} else if (transitionEnabledOnThisPage()) {
-		history.replaceState({ index: currentHistoryIndex, scrollX, scrollY }, '');
+		history.replaceState({ index: currentHistoryIndex, scrollX, scrollY, intraPage: false }, '');
 	}
 	const throttle = (cb: (...args: any[]) => any, delay: number) => {
 		let wait = false;
@@ -64,19 +72,28 @@ const { fallback = 'animate' } = Astro.props as Props;
 		};
 	};
 
-	async function getHTML(href: string) {
+	// returns the contents of the page or null if the router can't deal with it.
+	async function fetchHTML(
+		href: string
+	): Promise<null | { html: string; redirected?: string; mediaType: DOMParserSupportedType }> {
 		try {
 			const res = await fetch(href);
+			// drop potential charset (+ other name/value pairs) as parser needs the mediaType
+			const mediaType = res.headers.get('content-type')?.replace(/;.*$/, '');
+			// the DOMParser can handle two types of HTML
+			if (mediaType !== 'text/html' && mediaType !== 'application/xhtml+xml') {
+				// everything else (e.g. audio/mp3) will be handled by the browser but not by us
+				return null;
+			}
 			const html = await res.text();
 			return {
-				ok: res.ok,
 				html,
 				redirected: res.redirected ? res.url : undefined,
-				// drop potential charset (+ other name/value pairs) as parser needs the mediaType
-				mediaType: res.headers.get('content-type')?.replace(/;.*$/, ''),
+				mediaType,
 			};
 		} catch (err) {
-			return { ok: false };
+			// can't fetch, let someone else deal with it.
+			return null;
 		}
 	}
 
@@ -98,19 +115,19 @@ const { fallback = 'animate' } = Astro.props as Props;
 		let wait = Promise.resolve();
 		for (const script of Array.from(document.scripts)) {
 			if (script.dataset.astroExec === '') continue;
-			const s = document.createElement('script');
-			s.innerHTML = script.innerHTML;
+			const newScript = document.createElement('script');
+			newScript.innerHTML = script.innerHTML;
 			for (const attr of script.attributes) {
 				if (attr.name === 'src') {
 					const p = new Promise((r) => {
-						s.onload = r;
+						newScript.onload = r;
 					});
 					wait = wait.then(() => p as any);
 				}
-				s.setAttribute(attr.name, attr.value);
+				newScript.setAttribute(attr.name, attr.value);
 			}
-			s.dataset.astroExec = '';
-			script.replaceWith(s);
+			newScript.dataset.astroExec = '';
+			script.replaceWith(newScript);
 		}
 		return wait;
 	}
@@ -122,16 +139,39 @@ const { fallback = 'animate' } = Astro.props as Props;
 		return style.animationIterationCount === 'infinite';
 	}
 
-	const parser = new DOMParser();
-
-	// A noop element used to prevent styles from being removed
-	if (import.meta.env.DEV) {
-		var noopEl = document.createElement('div');
-	}
+	const updateHistoryAndScrollPosition = (toLocation) => {
+		if (toLocation.href !== location.href) {
+			history.pushState(
+				{ index: ++currentHistoryIndex, scrollX: 0, scrollY: 0 },
+				'',
+				toLocation.href
+			);
+			// now we are on the new page for non-history navigations!
+			// (with history navigation page change happens before popstate is fired)
+		}
+		// freshly loaded pages start from the top
+		scrollTo({ left: 0, top: 0, behavior: 'instant' });
+
+		if (toLocation.hash) {
+			// because we are already on the target page ...
+			// ... what comes next is a intra-page navigation
+			// that won't reload the page but instead scroll to the fragment
+			location.href = toLocation.href;
+		}
+	};
 
-	async function updateDOM(newDocument: 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.
+	// 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
+	// if fallback === "animate" then simulate view transitions
+	async function updateDOM(
+		newDocument: Document,
+		toLocation: URL,
+		popState?: State,
+		fallback?: Fallback
+	) {
+		// Check for a head element that should persist and returns it,
+		// either because it has the data attribute or is a link el.
 		const persistedHeadElement = (el: HTMLElement): Element | null => {
 			const id = el.getAttribute(PERSIST_ATTR);
 			const newEl = id && newDocument.head.querySelector(`[${PERSIST_ATTR}="${id}"]`);
@@ -142,7 +182,11 @@ const { fallback = 'animate' } = Astro.props as Props;
 				const href = el.getAttribute('href');
 				return newDocument.head.querySelector(`link[rel=stylesheet][href="${href}"]`);
 			}
-			// Only run this in dev. This will get stripped from production builds and is not needed.
+			// What follows is a fix for an issue (#8472) with missing client:only styles after transition.
+			// That problem exists only in dev mode where styles are injected into the page by Vite.
+			// Returning a noop element ensures that the styles are not removed from the old document.
+			// Guarding the code below with the dev mode check
+			// allows tree shaking to remove this code in production.
 			if (import.meta.env.DEV) {
 				if (el.tagName === 'STYLE' && el.dataset.viteDevId) {
 					const devId = el.dataset.viteDevId;
@@ -158,10 +202,6 @@ const { fallback = 'animate' } = Astro.props as Props;
 		};
 
 		const swap = () => {
-			// noscript tags inside head element are not honored on swap (#7969).
-			// Remove them before swapping.
-			newDocument.querySelectorAll('head noscript').forEach((el) => el.remove());
-
 			// swap attributes of the html element
 			// - delete all attributes from the current document
 			// - insert all attributes from doc
@@ -208,6 +248,8 @@ const { fallback = 'animate' } = Astro.props as Props;
 
 			// Persist elements in the existing body
 			const oldBody = document.body;
+
+			// this will reset scroll Position
 			document.body.replaceWith(newDocument.body);
 			for (const el of oldBody.querySelectorAll(`[${PERSIST_ATTR}]`)) {
 				const id = el.getAttribute(PERSIST_ATTR);
@@ -219,33 +261,12 @@ const { fallback = 'animate' } = Astro.props as Props;
 				}
 			}
 
-			// Simulate scroll behavior of Safari and
-			// Chromium based browsers (Chrome, Edge, Opera, ...)
-			scrollTo({ left: 0, top: 0, behavior: 'instant' });
-
-			let initialScrollX = 0;
-			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) {
-					elem.scrollIntoView();
-					initialScrollX = Math.max(
-						0,
-						elem.offsetLeft + elem.offsetWidth - document.documentElement.clientWidth
-					);
-					initialScrollY = elem.offsetTop;
-				}
-			} else if (state) {
-				scrollTo(state.scrollX, state.scrollY); // usings default scrollBehavior
+			if (popState) {
+				scrollTo(popState.scrollX, popState.scrollY); // usings 'auto' scrollBehavior
+			} else {
+				updateHistoryAndScrollPosition(toLocation);
 			}
-			!state &&
-				history.pushState(
-					{ index: ++currentHistoryIndex, scrollX: initialScrollX, scrollY: initialScrollY },
-					'',
-					loc.href
-				);
+
 			triggerEvent('astro:after-swap');
 		};
 
@@ -291,32 +312,44 @@ const { fallback = 'animate' } = Astro.props as Props;
 		}
 	}
 
-	async function navigate(dir: Direction, loc: URL, state?: State) {
+	async function transition(direction: Direction, toLocation: URL, popState?: State) {
 		let finished: Promise<void>;
-		const href = loc.href;
-		const { html, ok, mediaType, redirected } = await getHTML(href);
-		// if there was a redirection, show the final URL in the browser's address bar
-		redirected && (loc = new URL(redirected));
+		const href = toLocation.href;
+		const response = await fetchHTML(href);
 		// If there is a problem fetching the new page, just do an MPA navigation to it.
-		if (!ok || !(mediaType === 'text/html' || mediaType === 'application/xhtml+xml')) {
+		if (response === null) {
 			location.href = href;
 			return;
 		}
+		// if there was a redirection, show the final URL in the browser's address bar
+		if (response.redirected) {
+			toLocation = new URL(response.redirected);
+		}
+
+		const newDocument = parser.parseFromString(response.html, response.mediaType);
+		// The next line might look like a hack,
+		// but it is actually necessary as noscript elements
+		// and their contents are returned as markup by the parser,
+		// see https://developer.mozilla.org/en-US/docs/Web/API/DOMParser/parseFromString
+		newDocument.querySelectorAll('noscript').forEach((el) => el.remove());
 
-		const newDocument = parser.parseFromString(html, mediaType);
 		if (!newDocument.querySelector('[name="astro-view-transitions-enabled"]')) {
 			location.href = href;
 			return;
 		}
 
-		// Now we are sure that we will push state, and it is time to create a state if it is still missing.
-		!state && history.replaceState({ index: currentHistoryIndex, scrollX, scrollY }, '');
-
-		document.documentElement.dataset.astroTransition = dir;
+		if (!popState) {
+			// save the current scroll position before we change the DOM and transition to the new page
+			history.replaceState({ ...history.state, scrollX, scrollY }, '');
+		}
+		document.documentElement.dataset.astroTransition = direction;
 		if (supportsViewTransitions) {
-			finished = document.startViewTransition(() => updateDOM(newDocument, loc, state)).finished;
+			// @ts-expect-error: startViewTransition exist
+			finished = document.startViewTransition(() =>
+				updateDOM(newDocument, toLocation, popState)
+			).finished;
 		} else {
-			finished = updateDOM(newDocument, loc, state, getFallback());
+			finished = updateDOM(newDocument, toLocation, popState, getFallback());
 		}
 		try {
 			await finished;
@@ -332,7 +365,9 @@ const { fallback = 'animate' } = Astro.props as Props;
 	// Prefetching
 	function maybePrefetch(pathname: string) {
 		if (document.querySelector(`link[rel=prefetch][href="${pathname}"]`)) return;
+		// @ts-expect-error: connection might exist
 		if (navigator.connection) {
+			// @ts-expect-error: connection does exist
 			let conn = navigator.connection;
 			if (conn.saveData || /(2|3)g/.test(conn.effectiveType || '')) return;
 		}
@@ -343,8 +378,6 @@ const { fallback = 'animate' } = Astro.props as Props;
 	}
 
 	if (supportsViewTransitions || getFallback() !== 'none') {
-		markScriptsExec();
-
 		document.addEventListener('click', (ev) => {
 			let link = ev.target;
 			if (link instanceof Element && link.tagName !== 'A') {
@@ -366,43 +399,49 @@ const { fallback = 'animate' } = Astro.props as Props;
 				ev.ctrlKey || // new tab (windows)
 				ev.altKey || // download
 				ev.shiftKey || // new window
-				ev.defaultPrevented ||
-				!transitionEnabledOnThisPage()
+				ev.defaultPrevented
 			) {
 				// 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) {
-				if (link.hash) {
-					// The browser default action will handle navigations with hash fragments
-					return;
+			ev.preventDefault();
+			navigate(link.href);
+		});
+
+		function navigate(href) {
+			// not ours
+			if (!transitionEnabledOnThisPage()) {
+				location.href = href;
+				return;
+			}
+			const toLocation = new URL(href, location.href);
+			// We do not have page transitions on navigations to the same page (intra-page navigation)
+			// 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 &&
+				location.pathname === toLocation.pathname &&
+				location.search === toLocation.search
+			) {
+				// mark current position as non transition intra-page scrolling
+				if (location.href !== toLocation.href) {
+					history.replaceState({ ...history.state, intraPage: true }, '');
+					history.pushState(
+						{ index: ++currentHistoryIndex, scrollX: 0, scrollY: 0 },
+						'',
+						toLocation.href
+					);
+				}
+				if (toLocation.hash) {
+					location.href = toLocation.href;
 				} else {
-					// Special case: self link without hash
-					// If handed to the browser it will reload the page
-					// But we want to handle it like any other same page navigation
-					// So we scroll to the top of the page but do not start page transitions
-					ev.preventDefault();
-					// push state on the first navigation but not if we were here already
-					if (location.hash) {
-						history.replaceState(
-							{ index: currentHistoryIndex, scrollX, scrollY: -(scrollY + 1) },
-							''
-						);
-						const newState: State = { index: ++currentHistoryIndex, scrollX: 0, scrollY: 0 };
-						history.pushState(newState, '', link.href);
-					}
 					scrollTo({ left: 0, top: 0, behavior: 'instant' });
-					return;
 				}
+			} else {
+				transition('forward', toLocation);
 			}
-
-			// these are the cases we will handle: same origin, different page
-			ev.preventDefault();
-			navigate('forward', new URL(link.href));
-		});
+		}
 
 		addEventListener('popstate', (ev) => {
 			if (!transitionEnabledOnThisPage() && ev.state) {
@@ -410,7 +449,9 @@ const { fallback = 'animate' } = Astro.props as Props;
 				// but the page we navigate to does (because it set the state).
 				// Do a full page refresh to reload the client-side router from the new page.
 				// Scroll restauration will then happen during the reload when the router's code is re-executed
-				history.scrollRestoration && (history.scrollRestoration = 'manual');
+				if (history.scrollRestoration) {
+					history.scrollRestoration = 'manual';
+				}
 				location.reload();
 				return;
 			}
@@ -433,13 +474,14 @@ const { fallback = 'animate' } = Astro.props as Props;
 			}
 
 			const state: State = history.state;
-			const nextIndex = state.index;
-			const direction: Direction = nextIndex > currentHistoryIndex ? 'forward' : 'back';
-			currentHistoryIndex = nextIndex;
-			if (state.scrollY < 0) {
-				scrollTo(state.scrollX, -(state.scrollY + 1));
+			if (state.intraPage) {
+				// this is non transition intra-page scrolling
+				scrollTo(state.scrollX, state.scrollY);
 			} else {
-				navigate(direction, new URL(location.href), state);
+				const nextIndex = state.index;
+				const direction: Direction = nextIndex > currentHistoryIndex ? 'forward' : 'back';
+				currentHistoryIndex = nextIndex;
+				transition(direction, new URL(location.href), state);
 			}
 		});
 
@@ -461,6 +503,7 @@ const { fallback = 'animate' } = Astro.props as Props;
 				{ passive: true, capture: true }
 			);
 		});
+
 		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.
@@ -470,5 +513,7 @@ const { fallback = 'animate' } = Astro.props as Props;
 
 		if ('onscrollend' in window) addEventListener('scrollend', updateState);
 		else addEventListener('scroll', throttle(updateState, 300));
+
+		markScriptsExec();
 	}
 </script>
diff --git a/packages/astro/e2e/view-transitions.test.js b/packages/astro/e2e/view-transitions.test.js
index d777d9b2f..b06d5a988 100644
--- a/packages/astro/e2e/view-transitions.test.js
+++ b/packages/astro/e2e/view-transitions.test.js
@@ -293,12 +293,12 @@ test.describe('View Transitions', () => {
 		locator = page.locator('#click-one-again');
 		await expect(locator).toBeInViewport();
 
-		// Scroll up to top fragment
+		// goto page 1
 		await page.click('#click-one-again');
 		locator = page.locator('#one');
 		await expect(locator).toHaveText('Page 1');
 
-		// Back to middle of the page
+		// Back to middle of the previous page
 		await page.goBack();
 		locator = page.locator('#click-one-again');
 		await expect(locator).toBeInViewport();