Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Persist styles of persistent client:only components during view transitions in DEV mode #8840

Merged
merged 15 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/purple-dots-refuse.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes styles of `client:only` components not persisting during view transitions
martrapp marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { defineConfig } from 'astro/config';
import react from '@astrojs/react';
import vue from '@astrojs/vue';
import svelte from '@astrojs/svelte';
import nodejs from '@astrojs/node';

// https://astro.build/config
export default defineConfig({
output: 'hybrid',
adapter: nodejs({ mode: 'standalone' }),
integrations: [react()],
integrations: [react(),vue(),svelte()],
redirects: {
'/redirect-two': '/two',
'/redirect-external': 'http://example.com/',
Expand Down
4 changes: 4 additions & 0 deletions packages/astro/e2e/fixtures/view-transitions/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
"astro": "workspace:*",
"@astrojs/node": "workspace:*",
"@astrojs/react": "workspace:*",
"@astrojs/vue": "workspace:*",
"@astrojs/svelte": "workspace:*",
"svelte": "^4.2.0",
"vue": "^3.3.4",
"react": "^18.1.0",
"react-dom": "^18.1.0"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@

.counter-message {
text-align: center;
background-color: lightskyblue;
color:black
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, { useState } from 'react';
import './Island.css';
import { indirect} from './css.js';

export default function Counter({ children, count: initialCount, id }) {
const [count, setCount] = useState(initialCount);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<script lang="ts">
let count = 0;

function add() {
count += 1;
}

function subtract() {
count -= 1;
}
</script>

<div class="counter">
<button on:click={subtract}>-</button>
<pre>{count}</pre>
<button on:click={add}>+</button>
</div>
<div class="message">
<slot />
</div>

<style>
.counter {
display: grid;
font-size: 2em;
grid-template-columns: repeat(3, minmax(0, 1fr));
margin-top: 2em;
place-items: center;
}

.message {
text-align: center;
background-color: maroon;
color: tomato;
}
</style>
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<template>
<div class="counter">
<button @click="subtract()">-</button>
<pre>{{ count }}</pre>
<button @click="add()">+</button>
</div>
<div class="counter-message">
<slot />
</div>
</template>

<script lang="ts">
import { ref } from 'vue';
export default {
setup() {
const count = ref(0);
const add = () => (count.value = count.value + 1);
const subtract = () => (count.value = count.value - 1);

return {
count,
add,
subtract,
};
},
};
</script>

<style scoped>
.counter {
display: grid;
font-size: 2em;
grid-template-columns: repeat(3, minmax(0, 1fr));
margin-top: 2em;
place-items: center;
}

.counter-message {
text-align: center;
background: darkgreen;
color: greenyellow;
}
</style>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import "./other.postcss";
export const indirect = "<dummy>";

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/* not much to see */
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
import Layout from '../components/Layout.astro';
import Island from '../components/Island';
import VueCounter from '../components/VueCounter.vue';
import SvelteCounter from '../components/SvelteCounter.svelte';
---
<Layout>
<p id="page-four">Page 4</p>
<VueCounter client:only="vue">Vue</VueCounter>
<SvelteCounter client:only="svelte">Svelte</SvelteCounter>
</Layout>
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ import Island from '../components/Island';
<Layout>
<a id="click-two" href="/client-only-two">go to page 2</a>
<div transition:persist="island">
<Island client:only count={5}>message here</Island>
<Island id="one" client:only="react" count={5}>message here</Island>
</div>
</Layout>
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
import Layout from '../components/Layout.astro';
import Island from '../components/Island';
import VueCounter from '../components/VueCounter.vue';
import SvelteCounter from '../components/SvelteCounter.svelte';
---
<Layout>
<a id="click-four" href="/client-only-four">go to page 4</a>
<div>
<!-- intentional client:load, see /client-only-one for client:only -->
<Island id="one" client:load count={5}>message here</Island>
</div>
<VueCounter client:only="vue">Vue</VueCounter>
<SvelteCounter client:only="svelte">Svelte</SvelteCounter>
<p id="name">client-only-three</p>
</Layout>
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ import Island from '../components/Island';
<Layout>
<p id="page-two">Page 2</p>
<div transition:persist="island">
<Island client:only count={5}>message here</Island>
<Island id="two" client:only="react" count={5}>message here</Island>
</div>
</Layout>
32 changes: 26 additions & 6 deletions packages/astro/e2e/view-transitions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,15 +241,15 @@ test.describe('View Transitions', () => {
let p = page.locator('#totwo');
await expect(p, 'should have content').toHaveText('Go to listener two');
// on load a CSS transition is started triggered by a class on the html element
expect(transitions).toEqual(1);

expect(transitions).toBeLessThanOrEqual(1);
const transitionsBefore = transitions;
// go to page 2
await page.click('#totwo');
p = page.locator('#toone');
await expect(p, 'should have content').toHaveText('Go to listener one');
// swap() resets that class, the after-swap listener sets it again.
// the temporarily missing class must not trigger page rendering
expect(transitions).toEqual(1);
expect(transitions).toEqual(transitionsBefore);
});

test('click hash links does not do navigation', async ({ page, astro }) => {
Expand Down Expand Up @@ -670,10 +670,9 @@ test.describe('View Transitions', () => {
expect(loads.length, 'There should be 2 page loads').toEqual(2);
});

test.skip('client:only styles are retained on transition', async ({ page, astro }) => {
const totalExpectedStyles = 7;
test('client:only styles are retained on transition (1/2)', async ({ page, astro }) => {
const totalExpectedStyles = 8;

// Go to page 1
await page.goto(astro.resolveUrl('/client-only-one'));
let msg = page.locator('.counter-message');
await expect(msg).toHaveText('message here');
Expand All @@ -690,6 +689,27 @@ test.describe('View Transitions', () => {
expect(styles.length).toEqual(totalExpectedStyles, 'style count has not changed');
});

test('client:only styles are retained on transition (2/2)', async ({ page, astro }) => {
const totalExpectedStyles_page_three = 10;
const totalExpectedStyles_page_four = 8;

await page.goto(astro.resolveUrl('/client-only-three'));
let msg = page.locator('#name');
await expect(msg).toHaveText('client-only-three');
await page.waitForTimeout(400); // await hydration

let styles = await page.locator('style').all();
expect(styles.length).toEqual(totalExpectedStyles_page_three);

await page.click('#click-four');

let pageTwo = page.locator('#page-four');
await expect(pageTwo, 'should have content').toHaveText('Page 4');

styles = await page.locator('style').all();
expect(styles.length).toEqual(totalExpectedStyles_page_four, 'style count has not changed');
});

test('Horizontal scroll position restored on back button', async ({ page, astro }) => {
await page.goto(astro.resolveUrl('/wide-page'));
let article = page.locator('#widepage');
Expand Down
64 changes: 60 additions & 4 deletions packages/astro/src/transitions/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ const announce = () => {
};

const PERSIST_ATTR = 'data-astro-transition-persist';
const VITE_ID = 'data-vite-dev-id';

let parser: DOMParser;

Expand Down Expand Up @@ -202,8 +203,10 @@ async function updateDOM(
) {
// Check for a head element that should persist and returns it,
// either because it has the data attribute or is a link el.
const persistedHeadElement = (el: HTMLElement): Element | null => {
// Returns null if the element is not part of the new head, undefined if it should be left alone.
const persistedHeadElement = (el: HTMLElement): Element | null | undefined => {
const id = el.getAttribute(PERSIST_ATTR);
if (id === '') return undefined;
const newEl = id && newDocument.head.querySelector(`[${PERSIST_ATTR}="${id}"]`);
if (newEl) {
return newEl;
Expand All @@ -226,7 +229,7 @@ async function updateDOM(
// The element that currently has the focus is part of a DOM tree
// that will survive the transition to the new document.
// Save the element and the cursor position
if (activeElement?.closest('[data-astro-transition-persist]')) {
if (activeElement?.closest(`[${PERSIST_ATTR}]`)) {
if (
activeElement instanceof HTMLInputElement ||
activeElement instanceof HTMLTextAreaElement
Expand Down Expand Up @@ -290,7 +293,7 @@ async function updateDOM(
// from the new document and leave the current node alone
if (newEl) {
newEl.remove();
} else {
} else if (newEl === null) {
// Otherwise remove the element in the head. It doesn't exist in the new page.
el.remove();
}
Expand All @@ -306,6 +309,7 @@ async function updateDOM(

// this will reset scroll Position
document.body.replaceWith(newDocument.body);

for (const el of oldBody.querySelectorAll(`[${PERSIST_ATTR}]`)) {
const id = el.getAttribute(PERSIST_ATTR);
const newEl = document.querySelector(`[${PERSIST_ATTR}="${id}"]`);
Expand All @@ -315,7 +319,6 @@ async function updateDOM(
newEl.replaceWith(el);
}
}

restoreFocus(savedFocus);

if (popState) {
Expand Down Expand Up @@ -404,6 +407,8 @@ async function transition(
return;
}

if (import.meta.env.DEV) await prepareForClientOnlyComponents(newDocument, toLocation);

if (!popState) {
// save the current scroll position before we change the DOM and transition to the new page
history.replaceState({ ...history.state, scrollX, scrollY }, '');
Expand Down Expand Up @@ -438,6 +443,7 @@ export function navigate(href: string, options?: Options) {
'The view transtions client API was called during a server side render. This may be unintentional as the navigate() function is expected to be called in response to user interactions. Please make sure that your usage is correct.'
);
warning.name = 'Warning';
// eslint-disable-next-line no-console
console.warn(warning);
navigateOnServerWarned = true;
}
Expand Down Expand Up @@ -519,3 +525,53 @@ if (inBrowser) {
markScriptsExec();
}
}

// Keep all styles that are potentially created by client:only components
// and required on the next page
async function prepareForClientOnlyComponents(newDocument: Document, toLocation: URL) {
martrapp marked this conversation as resolved.
Show resolved Hide resolved
// Any client:only component on the next page?
if (newDocument.body.querySelector(`astro-island[client='only']`)) {
// Load the next page with an empty module loader cache
const nextPage = document.createElement('iframe');
nextPage.setAttribute('src', toLocation.href);
nextPage.style.display = 'none';
document.body.append(nextPage);
await hydrationDone(nextPage);

const nextHead = nextPage.contentDocument?.head;
if (nextHead) {
// Clear former persist marks
document.head
.querySelectorAll(`style[${PERSIST_ATTR}=""]`)
.forEach((s) => s.removeAttribute(PERSIST_ATTR));

// Collect the vite ids of all styles present in the next head
const viteIds = [...nextHead.querySelectorAll(`style[${VITE_ID}]`)].map((style) =>
style.getAttribute(VITE_ID)
);
// Mark styles of the current head as persistent
// if they come from hydration and not from the newDocument
viteIds.forEach((id) => {
const style = document.head.querySelector(`style[${VITE_ID}="${id}"]`);
if (style && !newDocument.head.querySelector(`style[${VITE_ID}="${id}"]`)) {
style.setAttribute(PERSIST_ATTR, '');
}
});
}

// return a promise that resolves when all astro-islands are hydrated
async function hydrationDone(loadingPage: HTMLIFrameElement) {
await new Promise(
(r) => loadingPage.contentWindow?.addEventListener('load', r, { once: true })
);

return new Promise<void>(async (r) => {
for (let count = 0; count <= 20; ++count) {
if (!loadingPage.contentDocument!.body.querySelector('astro-island[ssr]')) break;
await new Promise((r2) => setTimeout(r2, 50));
}
r();
});
}
}
}
12 changes: 12 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading