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 7 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
---

Persist styles of persistent client:only components during view transitions
martrapp marked this conversation as resolved.
Show resolved Hide resolved
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,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 */
11 changes: 6 additions & 5 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,8 +670,8 @@ 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', async ({ page, astro }) => {
const totalExpectedStyles = 8;

// Go to page 1
await page.goto(astro.resolveUrl('/client-only-one'));
Expand All @@ -686,6 +686,7 @@ test.describe('View Transitions', () => {
let pageTwo = page.locator('#page-two');
await expect(pageTwo, 'should have content').toHaveText('Page 2');

await page.waitForTimeout(500);
styles = await page.locator('style').all();
expect(styles.length).toEqual(totalExpectedStyles, 'style count has not changed');
});
Expand Down
63 changes: 59 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 @@ -519,3 +524,53 @@ if (inBrowser) {
markScriptsExec();
}
}

// Keep all styles that are potentially created by client:only components
// and required on the next page
//eslint-disable-next-line @typescript-eslint/no-unused-vars
async function prepareForClientOnlyComponents(newDocument: Document, toLocation: URL) {
martrapp marked this conversation as resolved.
Show resolved Hide resolved
if (
// Any persisted client:only component on the next page?
newDocument.body.querySelector(
`[${PERSIST_ATTR}] astro-island[client='only'],
astro-island[client='only'][${PERSIST_ATTR}]`
)
) {
// 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) {
// 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 those styles as persistent in the current head,
// if they came 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();
});
}
}
}
Loading