From cfc465ddebcc58d20f29ecffaa857a77525435a9 Mon Sep 17 00:00:00 2001 From: Martin Trapp <94928215+martrapp@users.noreply.github.com> Date: Tue, 22 Aug 2023 17:17:30 +0200 Subject: [PATCH 01/11] fix: self link does not trigger page reload (#8182) --- .changeset/gold-carpets-film.md | 5 ++ .../astro/components/ViewTransitions.astro | 68 +++++++++++++------ .../view-transitions/src/pages/one.astro | 1 + packages/astro/e2e/view-transitions.test.js | 16 +++++ 4 files changed, 68 insertions(+), 22 deletions(-) create mode 100644 .changeset/gold-carpets-film.md diff --git a/.changeset/gold-carpets-film.md b/.changeset/gold-carpets-film.md new file mode 100644 index 000000000..dc17f7ab8 --- /dev/null +++ b/.changeset/gold-carpets-film.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +View Transitions: self link (`href=""`) does not trigger page reload diff --git a/packages/astro/components/ViewTransitions.astro b/packages/astro/components/ViewTransitions.astro index 4b7a46551..be312b1bf 100644 --- a/packages/astro/components/ViewTransitions.astro +++ b/packages/astro/components/ViewTransitions.astro @@ -274,30 +274,54 @@ const { fallback = 'animate' } = Astro.props as Props; // that is going to another page within the same origin. Basically it determines // same-origin navigation, but omits special key combos for new tabs, etc. if ( - link && - link instanceof HTMLAnchorElement && - link.href && - (!link.target || link.target === '_self') && - link.origin === location.origin && - !( - // Same page means same path and same query params - (location.pathname === link.pathname && location.search === link.search) - ) && - ev.button === 0 && // left clicks only - !ev.metaKey && // new tab (mac) - !ev.ctrlKey && // new tab (windows) - !ev.altKey && // download - !ev.shiftKey && - !ev.defaultPrevented && - transitionEnabledOnThisPage() - ) { - ev.preventDefault(); - navigate('forward', link.href, { index: ++currentHistoryIndex, scrollY: 0 }); - const newState: State = { index: currentHistoryIndex, scrollY }; - persistState({ index: currentHistoryIndex - 1, scrollY }); - history.pushState(newState, '', link.href); + !link || + !(link instanceof HTMLAnchorElement) || + !link.href || + (link.target && link.target !== '_self') || + link.origin !== location.origin || + ev.button !== 0 || // left clicks only + ev.metaKey || // new tab (mac) + ev.ctrlKey || // new tab (windows) + ev.altKey || // download + ev.shiftKey || // new window + ev.defaultPrevented || + !transitionEnabledOnThisPage() + ) + // 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; + } 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(); + persistState({ ...history.state, scrollY }); + scrollTo({ left: 0, top: 0, behavior: 'instant' }); + if (location.hash) { + // last target was different + const newState: State = { index: ++currentHistoryIndex, scrollY: 0 }; + history.pushState(newState, '', link.href); + } + return; + } } + + // these are the cases we will handle: same origin, different page + ev.preventDefault(); + navigate('forward', link.href, { index: ++currentHistoryIndex, scrollY: 0 }); + const newState: State = { index: currentHistoryIndex, scrollY }; + persistState({ index: currentHistoryIndex - 1, scrollY }); + history.pushState(newState, '', link.href); }); + addEventListener('popstate', (ev) => { if (!transitionEnabledOnThisPage()) { // The current page doesn't haven't View Transitions, diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/one.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/one.astro index b24338d9d..3f9666c1d 100644 --- a/packages/astro/e2e/fixtures/view-transitions/src/pages/one.astro +++ b/packages/astro/e2e/fixtures/view-transitions/src/pages/one.astro @@ -7,6 +7,7 @@ import Layout from '../components/Layout.astro'; go to 2 go to 3 go to long page + go to top
test content
diff --git a/packages/astro/e2e/view-transitions.test.js b/packages/astro/e2e/view-transitions.test.js index 7aeb6502a..69fa7c55f 100644 --- a/packages/astro/e2e/view-transitions.test.js +++ b/packages/astro/e2e/view-transitions.test.js @@ -190,6 +190,22 @@ test.describe('View Transitions', () => { await expect(p, 'should have content').toHaveText('Page 1'); }); + test('click self link (w/o hash) does not do navigation', async ({ page, astro }) => { + const loads = []; + page.addListener('load', (p) => { + loads.push(p.title()); + }); + // Go to page 1 + await page.goto(astro.resolveUrl('/one')); + const p = page.locator('#one'); + await expect(p, 'should have content').toHaveText('Page 1'); + + // Clicking href="" stays on page + await page.click('#click-self'); + await expect(p, 'should have content').toHaveText('Page 1'); + expect(loads.length, 'There should only be 1 page load').toEqual(1); + }); + test('Scroll position restored on back button', async ({ page, astro }) => { // Go to page 1 await page.goto(astro.resolveUrl('/long-page')); From fddd4dc71af321bd6b4d01bb4b1b955284846e60 Mon Sep 17 00:00:00 2001 From: Martin Trapp <94928215+martrapp@users.noreply.github.com> Date: Tue, 22 Aug 2023 18:59:17 +0200 Subject: [PATCH 02/11] Fixes in the client-side router (#8166) * Fixes in the client-side router * reverted function declaration after review (#8166) --------- Co-authored-by: Nate Moore --- .changeset/chilled-shoes-fail.md | 5 ++ .../astro/components/ViewTransitions.astro | 20 +++--- .../src/pages/half-baked.astro | 24 +++++++ .../view-transitions/src/pages/three.astro | 3 + packages/astro/e2e/view-transitions.test.js | 64 +++++++++++++++++++ 5 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 .changeset/chilled-shoes-fail.md create mode 100644 packages/astro/e2e/fixtures/view-transitions/src/pages/half-baked.astro diff --git a/.changeset/chilled-shoes-fail.md b/.changeset/chilled-shoes-fail.md new file mode 100644 index 000000000..1567ecca3 --- /dev/null +++ b/.changeset/chilled-shoes-fail.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +ViewTransitions: Fixes in the client-side router diff --git a/packages/astro/components/ViewTransitions.astro b/packages/astro/components/ViewTransitions.astro index be312b1bf..12dfe0f4f 100644 --- a/packages/astro/components/ViewTransitions.astro +++ b/packages/astro/components/ViewTransitions.astro @@ -20,15 +20,6 @@ const { fallback = 'animate' } = Astro.props as Props; type Events = 'astro:load' | 'astro:beforeload'; const persistState = (state: State) => history.replaceState(state, ''); - - // 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 - // can use that to determine popstate if going forward or back. - let currentHistoryIndex = history.state?.index || 0; - if (!history.state) { - persistState({ index: currentHistoryIndex, scrollY: 0 }); - } - const supportsViewTransitions = !!document.startViewTransition; const transitionEnabledOnThisPage = () => !!document.querySelector('[name="astro-view-transitions-enabled"]'); @@ -36,6 +27,14 @@ const { fallback = 'animate' } = Astro.props as Props; const onload = () => triggerEvent('astro:load'); const PERSIST_ATTR = 'data-astro-transition-persist'; + // 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 + // can use that to determine popstate if going forward or back. + let currentHistoryIndex = history.state?.index || 0; + if (!history.state && transitionEnabledOnThisPage()) { + persistState({ index: currentHistoryIndex, scrollY: 0 }); + } + const throttle = (cb: (...args: any[]) => any, delay: number) => { let wait = false; // During the waiting time additional events are lost. @@ -323,9 +322,10 @@ const { fallback = 'animate' } = Astro.props as Props; }); addEventListener('popstate', (ev) => { - if (!transitionEnabledOnThisPage()) { + if (!transitionEnabledOnThisPage() && ev.state) { // The current page doesn't haven't View Transitions, // respect that with a full page reload + // -- but only for transition managed by us (ev.state is set) location.reload(); return; } diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/half-baked.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/half-baked.astro new file mode 100644 index 000000000..40298d125 --- /dev/null +++ b/packages/astro/e2e/fixtures/view-transitions/src/pages/half-baked.astro @@ -0,0 +1,24 @@ +--- +import { ViewTransitions } from 'astro:transitions'; + +// For the test fixture, we import the script but we don't use the component +// While this seems to be some strange mistake, +// it might be realistic, e.g. in a configurable CommenHead component + +interface Props { + transitions?: string; +} +const { transitions } = Astro.props; +--- + + + Half-Baked + {transitions && } + + +
+

Half Baked

+ hash target +
+ + diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro index 676e8b61b..eddc049a8 100644 --- a/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro +++ b/packages/astro/e2e/fixtures/view-transitions/src/pages/three.astro @@ -6,6 +6,9 @@

Page 3

go to 2 +
+ hash target +

Long paragraph

diff --git a/packages/astro/e2e/view-transitions.test.js b/packages/astro/e2e/view-transitions.test.js index 69fa7c55f..c681bfea0 100644 --- a/packages/astro/e2e/view-transitions.test.js +++ b/packages/astro/e2e/view-transitions.test.js @@ -112,6 +112,40 @@ test.describe('View Transitions', () => { ).toEqual(2); }); + test('Moving within a page without ViewTransitions does not trigger a full page navigation', async ({ + page, + astro, + }) => { + const loads = []; + page.addListener('load', async (p) => { + loads.push(p.title()); + }); + // Go to page 1 + await page.goto(astro.resolveUrl('/one')); + let p = page.locator('#one'); + await expect(p, 'should have content').toHaveText('Page 1'); + + // Go to page 3 which does *not* have ViewTransitions enabled + await page.click('#click-three'); + p = page.locator('#three'); + await expect(p, 'should have content').toHaveText('Page 3'); + + // click a hash link to navigate further down the page + await page.click('#click-hash'); + // still on page 3 + p = page.locator('#three'); + await expect(p, 'should have content').toHaveText('Page 3'); + + // check that we are further down the page + const Y = await page.evaluate(() => window.scrollY); + expect(Y, 'The target is further down the page').toBeGreaterThan(0); + + expect( + loads.length, + 'There should be only 1 page load. The original, but no additional loads for the hash change' + ).toEqual(1); + }); + test('Moving from a page without ViewTransitions w/ back button', async ({ page, astro }) => { const loads = []; page.addListener('load', (p) => { @@ -332,4 +366,34 @@ test.describe('View Transitions', () => { await expect(loads.length, 'There should only be 1 page load').toEqual(1); }); + + test('Importing ViewTransitions w/o using the component must not mess with history', async ({ + page, + astro, + }) => { + const loads = []; + page.addListener('load', async (p) => { + loads.push(p); + }); + // Go to the half bakeed page + await page.goto(astro.resolveUrl('/half-baked')); + let p = page.locator('#half-baked'); + await expect(p, 'should have content').toHaveText('Half Baked'); + + // click a hash link to navigate further down the page + await page.click('#click-hash'); + // still on page + p = page.locator('#half-baked'); + await expect(p, 'should have content').toHaveText('Half Baked'); + + // go back within same page without reloading + await page.goBack(); + p = page.locator('#half-baked'); + await expect(p, 'should have content').toHaveText('Half Baked'); + + expect( + loads.length, + 'There should be only 1 page load. No additional loads for going back on same page' + ).toEqual(1); + }); }); From 9142178b113443749b87c1d259859b42a3d7a9c4 Mon Sep 17 00:00:00 2001 From: Martin Trapp <94928215+martrapp@users.noreply.github.com> Date: Tue, 22 Aug 2023 19:33:49 +0200 Subject: [PATCH 03/11] Mt scroll behavior (#8184) * The scrolling behavior of ViewTransition is now more similar to the expected browser behavior * format * removed browser detection --- .changeset/pretty-dancers-admire.md | 5 +++++ .../astro/components/ViewTransitions.astro | 19 +++++++++++++------ 2 files changed, 18 insertions(+), 6 deletions(-) create mode 100644 .changeset/pretty-dancers-admire.md diff --git a/.changeset/pretty-dancers-admire.md b/.changeset/pretty-dancers-admire.md new file mode 100644 index 000000000..13553488b --- /dev/null +++ b/.changeset/pretty-dancers-admire.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fix: The scrolling behavior of ViewTransitions is now more similar to the expected browser behavior diff --git a/packages/astro/components/ViewTransitions.astro b/packages/astro/components/ViewTransitions.astro index 12dfe0f4f..612b89659 100644 --- a/packages/astro/components/ViewTransitions.astro +++ b/packages/astro/components/ViewTransitions.astro @@ -164,14 +164,21 @@ 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' }); + if (state?.scrollY === 0 && location.hash) { const id = decodeURIComponent(location.hash.slice(1)); - state.scrollY = document.getElementById(id)?.offsetTop || 0; - } - if (state?.scrollY != null) { - scrollTo(0, state.scrollY); - // Overwrite erroneous updates by the scroll handler during transition - persistState(state); + const elem = document.getElementById(id); + // prefer scrollIntoView() over scrollTo() because it takes scroll-padding into account + if (elem) { + state.scrollY = elem.offsetTop; + persistState(state); // first guess, later updated by scroll handler + elem.scrollIntoView(); // for Firefox, this should better be {behavior: 'instant'} + } + } else if (state && state.scrollY !== 0) { + scrollTo(0, state.scrollY); // usings default scrollBehavior } triggerEvent('astro:beforeload'); From a571a1ac07fad4a898158b04594e03ad0c1de83b Mon Sep 17 00:00:00 2001 From: Paul Valladares <85648028+dreyfus92@users.noreply.github.com> Date: Tue, 22 Aug 2023 19:41:12 -0600 Subject: [PATCH 04/11] Fix(examples): changed inline-style to regular selector (#8185) * fix: changed inline-style to a regular selector * fix: fixed typo * fix: removed styles from noscript tags --- examples/portfolio/src/components/Nav.astro | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/portfolio/src/components/Nav.astro b/examples/portfolio/src/components/Nav.astro index 2e9717884..bf9ac7869 100644 --- a/examples/portfolio/src/components/Nav.astro +++ b/examples/portfolio/src/components/Nav.astro @@ -36,7 +36,7 @@ const iconLinks: { label: string; href: string; icon: keyof typeof iconPaths }[] -