From c86b9f9c0a26f5f6c29878913c398fe62fb81b1f Mon Sep 17 00:00:00 2001 From: bluwy Date: Sun, 2 Jan 2022 16:43:48 +0800 Subject: [PATCH 1/3] fix: skip navigation if only url hash change --- .changeset/funny-trees-breathe.md | 5 ++ packages/kit/src/runtime/client/router.js | 16 +++--- .../apps/basics/src/routes/routing/_tests.js | 55 ++++++++++++++++++- .../src/routes/routing/hashes/target.svelte | 15 +++++ 4 files changed, 80 insertions(+), 11 deletions(-) create mode 100644 .changeset/funny-trees-breathe.md create mode 100644 packages/kit/test/apps/basics/src/routes/routing/hashes/target.svelte diff --git a/.changeset/funny-trees-breathe.md b/.changeset/funny-trees-breathe.md new file mode 100644 index 000000000000..a4d747b87388 --- /dev/null +++ b/.changeset/funny-trees-breathe.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +Fix hash change focus behaviour diff --git a/packages/kit/src/runtime/client/router.js b/packages/kit/src/runtime/client/router.js index 8c0b9e071da3..6aa5d588e253 100644 --- a/packages/kit/src/runtime/client/router.js +++ b/packages/kit/src/runtime/client/router.js @@ -157,16 +157,16 @@ export class Router { if (!this.owns(url)) return; - const noscroll = a.hasAttribute('sveltekit:noscroll'); + // Check if new url only differs by hash + if (url.href.split('#')[0] === location.href.split('#')[0]) { + // Delay pushState, otherwise the browser default behaviour would not kick in + setTimeout(() => history.pushState({}, '', url.href)); + return; + } - const i1 = url_string.indexOf('#'); - const i2 = location.href.indexOf('#'); - const u1 = i1 >= 0 ? url_string.substring(0, i1) : url_string; - const u2 = i2 >= 0 ? location.href.substring(0, i2) : location.href; history.pushState({}, '', url.href); - if (u1 === u2) { - window.dispatchEvent(new HashChangeEvent('hashchange')); - } + + const noscroll = a.hasAttribute('sveltekit:noscroll'); this._navigate(url, noscroll ? scroll_state() : null, false, [], url.hash); event.preventDefault(); }); diff --git a/packages/kit/test/apps/basics/src/routes/routing/_tests.js b/packages/kit/test/apps/basics/src/routes/routing/_tests.js index 76ba9e711ee3..270497805633 100644 --- a/packages/kit/test/apps/basics/src/routes/routing/_tests.js +++ b/packages/kit/test/apps/basics/src/routes/routing/_tests.js @@ -188,15 +188,64 @@ export default function (test, is_dev) { test( 'back button returns to previous route when previous route has been navigated to via hash anchor', '/routing/hashes/a', - async ({ page, clicknav, back }) => { - await clicknav('[href="#hash-target"]'); + async ({ page, clicknav }) => { + await page.click('[href="#hash-target"]'); await clicknav('[href="/routing/hashes/b"]'); - await back(); + await page.goBack(); assert.equal(await page.textContent('h1'), 'a'); } ); + test( + 'focus works if page load has hash', + '/routing/hashes/target#p2', + async ({ page }) => { + await page.keyboard.press('Tab'); + assert.equal( + await page.evaluate(() => (document.activeElement || {}).textContent), + 'next focus element' + ); + }, + // Does not work in nojs + { nojs: false } + ); + + test( + 'focus works when navigating to a hash on the same page', + '/routing/hashes/target', + async ({ page }) => { + await page.click('[href="#p2"]'); + await page.keyboard.press('Tab'); + assert.equal( + await page.evaluate(() => (document.activeElement || {}).textContent), + 'next focus element' + ); + } + ); + + test( + ':target pseudo-selector works when navigating to a hash on the same page', + '/routing/hashes/target#p1', + async ({ page }) => { + assert.equal( + await page.evaluate(() => { + const el = document.getElementById('p1'); + return el && getComputedStyle(el).color; + }), + 'rgb(255, 0, 0)' + ); + await page.click('[href="#p2"]'); + assert.equal( + await page.evaluate(() => { + const el = document.getElementById('p2'); + return el && getComputedStyle(el).color; + }), + 'rgb(255, 0, 0)' + ); + } + ); + test('fallthrough', '/routing/fallthrough-simple/invalid', async ({ page }) => { assert.equal(await page.textContent('h1'), 'Page'); }); diff --git a/packages/kit/test/apps/basics/src/routes/routing/hashes/target.svelte b/packages/kit/test/apps/basics/src/routes/routing/hashes/target.svelte new file mode 100644 index 000000000000..8f56c7c1920c --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/hashes/target.svelte @@ -0,0 +1,15 @@ +
    +
  1. first paragraph
    2. +
  2. second paragraph
    3. +
+ +

paragraph 1

+

paragraph 2

+ +
  • next focus element
    • + + From 79350cba75af69d135e10bb9e9fd4b13903cdde7 Mon Sep 17 00:00:00 2001 From: bluwy Date: Mon, 3 Jan 2022 14:12:35 +0800 Subject: [PATCH 2/3] docs: update pushState comment --- packages/kit/src/runtime/client/router.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/kit/src/runtime/client/router.js b/packages/kit/src/runtime/client/router.js index 6aa5d588e253..0a723b4232b7 100644 --- a/packages/kit/src/runtime/client/router.js +++ b/packages/kit/src/runtime/client/router.js @@ -159,7 +159,8 @@ export class Router { // Check if new url only differs by hash if (url.href.split('#')[0] === location.href.split('#')[0]) { - // Delay pushState, otherwise the browser default behaviour would not kick in + // Call `pushState` to add url to history so going back works. + // Also make a delay, otherwise the browser default behaviour would not kick in setTimeout(() => history.pushState({}, '', url.href)); return; } From ed91ca8993c06ced11efde348a437cbe5c1ad2b1 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 3 Jan 2022 11:44:50 +0000 Subject: [PATCH 3/3] remove _tests.js --- .../apps/basics/src/routes/routing/_tests.js | 305 ------------------ 1 file changed, 305 deletions(-) delete mode 100644 packages/kit/test/apps/basics/src/routes/routing/_tests.js diff --git a/packages/kit/test/apps/basics/src/routes/routing/_tests.js b/packages/kit/test/apps/basics/src/routes/routing/_tests.js deleted file mode 100644 index 270497805633..000000000000 --- a/packages/kit/test/apps/basics/src/routes/routing/_tests.js +++ /dev/null @@ -1,305 +0,0 @@ -import * as assert from 'uvu/assert'; -import fs from 'fs'; -import path from 'path'; -import { fileURLToPath } from 'url'; - -/** @type {import('test').TestMaker} */ -export default function (test, is_dev) { - test( - 'redirects from /routing/ to /routing', - '/routing/slashes', - async ({ base, page, clicknav, app, js }) => { - await clicknav('a[href="/routing/"]'); - assert.equal(page.url(), `${base}/routing`); - assert.equal(await page.textContent('h1'), 'Great success!'); - - if (js) { - await page.goto(`${base}/routing/slashes`); - await page.evaluate(() => window.started); - await app.goto('/routing/'); - assert.equal(page.url(), `${base}/routing`); - assert.equal(await page.textContent('h1'), 'Great success!'); - } - } - ); - - test( - 'redirects from /routing/? to /routing', - '/routing/slashes', - async ({ base, page, clicknav, app, js }) => { - await clicknav('a[href="/routing/?"]'); - assert.equal(page.url(), `${base}/routing`); - assert.equal(await page.textContent('h1'), 'Great success!'); - - if (js) { - await page.goto(`${base}/routing/slashes`); - await page.evaluate(() => window.started); - await app.goto('/routing/?'); - assert.equal(page.url(), `${base}/routing`); - assert.equal(await page.textContent('h1'), 'Great success!'); - } - } - ); - - test( - 'redirects from /routing/?foo=bar to /routing?foo=bar', - '/routing/slashes', - async ({ base, page, clicknav, app, js }) => { - await clicknav('a[href="/routing/?foo=bar"]'); - assert.equal(page.url(), `${base}/routing?foo=bar`); - assert.equal(await page.textContent('h1'), 'Great success!'); - - if (js) { - await page.goto(`${base}/routing/slashes`); - await page.evaluate(() => window.started); - await app.goto('/routing/?foo=bar'); - assert.equal(page.url(), `${base}/routing?foo=bar`); - assert.equal(await page.textContent('h1'), 'Great success!'); - } - } - ); - - test('serves static route', '/routing/a', async ({ page }) => { - assert.equal(await page.textContent('h1'), 'a'); - }); - - test('serves static route from dir/index.html file', '/routing/b', async ({ page }) => { - assert.equal(await page.textContent('h1'), 'b'); - }); - - test( - 'serves static route under client directory', - '/routing/client/foo', - async ({ base, page }) => { - assert.equal(await page.textContent('h1'), 'foo'); - - await page.goto(`${base}/routing/client/bar`); - assert.equal(await page.textContent('h1'), 'bar'); - - await page.goto(`${base}/routing/client/bar/b`); - assert.equal(await page.textContent('h1'), 'b'); - } - ); - - test('serves dynamic route', '/routing/test-slug', async ({ page }) => { - assert.equal(await page.textContent('h1'), 'test-slug'); - }); - - test( - 'navigates to a new page without reloading', - '/routing', - async ({ app, capture_requests, page, clicknav, js }) => { - if (js) { - await app.prefetchRoutes(['/routing/a']).catch((e) => { - // from error handler tests; ignore - if (!e.message.includes('Crashing now')) throw e; - }); - - const requests = await capture_requests(async () => { - await clicknav('a[href="/routing/a"]'); - assert.equal(await page.textContent('h1'), 'a'); - }); - - assert.equal(requests, []); - } - } - ); - - test('navigates programmatically', '/routing/a', async ({ page, app, js }) => { - if (js) { - await app.goto('/routing/b'); - assert.equal(await page.textContent('h1'), 'b'); - } - }); - - test('prefetches programmatically', '/routing/a', async ({ base, capture_requests, app, js }) => { - if (js) { - const requests1 = await capture_requests(() => app.prefetch('/routing/prefetched')); - assert.equal(requests1.length, 2, requests1.join(',')); - assert.equal(requests1[1], `${base}/routing/prefetched.json`); - - const requests2 = await capture_requests(() => app.goto('/routing/prefetched')); - assert.equal(requests2, []); - - try { - await app.prefetch('https://example.com'); - throw new Error('Error was not thrown'); - } catch (/** @type {any} */ e) { - assert.ok( - e.message.includes('Attempted to prefetch a URL that does not belong to this app') - ); - } - } - }); - - test('does not attempt client-side navigation to server routes', '/routing', async ({ page }) => { - await page.click('[href="/routing/ambiguous/ok.json"]'); - assert.equal(await page.textContent('body'), 'ok'); - }); - - test('allows reserved words as route names', '/routing/const', async ({ page }) => { - assert.equal(await page.textContent('h1'), 'reserved words are okay as routes'); - }); - - test('resets the active element after navigation', '/routing', async ({ page, clicknav }) => { - await clicknav('[href="/routing/a"]'); - await page.waitForFunction(() => (document.activeElement || {}).nodeName == 'BODY'); - }); - - test( - 'navigates between routes with empty parts', - '/routing/dirs/foo', - async ({ page, clicknav }) => { - assert.equal(await page.textContent('h1'), 'foo'); - await clicknav('[href="bar"]'); - assert.equal(await page.textContent('h1'), 'bar'); - } - ); - - test( - 'navigates between dynamic routes with same segments', - '/routing/dirs/bar/xyz', - async ({ page, clicknav }) => { - assert.equal(await page.textContent('h1'), 'A page'); - - await clicknav('[href="/routing/dirs/foo/xyz"]'); - assert.equal(await page.textContent('h1'), 'B page'); - } - ); - - test( - 'invalidates page when a segment is skipped', - '/routing/skipped/x/1', - async ({ page, clicknav }) => { - assert.equal(await page.textContent('h1'), 'x/1'); - - await clicknav('#goto-y1'); - assert.equal(await page.textContent('h1'), 'y/1'); - } - ); - - test('back button returns to initial route', '/routing', async ({ page, clicknav, back }) => { - await clicknav('[href="/routing/a"]'); - - await back(); - assert.equal(await page.textContent('h1'), 'Great success!'); - }); - - test( - 'back button returns to previous route when previous route has been navigated to via hash anchor', - '/routing/hashes/a', - async ({ page, clicknav }) => { - await page.click('[href="#hash-target"]'); - await clicknav('[href="/routing/hashes/b"]'); - - await page.goBack(); - assert.equal(await page.textContent('h1'), 'a'); - } - ); - - test( - 'focus works if page load has hash', - '/routing/hashes/target#p2', - async ({ page }) => { - await page.keyboard.press('Tab'); - assert.equal( - await page.evaluate(() => (document.activeElement || {}).textContent), - 'next focus element' - ); - }, - // Does not work in nojs - { nojs: false } - ); - - test( - 'focus works when navigating to a hash on the same page', - '/routing/hashes/target', - async ({ page }) => { - await page.click('[href="#p2"]'); - await page.keyboard.press('Tab'); - assert.equal( - await page.evaluate(() => (document.activeElement || {}).textContent), - 'next focus element' - ); - } - ); - - test( - ':target pseudo-selector works when navigating to a hash on the same page', - '/routing/hashes/target#p1', - async ({ page }) => { - assert.equal( - await page.evaluate(() => { - const el = document.getElementById('p1'); - return el && getComputedStyle(el).color; - }), - 'rgb(255, 0, 0)' - ); - await page.click('[href="#p2"]'); - assert.equal( - await page.evaluate(() => { - const el = document.getElementById('p2'); - return el && getComputedStyle(el).color; - }), - 'rgb(255, 0, 0)' - ); - } - ); - - test('fallthrough', '/routing/fallthrough-simple/invalid', async ({ page }) => { - assert.equal(await page.textContent('h1'), 'Page'); - }); - - test( - 'dynamic fallthrough of pages and endpoints', - '/routing/fallthrough-advanced/borax', - async ({ page, clicknav }) => { - assert.equal(await page.textContent('h1'), 'borax is a mineral'); - - await clicknav('[href="/routing/fallthrough-advanced/camel"]'); - assert.equal(await page.textContent('h1'), 'camel is an animal'); - - await clicknav('[href="/routing/fallthrough-advanced/potato"]'); - assert.equal(await page.textContent('h1'), '404'); - } - ); - - test( - 'last parameter in a segment wins in cases of ambiguity', - '/routing/split-params', - async ({ page, clicknav }) => { - await clicknav('[href="/routing/split-params/x-y-z"]'); - assert.equal(await page.textContent('h1'), 'x'); - assert.equal(await page.textContent('h2'), 'y-z'); - } - ); - - test('ignores navigation to URLs the app does not own', '/routing', async ({ page }) => { - await page.click('[href="https://www.google.com"]'); - assert.equal(page.url(), 'https://www.google.com/'); - }); - - // skipping this test because it causes a bunch of failures locally - test.skip('watch new route in dev', '/routing', async ({ page, base, js, watcher }) => { - if (!is_dev || js) { - return; - } - - // hash the filename so that it won't conflict with - // future test file that has the same name - const route = 'bar' + new Date().valueOf(); - const content = 'Hello new route'; - const __dirname = path.dirname(fileURLToPath(import.meta.url)); - const filePath = path.join(__dirname, `${route}.svelte`); - - try { - fs.writeFileSync(filePath, `

    ${content}

    `); - watcher.update(); - await page.goto(`${base}/routing/${route}`); - - assert.equal(await page.textContent('h1'), content); - } finally { - fs.unlinkSync(filePath); - } - }); -}