fix: respect scroll-margin when navigating to a url-supplied anchor

.changeset/clever-bars-throw.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+fix: respect scroll-margin when navigating to a url-supplied anchor

packages/kit/src/runtime/client/client.js

@@ -1761,26 +1761,18 @@ async function navigate({
 	await svelte.tick();
 
 	// we reset scroll before dealing with focus, to avoid a flash of unscrolled content
-	let scroll = popped ? popped.scroll : noscroll ? scroll_state() : null;
+	/** @type {Element | null | ''} */
+	let deep_linked = null;
 
 	if (autoscroll) {
-		const deep_linked = url.hash && document.getElementById(get_id(url));
+		const scroll = popped ? popped.scroll : noscroll ? scroll_state() : null;
 		if (scroll) {
 			scrollTo(scroll.x, scroll.y);
-		} else if (deep_linked) {
+		} else if ((deep_linked = url.hash && document.getElementById(get_id(url)))) {
 			// Here we use `scrollIntoView` on the element instead of `scrollTo`
 			// because it natively supports the `scroll-margin` and `scroll-behavior`
 			// CSS properties.
 			deep_linked.scrollIntoView();
-
-			// Get target position at this point because with smooth scrolling the scroll position
-			// retrieved from current x/y above might be wrong (since we might not have arrived at the destination yet)
-			const { top, left } = deep_linked.getBoundingClientRect();
-
-			scroll = {
-				x: pageXOffset + left,
-				y: pageYOffset + top
-			};
 		} else {
 			scrollTo(0, 0);
 		}
@@ -1794,7 +1786,10 @@ async function navigate({
 		document.activeElement !== document.body;
 
 	if (!keepfocus && !changed_focus) {
-		reset_focus(url, scroll);
+		// We don't need to manually restore the scroll position if we're navigating
+		// to a fragment identifier. It is automatically done for us when we set the
+		// sequential navigation starting point with `location.replace`
+		reset_focus(url, !deep_linked);
 	}
 
 	autoscroll = true;
@@ -2999,9 +2994,9 @@ let resetting_focus = false;
 
 /**
  * @param {URL} url
- * @param {{ x: number, y: number } | null} scroll
+ * @param {boolean} [scroll]
  */
-function reset_focus(url, scroll = null) {
+function reset_focus(url, scroll = true) {
 	const autofocus = document.querySelector('[autofocus]');
 	if (autofocus) {
 		// @ts-ignore
@@ -3013,7 +3008,7 @@ function reset_focus(url, scroll = null) {
 		// starting point to the fragment identifier.
 		const id = get_id(url);
 		if (id && document.getElementById(id)) {
-			const { x, y } = scroll ?? scroll_state();
+			const { x, y } = scroll_state();
 
 			// `element.focus()` doesn't work on Safari and Firefox Ubuntu so we need
 			// to use this hack with `location.replace()` instead.
@@ -3028,9 +3023,9 @@ function reset_focus(url, scroll = null) {
 				// This is also needed to restore the original hash if we're using hash routing
 				history.replaceState(history_state, '', url);
 
-				// Scroll management has already happened earlier so we need to restore
+				// If scroll management has already happened earlier, we need to restore
 				// the scroll position after setting the sequential focus navigation starting point
-				scrollTo(x, y);
+				if (scroll) scrollTo(x, y);
 				resetting_focus = false;
 			});
 		} else {

packages/kit/test/apps/basics/src/routes/anchor/+page.svelte

@@ -11,6 +11,7 @@
 <a id="last-anchor" href="/anchor/anchor#go-to-element">Anchor demo (last)</a>
 <a id="last-anchor-2" href="/anchor/anchor">Anchor demo (last 2)</a>
 <a id="routing-page" href="/routing/hashes/target">Different page</a>
+<a id="to-scroll-margin" href="/anchor/anchor#scroll-margin">Scroll margin anchor</a>
 
 <style>
 	:global(body) {

packages/kit/test/apps/basics/src/routes/anchor/anchor/+page.svelte

@@ -8,4 +8,7 @@
 <div style="height: 180vh; background-color: honeydew;">
 	<p id="go-to-.=" class="special-char-id">The browser scrolls to me</p>
 </div>
+<div style="height: 180vh; background-color: lightblue;">
+	<p id="scroll-margin" style="scroll-margin-top: 40px;">The browser scrolls to me</p>
+</div>
 <a id="non-ascii-anchor" href="#go-to-encöded">Anchor demo (non-ASCII)</a>

packages/kit/test/apps/basics/test/cross-platform/client.test.js

@@ -9,7 +9,7 @@
 test.describe.configure({ mode: 'parallel' });
 
 test.describe('a11y', () => {
 	test('resets focus', async ({ page, clicknav, browserName }) => {
 		const tab = browserName === 'webkit' ? 'Alt+Tab' : 'Tab';
 
 		await page.goto('/accessibility/a');
@@ -32,7 +32,7 @@
 		expect(await page.evaluate(() => document.documentElement.getAttribute('tabindex'))).toBe(null);
 	});
 
 	test('applies autofocus after a navigation', async ({ page, clicknav }) => {
 		await page.goto('/accessibility/autofocus/a');
 
 		await clicknav('[href="/accessibility/autofocus/b"]', {
@@ -95,7 +95,7 @@
 		).toBe(0);
 	});
 
 	test('keepfocus works', async ({ page }) => {
 		await page.goto('/keepfocus');
 
 		await Promise.all([
@@ -488,6 +488,16 @@
 		expect(await in_view('#go-to-element')).toBe(true);
 	});
 
+	test('scrolling to url-supplied anchor respects scroll-margin', async ({ page, clicknav }) => {
+		await page.goto('/anchor');
+		await clicknav('#to-scroll-margin');
+		expect(
+			await page.evaluate(
+				() => document.getElementById('scroll-margin')?.getBoundingClientRect().top
+			)
+		).toBe(40);
+	});
+
 	test('no-anchor url will scroll to top when navigated from bottom of page', async ({
 		clicknav,
 		page
@@ -1069,7 +1079,7 @@
 		expect(requests.filter((r) => !r.includes('__route.js'))).toEqual([]);
 	});
 
 	test('responds to <form target="_blank"> submission with new tab', async ({ page }) => {
 		await page.goto('/routing/form-target-blank');
 
 		let tabs = page.context().pages();