From c9b4dd4cb8a2cb1b283121349c775b2e352e8c9f Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 10 Oct 2023 21:40:19 +0800 Subject: [PATCH 1/9] use window.location to scroll to deep linked element --- packages/kit/src/runtime/client/client.js | 8 ++++---- .../apps/basics/src/routes/routing/focus/+page.svelte | 1 + .../apps/basics/src/routes/routing/focus/a/+page.svelte | 4 ++++ .../test/apps/basics/test/cross-platform/client.test.js | 9 +++++++++ 4 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 packages/kit/test/apps/basics/src/routes/routing/focus/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/routing/focus/a/+page.svelte diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index f788b8b917e6..4c2378054b00 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -1126,10 +1126,10 @@ export function create_client(app, target) { if (scroll) { scrollTo(scroll.x, scroll.y); } else if (deep_linked) { - // 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(); + // `location.replace` emulates the browser native behaviour when a hash + // link is clicked by scrolling to and focusing the correct element + // even when the element cannot be manually focused. + location.replace(url.hash); } else { scrollTo(0, 0); } diff --git a/packages/kit/test/apps/basics/src/routes/routing/focus/+page.svelte b/packages/kit/test/apps/basics/src/routes/routing/focus/+page.svelte new file mode 100644 index 000000000000..d3dc9d21667b --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/focus/+page.svelte @@ -0,0 +1 @@ +click me! \ No newline at end of file diff --git a/packages/kit/test/apps/basics/src/routes/routing/focus/a/+page.svelte b/packages/kit/test/apps/basics/src/routes/routing/focus/a/+page.svelte new file mode 100644 index 000000000000..941c934c6c89 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/routing/focus/a/+page.svelte @@ -0,0 +1,4 @@ + + +

cannot be focused

+ \ 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 598c6e4c356e..6fe7242c94f4 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 @@ -646,6 +646,15 @@ test.describe('Routing', () => { expect(await page.textContent('#page-url-hash')).toBe('#target'); }); + test('clicking on hash link to a new page focuses the correct element', async ({ page, browserName }) => { + const tab = browserName === 'webkit' ? 'Alt+Tab' : 'Tab'; + await page.goto('/routing/focus'); + await page.locator('[href="/routing/focus/a#p"]').click(); + expect(await page.evaluate(() => (document.activeElement || {}).nodeName)).toBe('BODY'); + await page.keyboard.press(tab); + await expect(page.locator('#button3')).toBeFocused(); + }); + test('back button returns to previous route when previous route has been navigated to via hash anchor', async ({ page, clicknav From f9118373f30d94c9cdfdab47748f6a5db9aaadae Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 10 Oct 2023 21:41:15 +0800 Subject: [PATCH 2/9] changeset --- .changeset/big-pants-peel.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/big-pants-peel.md diff --git a/.changeset/big-pants-peel.md b/.changeset/big-pants-peel.md new file mode 100644 index 000000000000..077f8137c2e0 --- /dev/null +++ b/.changeset/big-pants-peel.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: hash links to new pages focuses the correct element From 9bc0c8f8e2169d0eafe6602db63dbc64f60470a8 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Tue, 10 Oct 2023 21:41:51 +0800 Subject: [PATCH 3/9] prettier --- .../kit/test/apps/basics/test/cross-platform/client.test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 6fe7242c94f4..d9c1c367b960 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 @@ -646,7 +646,10 @@ test.describe('Routing', () => { expect(await page.textContent('#page-url-hash')).toBe('#target'); }); - test('clicking on hash link to a new page focuses the correct element', async ({ page, browserName }) => { + test('clicking on hash link to a new page focuses the correct element', async ({ + page, + browserName + }) => { const tab = browserName === 'webkit' ? 'Alt+Tab' : 'Tab'; await page.goto('/routing/focus'); await page.locator('[href="/routing/focus/a#p"]').click(); From ff43d6abc86676d2b51dcc638cb850d84a87e0d1 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Wed, 11 Oct 2023 23:56:52 +0800 Subject: [PATCH 4/9] fix --- packages/kit/src/runtime/client/client.js | 46 +++++++++++-------- .../basics/test/cross-platform/client.test.js | 2 +- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 4c2378054b00..0533fec3b5cf 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -1126,10 +1126,10 @@ export function create_client(app, target) { if (scroll) { scrollTo(scroll.x, scroll.y); } else if (deep_linked) { - // `location.replace` emulates the browser native behaviour when a hash - // link is clicked by scrolling to and focusing the correct element - // even when the element cannot be manually focused. - location.replace(url.hash); + // 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(); } else { scrollTo(0, 0); } @@ -1146,6 +1146,8 @@ export function create_client(app, target) { reset_focus(); } + console.log(document.activeElement); + autoscroll = true; if (navigation_result.props.page) { @@ -1962,22 +1964,28 @@ function reset_focus() { autofocus.focus(); } else { // Reset page selection and focus - // We try to mimic browsers' behaviour as closely as possible by targeting the - // first scrollable region, but unfortunately it's not a perfect match — e.g. - // shift-tabbing won't immediately cycle up from the end of the page on Chromium - // See https://html.spec.whatwg.org/multipage/interaction.html#get-the-focusable-area - const root = document.body; - const tabindex = root.getAttribute('tabindex'); - - root.tabIndex = -1; - // @ts-expect-error - root.focus({ preventScroll: true, focusVisible: false }); - - // restore `tabindex` as to prevent `root` from stealing input from elements - if (tabindex !== null) { - root.setAttribute('tabindex', tabindex); + // Mimic browsers' behaviour and set the sequential focus navigation starting point + // to the fragment identifier + if (location.hash) { + location.replace(location.hash); } else { - root.removeAttribute('tabindex'); + // We try to mimic browsers' behaviour as closely as possible by targeting the + // first scrollable region, but unfortunately it's not a perfect match — e.g. + // shift-tabbing won't immediately cycle up from the end of the page on Chromium + // See https://html.spec.whatwg.org/multipage/interaction.html#get-the-focusable-area + const root = document.body; + const tabindex = root.getAttribute('tabindex'); + + root.tabIndex = -1; + // @ts-expect-error + root.focus({ preventScroll: true, focusVisible: false }); + + // restore `tabindex` as to prevent `root` from stealing input from elements + if (tabindex !== null) { + root.setAttribute('tabindex', tabindex); + } else { + root.removeAttribute('tabindex'); + } } // capture current selection, so we can compare the state after 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 d9c1c367b960..8d8e6f0e9012 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 @@ -646,7 +646,7 @@ test.describe('Routing', () => { expect(await page.textContent('#page-url-hash')).toBe('#target'); }); - test('clicking on hash link to a new page focuses the correct element', async ({ + test('sequential focus navigation starting point is set correctly on navigation', async ({ page, browserName }) => { From b4c724d35cdcdb2fc5844f4ff5883bc3525e2eab Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Thu, 12 Oct 2023 20:39:41 +0800 Subject: [PATCH 5/9] fix scroll position being changed --- .changeset/big-pants-peel.md | 2 +- packages/kit/src/runtime/client/client.js | 15 +++++++++------ .../basics/test/cross-platform/client.test.js | 1 + 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.changeset/big-pants-peel.md b/.changeset/big-pants-peel.md index 077f8137c2e0..53d0cc35da67 100644 --- a/.changeset/big-pants-peel.md +++ b/.changeset/big-pants-peel.md @@ -2,4 +2,4 @@ '@sveltejs/kit': patch --- -fix: hash links to new pages focuses the correct element +fix: correctly set sequential focus navigation starting point after navigation diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 0533fec3b5cf..1982ba7bf1f8 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -1146,8 +1146,6 @@ export function create_client(app, target) { reset_focus(); } - console.log(document.activeElement); - autoscroll = true; if (navigation_result.props.page) { @@ -1964,10 +1962,14 @@ function reset_focus() { autofocus.focus(); } else { // Reset page selection and focus - // Mimic browsers' behaviour and set the sequential focus navigation starting point - // to the fragment identifier - if (location.hash) { + if (location.hash && document.querySelector(location.hash)) { + // scroll management has already happened earlier so we need to make sure + // the scroll position stays the same after setting the sequential focus navigation starting point + const { x, y } = scroll_state(); + // mimic browsers' behaviour and set the sequential focus navigation starting point + // to the fragment identifier location.replace(location.hash); + scrollTo(x, y); } else { // We try to mimic browsers' behaviour as closely as possible by targeting the // first scrollable region, but unfortunately it's not a perfect match — e.g. @@ -1977,7 +1979,8 @@ function reset_focus() { const tabindex = root.getAttribute('tabindex'); root.tabIndex = -1; - // @ts-expect-error + // @ts-expect-error options.focusVisible is only supported in Firefox + // See https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus#browser_compatibility root.focus({ preventScroll: true, focusVisible: false }); // restore `tabindex` as to prevent `root` from stealing input from elements 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 8d8e6f0e9012..2d44e0b34d09 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 @@ -653,6 +653,7 @@ test.describe('Routing', () => { const tab = browserName === 'webkit' ? 'Alt+Tab' : 'Tab'; await page.goto('/routing/focus'); await page.locator('[href="/routing/focus/a#p"]').click(); + await page.waitForURL('**/routing/focus/a#p'); expect(await page.evaluate(() => (document.activeElement || {}).nodeName)).toBe('BODY'); await page.keyboard.press(tab); await expect(page.locator('#button3')).toBeFocused(); From 777db8508cd7377dea75848e0ef4a9b03eb8076e Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Fri, 17 May 2024 00:57:06 +0800 Subject: [PATCH 6/9] prettier --- .../kit/test/apps/basics/src/routes/routing/focus/+page.svelte | 2 +- .../test/apps/basics/src/routes/routing/focus/a/+page.svelte | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/kit/test/apps/basics/src/routes/routing/focus/+page.svelte b/packages/kit/test/apps/basics/src/routes/routing/focus/+page.svelte index d3dc9d21667b..c5007d09e2b7 100644 --- a/packages/kit/test/apps/basics/src/routes/routing/focus/+page.svelte +++ b/packages/kit/test/apps/basics/src/routes/routing/focus/+page.svelte @@ -1 +1 @@ -click me! \ No newline at end of file +click me! diff --git a/packages/kit/test/apps/basics/src/routes/routing/focus/a/+page.svelte b/packages/kit/test/apps/basics/src/routes/routing/focus/a/+page.svelte index 941c934c6c89..573e7efe8e84 100644 --- a/packages/kit/test/apps/basics/src/routes/routing/focus/a/+page.svelte +++ b/packages/kit/test/apps/basics/src/routes/routing/focus/a/+page.svelte @@ -1,4 +1,4 @@

cannot be focused

- \ No newline at end of file + From a85fa568d782697e48851612ac51ac531b1cfac6 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Thu, 23 May 2024 01:50:52 +0800 Subject: [PATCH 7/9] found a fix that works! --- packages/kit/src/runtime/client/client.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 92b5fe3e805f..33bc3ca3cb96 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -2522,9 +2522,18 @@ function reset_focus() { // scroll management has already happened earlier so we need to make sure // the scroll position stays the same after setting the sequential focus navigation starting point const { x, y } = scroll_state(); - // mimic browsers' behaviour and set the sequential focus navigation starting point - // to the fragment identifier - location.replace(location.hash); + + 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); + }); + scrollTo(x, y); } else { // We try to mimic browsers' behaviour as closely as possible by targeting the From f27dd7f44ba7e714a0f316908ff49933139eaa29 Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Thu, 23 May 2024 01:51:00 +0800 Subject: [PATCH 8/9] improve some old related tests --- .../apps/basics/test/cross-platform/client.test.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) 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 c717809da8e2..3f35a7f32217 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 @@ -697,14 +697,18 @@ test.describe('Routing', () => { await expect(page.locator('#button3')).toBeFocused(); }); - test('back button returns to previous route when previous route has been navigated to via hash anchor', async ({ + test('back button returns to previous route when previous route was navigated to via hash anchor', async ({ page, - clicknav + clicknav, + baseURL }) => { await page.goto('/routing/hashes/a'); await page.locator('[href="#hash-target"]').click(); + expect(page.url()).toBe(`${baseURL}/routing/hashes/a#hash-target`); + await clicknav('[href="/routing/hashes/b"]'); + expect(await page.textContent('h1')).toBe('b'); await page.goBack(); expect(await page.textContent('h1')).toBe('a'); @@ -718,10 +722,13 @@ test.describe('Routing', () => { await page.goto('/routing/hashes/a'); await clicknav('[href="#hash-target"]'); + expect(page.url()).toBe(`${baseURL}/routing/hashes/a#hash-target`); + await clicknav('[href="#replace-state"]'); + expect(page.url()).toBe(`${baseURL}/routing/hashes/a#replace-state`); await page.goBack(); - expect(await page.url()).toBe(`${baseURL}/routing/hashes/a`); + expect(page.url()).toBe(`${baseURL}/routing/hashes/a`); }); test('does not normalize external path', async ({ page, start_server }) => { From b4cf8227a34a6d270b856be97527d8e7673dca2b Mon Sep 17 00:00:00 2001 From: Tee Ming Date: Thu, 23 May 2024 02:08:08 +0800 Subject: [PATCH 9/9] forgot to restore scroll pos --- packages/kit/src/runtime/client/client.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index eb63291a4c24..223f421768a3 100644 --- a/packages/kit/src/runtime/client/client.js +++ b/packages/kit/src/runtime/client/client.js @@ -2529,22 +2529,22 @@ function reset_focus() { } else { // Reset page selection and focus if (location.hash && document.querySelector(location.hash)) { - // scroll management has already happened earlier so we need to make sure - // the scroll position stays the same after setting the sequential focus navigation starting point const { x, y } = scroll_state(); setTimeout(() => { const history_state = history.state; // Mimic the browsers' behaviour and set the sequential focus navigation - // starting point to the fragment identifier... + // starting point to the fragment identifier location.replace(location.hash); - // ...but Firefox has a bug that sets the history state as null so we + // 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 + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1199924 history.replaceState(history_state, '', location.hash); - }); - scrollTo(x, y); + // Scroll management has already happened earlier so we need to restore + // the scroll position after setting the sequential focus navigation starting point + scrollTo(x, y); + }); } else { // We try to mimic browsers' behaviour as closely as possible by targeting the // first scrollable region, but unfortunately it's not a perfect match — e.g.