From 5e7cc9b677d1e0ee075097eaa5df80312af576bf Mon Sep 17 00:00:00 2001 From: Chew Tee Ming Date: Wed, 23 Oct 2024 15:06:40 +0800 Subject: [PATCH 01/10] add tests --- .../data-sveltekit/reload/hash/+page.svelte | 3 +++ .../reload/hash/new-page/+page.svelte | 0 .../routes/routing/hashes/focus/+page.svelte | 2 ++ .../basics/test/cross-platform/client.test.js | 17 +++++++++++++++++ 4 files changed, 22 insertions(+) create mode 100644 packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new-page/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/routing/hashes/focus/+page.svelte diff --git a/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/+page.svelte b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/+page.svelte new file mode 100644 index 000000000000..9bf39aab984d --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/+page.svelte @@ -0,0 +1,3 @@ +focus + +new page diff --git a/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new-page/+page.svelte b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new-page/+page.svelte new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/packages/kit/test/apps/basics/src/routes/routing/hashes/focus/+page.svelte b/packages/kit/test/apps/basics/src/routes/routing/hashes/focus/+page.svelte new file mode 100644 index 000000000000..17954338b764 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/hashes/focus/+page.svelte @@ -0,0 +1,2 @@ +focus + diff --git a/packages/kit/test/apps/basics/test/cross-platform/client.test.js b/packages/kit/test/apps/basics/test/cross-platform/client.test.js index c2e9bf6cc474..3079b195db17 100644 --- a/packages/kit/test/apps/basics/test/cross-platform/client.test.js +++ b/packages/kit/test/apps/basics/test/cross-platform/client.test.js @@ -715,6 +715,23 @@ test.describe('Routing', () => { expect(await page.textContent('#page-url-hash')).toBe('#target'); }); + test('clicking on a hash link focuses the associated element', async ({ page }) => { + await page.goto('/routing/hashes/focus'); + await page.locator('a[href="#example"]').click(); + await expect(page.getByRole('textbox')).toBeFocused(); + // check it still works when the hash is already present in the URL + await page.locator('a[href="#example"]').click(); + await expect(page.getByRole('textbox')).toBeFocused(); + }); + + test('backwards navigation works after clicking a hash link with data-sveltekit-reload', async ({ page, baseURL }) => { + await page.goto('/data-sveltekit/reload/hash'); + await page.locator('a[href="#example"]').click(); + await page.locator('a[href="/data-sveltekit/reload/hash/new-page"]').click(); + await page.goBack(); + expect(page.url()).toBe(`${baseURL}/data-sveltekit/reload/hash#example`); + }); + test('back button returns to previous route when previous route has been navigated to via hash anchor', async ({ page, clicknav From 3b6a2ec0f50adfe6564fd9c3e221a0c34a8ab2a4 Mon Sep 17 00:00:00 2001 From: Chew Tee Ming Date: Wed, 23 Oct 2024 15:20:34 +0800 Subject: [PATCH 02/10] fix test --- .../routes/data-sveltekit/reload/hash/+page.svelte | 2 +- .../data-sveltekit/reload/hash/new-page/+page.svelte | 0 .../data-sveltekit/reload/hash/new/+page.svelte | 1 + .../apps/basics/test/cross-platform/client.test.js | 11 +++++++++-- 4 files changed, 11 insertions(+), 3 deletions(-) delete mode 100644 packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new-page/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new/+page.svelte diff --git a/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/+page.svelte b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/+page.svelte index 9bf39aab984d..ca54461e368f 100644 --- a/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/+page.svelte +++ b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/+page.svelte @@ -1,3 +1,3 @@ focus -new page +new page diff --git a/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new-page/+page.svelte b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new-page/+page.svelte deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new/+page.svelte b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new/+page.svelte new file mode 100644 index 000000000000..6d694da1b6db --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new/+page.svelte @@ -0,0 +1 @@ +

hello world

\ No newline at end of file diff --git a/packages/kit/test/apps/basics/test/cross-platform/client.test.js b/packages/kit/test/apps/basics/test/cross-platform/client.test.js index 3079b195db17..71338a8422ce 100644 --- a/packages/kit/test/apps/basics/test/cross-platform/client.test.js +++ b/packages/kit/test/apps/basics/test/cross-platform/client.test.js @@ -724,12 +724,19 @@ test.describe('Routing', () => { await expect(page.getByRole('textbox')).toBeFocused(); }); - test('backwards navigation works after clicking a hash link with data-sveltekit-reload', async ({ page, baseURL }) => { + test('backwards navigation works after clicking a hash link with data-sveltekit-reload', async ({ + page, + clicknav, + baseURL + }) => { await page.goto('/data-sveltekit/reload/hash'); await page.locator('a[href="#example"]').click(); - await page.locator('a[href="/data-sveltekit/reload/hash/new-page"]').click(); + expect(page.url()).toBe(`${baseURL}/data-sveltekit/reload/hash#example`); + await clicknav('a[href="/data-sveltekit/reload/hash/new"]'); + expect(page.url()).toBe(`${baseURL}/data-sveltekit/reload/hash/new`); await page.goBack(); expect(page.url()).toBe(`${baseURL}/data-sveltekit/reload/hash#example`); + await expect(page.getByRole('textbox')).toBeVisible(); }); test('back button returns to previous route when previous route has been navigated to via hash anchor', async ({ From d57f0704ae589a087290069c78c0571327a040bd Mon Sep 17 00:00:00 2001 From: Chew Tee Ming Date: Wed, 23 Oct 2024 15:21:35 +0800 Subject: [PATCH 03/10] add fix --- packages/kit/src/runtime/client/client.js | 32 +++++++++++------------ 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 432c7165fb02..770c6f90f766 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -2089,8 +2089,11 @@ function _start_router() { if (download) return; + const [nonhash, hash] = url.href.split('#'); + const same_pathname = nonhash === strip_hash(location); + // Ignore the following but fire beforeNavigate - if (external || options.reload) { + if (external || (options.reload && !same_pathname && !hash)) { if (_before_navigate({ url, type: 'link' })) { // set `navigating` to `true` to prevent `beforeNavigate` callbacks // being called when the page unloads @@ -2105,25 +2108,20 @@ function _start_router() { // Check if new url only differs by hash and use the browser default behavior in that case // This will ensure the `hashchange` event is fired // Removing the hash does a full page navigation in the browser, so make sure a hash is present - const [nonhash, hash] = url.href.split('#'); - if (hash !== undefined && nonhash === strip_hash(location)) { - // If we are trying to navigate to the same hash, we should only - // attempt to scroll to that element and avoid any history changes. - // Otherwise, this can cause Firefox to incorrectly assign a null - // history state value without any signal that we can detect. + if (hash !== undefined && same_pathname) { const [, current_hash] = current.url.href.split('#'); if (current_hash === hash) { event.preventDefault(); - - // We're already on /# and click on a link that goes to /#, or we're on - // /#top and click on a link that goes to /#top. In those cases just go to - // the top of the page, and avoid a history change. - if (hash === '' || (hash === 'top' && a.ownerDocument.getElementById('top') === null)) { - window.scrollTo({ top: 0 }); - } else { - a.ownerDocument.getElementById(decodeURIComponent(hash))?.scrollIntoView(); - } - + setTimeout(() => { + const history_state = history.state; + // Mimic the browsers' behaviour and set the sequential focus navigation + // starting point to the fragment identifier + location.replace(location.hash); + // but Firefox has a bug that sets the history state as null so we + // need to restore the history state + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1199924 + history.replaceState(history_state, '', location.hash); + }); return; } // set this flag to distinguish between navigations triggered by From 4e8708db6646bccea0c97b595f52a575babe959f Mon Sep 17 00:00:00 2001 From: Chew Tee Ming Date: Wed, 23 Oct 2024 15:37:01 +0800 Subject: [PATCH 04/10] prettier --- .../src/routes/data-sveltekit/reload/hash/new/+page.svelte | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new/+page.svelte b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new/+page.svelte index 6d694da1b6db..48aa4cb69f99 100644 --- a/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new/+page.svelte +++ b/packages/kit/test/apps/basics/src/routes/data-sveltekit/reload/hash/new/+page.svelte @@ -1 +1 @@ -

hello world

\ No newline at end of file +

hello world

From 822f7584529f670051dd0f8faf8e2474fcd2cdf5 Mon Sep 17 00:00:00 2001 From: Chew Tee Ming Date: Wed, 23 Oct 2024 15:44:05 +0800 Subject: [PATCH 05/10] revert --- packages/kit/src/runtime/client/client.js | 24 +++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 770c6f90f766..6b68e741a683 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -2109,19 +2109,23 @@ function _start_router() { // This will ensure the `hashchange` event is fired // Removing the hash does a full page navigation in the browser, so make sure a hash is present if (hash !== undefined && same_pathname) { + // If we are trying to navigate to the same hash, we should only + // attempt to scroll to that element and avoid any history changes. + // Otherwise, this can cause Firefox to incorrectly assign a null + // history state value without any signal that we can detect. const [, current_hash] = current.url.href.split('#'); if (current_hash === hash) { event.preventDefault(); - setTimeout(() => { - const history_state = history.state; - // Mimic the browsers' behaviour and set the sequential focus navigation - // starting point to the fragment identifier - location.replace(location.hash); - // but Firefox has a bug that sets the history state as null so we - // need to restore the history state - // See https://bugzilla.mozilla.org/show_bug.cgi?id=1199924 - history.replaceState(history_state, '', location.hash); - }); + + // We're already on /# and click on a link that goes to /#, or we're on + // /#top and click on a link that goes to /#top. In those cases just go to + // the top of the page, and avoid a history change. + if (hash === '' || (hash === 'top' && a.ownerDocument.getElementById('top') === null)) { + window.scrollTo({ top: 0 }); + } else { + a.ownerDocument.getElementById(decodeURIComponent(hash))?.scrollIntoView(); + } + return; } // set this flag to distinguish between navigations triggered by From 9855e3ec023b672ec99c030d1ae4d740bbc6476b Mon Sep 17 00:00:00 2001 From: Chew Tee Ming Date: Wed, 23 Oct 2024 17:15:52 +0800 Subject: [PATCH 06/10] fix --- packages/kit/src/runtime/client/client.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 6b68e741a683..496c6b971bcc 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -2123,7 +2123,9 @@ function _start_router() { if (hash === '' || (hash === 'top' && a.ownerDocument.getElementById('top') === null)) { window.scrollTo({ top: 0 }); } else { - a.ownerDocument.getElementById(decodeURIComponent(hash))?.scrollIntoView(); + const element = a.ownerDocument.getElementById(decodeURIComponent(hash)); + element?.scrollIntoView(); + element?.focus(); } return; From dfa295d0659b7f314c64377f57f496dda69d5cad Mon Sep 17 00:00:00 2001 From: Chew Tee Ming Date: Wed, 23 Oct 2024 17:19:27 +0800 Subject: [PATCH 07/10] maybe this is better --- packages/kit/src/runtime/client/client.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 496c6b971bcc..3cea42ca5f3c 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -2124,8 +2124,10 @@ function _start_router() { window.scrollTo({ top: 0 }); } else { const element = a.ownerDocument.getElementById(decodeURIComponent(hash)); - element?.scrollIntoView(); - element?.focus(); + if (element) { + element.scrollIntoView(); + element.focus(); + } } return; From 579cf1c71f21b3ead88393184ae02a1a899037fe Mon Sep 17 00:00:00 2001 From: Chew Tee Ming Date: Wed, 23 Oct 2024 17:22:32 +0800 Subject: [PATCH 08/10] changesets --- .changeset/chilled-cats-hang.md | 5 +++++ .changeset/strange-buckets-sell.md | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 .changeset/chilled-cats-hang.md create mode 100644 .changeset/strange-buckets-sell.md diff --git a/.changeset/chilled-cats-hang.md b/.changeset/chilled-cats-hang.md new file mode 100644 index 000000000000..9563dec4aaf1 --- /dev/null +++ b/.changeset/chilled-cats-hang.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: ensure element is focused after subsequent clicks of hash link diff --git a/.changeset/strange-buckets-sell.md b/.changeset/strange-buckets-sell.md new file mode 100644 index 000000000000..3cbaa6a7184c --- /dev/null +++ b/.changeset/strange-buckets-sell.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: avoid reloading behaviour for hash links with data-sveltekit-reload if the hash is on the same page From fb5a4fb2d4b684d559d059ea2439a8fcc57090ee Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Wed, 23 Oct 2024 17:27:22 +0800 Subject: [PATCH 09/10] Update .changeset/chilled-cats-hang.md --- .changeset/chilled-cats-hang.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/chilled-cats-hang.md b/.changeset/chilled-cats-hang.md index 9563dec4aaf1..47dae6ac5f68 100644 --- a/.changeset/chilled-cats-hang.md +++ b/.changeset/chilled-cats-hang.md @@ -2,4 +2,4 @@ '@sveltejs/kit': patch --- -fix: ensure element is focused after subsequent clicks of hash link +fix: ensure element is focused after subsequent clicks of the same hash link From fd0a3aa9c3975b02770392d6f0382c25d54b49cb Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Fri, 25 Oct 2024 21:52:43 +0800 Subject: [PATCH 10/10] Update packages/kit/src/runtime/client/client.js Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> --- packages/kit/src/runtime/client/client.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 3cea42ca5f3c..1a6695c385e9 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -2093,7 +2093,7 @@ function _start_router() { const same_pathname = nonhash === strip_hash(location); // Ignore the following but fire beforeNavigate - if (external || (options.reload && !same_pathname && !hash)) { + if (external || (options.reload && (!same_pathname || !hash))) { if (_before_navigate({ url, type: 'link' })) { // set `navigating` to `true` to prevent `beforeNavigate` callbacks // being called when the page unloads