Re-encode view-transition-names (#10099)

* Fixes an issue with view transition names containing spaces or punctuation. * reworked, more robust approach * better readability and also escapes the escape character (_) * update changeset * add comemnts to describe the re-encoding * updated changeset * typos * Apply suggestions from code review Co-authored-by: Sarah Rainsberger <[email protected]> * simplify decoding it ever required. * better coverage and now also checks animation behavior --------- Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: Emanuele Stoppa <[email protected]>
withastro · Feb 14, 2024 · b340f8f · b340f8f
1 parent 7443929
commit b340f8f
Show file tree

Hide file tree

Showing 4 changed files with 124 additions and 40 deletions.
diff --git a/.changeset/many-lobsters-accept.md b/.changeset/many-lobsters-accept.md
@@ -0,0 +1,11 @@
+---
+"astro": minor
+---
+
+Fixes a regression where view transition names containing special characters such as spaces or punctuation stopped working.
+
+Regular use naming your transitions with `transition: name` is unaffected.
+
+However, this fix may result in breaking changes if your project relies on the particular character encoding strategy Astro uses to translate `transition:name` directives into values of the underlying CSS `view-transition-name` property. For example, `Welcome to Astro` is now encoded as `Welcome_20to_20Astro_2e`.
+
+This mainly affects spaces and punctuation marks but no Unicode characters with codes >= 128.
diff --git a/packages/astro/e2e/fixtures/view-transitions/src/pages/transition-name.astro b/packages/astro/e2e/fixtures/view-transitions/src/pages/transition-name.astro
@@ -13,4 +13,25 @@ import Layout from '../components/Layout.astro';
 	<div id="nine" transition:name="--9">--9</div>
 	<div id="ten" transition:name="10">10</div>
 	<div id="eleven" transition:name="-11">-11</div>
+	<div id="twelve" transition:name="#! /">#!	 /</div>
+	<div id="thirteen" transition:name="_01__02___">_01__02___</div>
+	<div id="batch0" transition:name="\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f">control chars</div>
+	<div id="batch1" transition:name="\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f">punctation &amp; numbers</div>
+	<div id="batch2" transition:name="\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f">upper case chars</div>
+	<div id="batch3" transition:name="\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f">lower case chars</div>
+	<div id="batch4" transition:name="\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f">80-9f</div>
+	<div id="batch5" transition:name="\xa0\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf">a0-bf</div>
+	<div id="batch6" transition:name="\xc0\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf">c0-df</div>
+	<div id="batch7" transition:name="\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff">e0-ff</div>
+	<a id="navigate" href="">navigate</a>
 </Layout>
+<script>
+	document.addEventListener('animationstart', start);
+	function start(event:Event) {
+		if (event instanceof AnimationEvent) {
+			const name = event.animationName;
+			const match = name.match(/^-ua-view-transition-group-anim-(.*)$/)
+			if (match) console.log("anim:",match[1]);
+		}
+	}
+</script>
diff --git a/packages/astro/e2e/view-transitions.test.js b/packages/astro/e2e/view-transitions.test.js
@@ -1305,50 +1305,69 @@ test.describe('View Transitions', () => {
 	});
 
 	test('transition:name should be escaped correctly', async ({ page, astro }) => {
+		const expectedAnimations = new Set();
+		const checkName = async (selector, name) => {
+			expectedAnimations.add(name);
+			expect(page.locator(selector), 'should be escaped correctly').toHaveCSS(
+				'view-transition-name',
+				name
+			);
+		};
+
+		page.on('console', (msg) => {
+			if (msg.text().startsWith('anim: ')) {
+				const split = msg.text().split(' ', 2);
+				expectedAnimations.delete(split[1]);
+			}
+		});
+
 		await page.goto(astro.resolveUrl('/transition-name'));
-		await expect(page.locator('#one'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'front-end'
-		);
-		await expect(page.locator('#two'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'开源'
-		);
-		await expect(page.locator('#three'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'开a源'
-		);
-		await expect(page.locator('#four'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'c开a源c'
-		);
-		await expect(page.locator('#five'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'オープンソース'
-		);
-		await expect(page.locator('#six'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'开\\$源'
+
+		await checkName('#one', 'front-end');
+		await checkName('#two', '开源');
+		await checkName('#three', '开a源');
+		await checkName('#four', 'c开a源c');
+		await checkName('#five', 'オープンソース');
+		await checkName('#six', '开_24源');
+		await checkName('#seven', '开_2e源');
+		await checkName('#eight', '🐎👱❤');
+		await checkName('#nine', '_--9');
+		await checkName('#ten', '_10');
+		await checkName('#eleven', '_-11');
+		await checkName('#twelve', '__23_21_20_2f');
+		await checkName('#thirteen', '___01____02______');
+		await checkName(
+			'#batch0',
+			'__00_01_02_03_04_05_06_07_08_09_0a_0b_0c_0d_0e_0f_10_11_12_13_14_15_16_17_18_19_1a_1b_1c_1d_1e_1f'
 		);
-		await expect(page.locator('#seven'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'开\\.源'
+		await checkName(
+			'#batch1',
+			'__20_21_22_23_24_25_26_27_28_29_2a_2b_2c-_2e_2f0123456789_3a_3b_3c_3d_3e_3f'
 		);
-		await expect(page.locator('#eight'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'🐎👱❤'
+		await checkName('#batch2', '__40ABCDEFGHIJKLMNOPQRSTUVWXYZ_5b_5c_5d_5e__');
+		await checkName('#batch3', '__60abcdefghijklmnopqrstuvwxyz_7b_7c_7d_7e_7f');
+		await checkName(
+			'#batch4',
+			'\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f'
 		);
-		await expect(page.locator('#nine'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'--9'
+		await checkName(
+			'#batch5',
+			'\xa0\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf'
 		);
-		await expect(page.locator('#ten'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'\\31 0'
+		await checkName(
+			'#batch6',
+			'\xc0\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf'
 		);
-		await expect(page.locator('#eleven'), 'should be escaped correctly').toHaveCSS(
-			'view-transition-name',
-			'-\\31 1'
+		await checkName(
+			'#batch7',
+			'\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff'
 		);
+
+		await page.click('#navigate');
+		await page.waitForTimeout(400);	// yes, I dislike this, too. Might fix later.
+		expect(
+			expectedAnimations.size,
+			'all animations for transition:names should have been found'
+		).toEqual(0);
 	});
 });
diff --git a/packages/astro/src/runtime/server/transition.ts b/packages/astro/src/runtime/server/transition.ts
@@ -45,6 +45,39 @@ const addPairs = (
 	}
 };
 
+// Chrome (121) accepts custom-idents for view-transition-names as generated by cssesc,
+// but it just ignores them during view transitions if they contain escaped 7-bit ASCII characters
+// like \<space> or \. A special case are digits and minus at the beginning of the string,
+// which cssesc also encodes as \xx
+const reEncodeValidChars: string[] = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-_"
+	.split('').reduce((v, c) => (v[c.charCodeAt(0)] = c, v), [] as string[]);
+const reEncodeInValidStart: string[] = "-0123456789_"
+	.split('').reduce((v, c) => (v[c.charCodeAt(0)] = c, v), [] as string[]);
+
+function reEncode(s: string) {
+	let result = '';
+	let codepoint;
+	// we work on codepoints that might use more than 16bit, not character codes.
+	// so the index will often step by 1 as usual or by 2 if the codepoint is greater than 0xFFFF
+	for (let i = 0; i < s.length; i += (codepoint ?? 0) > 0xFFFF ? 2 : 1) {
+		codepoint = s.codePointAt(i);
+		if (codepoint !== undefined) { // this should never happen, they said!
+
+			// If we find a character in the range \x00 - \x7f that is not one of the reEncodeValidChars,
+			// we replace it with its hex value escaped by an underscore for decodability (and better readability,
+			// because most of them are punctuations like ,'"":;_..., and '_' might be a better choice than '-')
+			// The underscore itself (code 95) is also escaped and encoded as two underscores to avoid
+			// collitions between original and encoded strings.
+			// All other values are just copied over
+			result += codepoint < 0x80
+				? (codepoint === 95 ? "__" : (reEncodeValidChars[codepoint] ?? ('_' + (codepoint.toString(16).padStart(2, "0")))))
+				: String.fromCodePoint(codepoint);
+		}
+	}
+	// Digits and minus sign at the beginning of the string are special, so we simply prepend an underscore
+	return reEncodeInValidStart[result.codePointAt(0) ?? 0] ? '_' + result : result;
+}
+
 export function renderTransition(
 	result: SSRResult,
 	hash: string,
@@ -54,7 +87,7 @@ export function renderTransition(
 	// Default to `fade` (similar to `initial`, but snappier)
 	if (!animationName) animationName = 'fade';
 	const scope = createTransitionScope(result, hash);
-	const name = transitionName ? cssesc(transitionName, { isIdentifier: true }) : scope;
+	const name = transitionName ? cssesc(reEncode(transitionName), { isIdentifier: true }) : scope;
 	const sheet = new ViewTransitionStyleSheet(scope, name);
 
 	const animations = getAnimations(animationName);
@@ -90,7 +123,7 @@ class ViewTransitionStyleSheet {
 	constructor(
 		private scope: string,
 		private name: string
-	) {}
+	) { }
 
 	toString() {
 		const { scope, name } = this;