[fix] avoid downloading CSS with highest priority when inlineStyleThreshold is used

[dwsmart]

Fixes #3307
<link disabled rel="stylesheet" href="..."> still causes the browser to download the file, with the highest priority, even though it's never used.

This PR adds a media query with a 1px max screen width, this prevents the browser from downloading the CSS, but leaves the link tag in place so vite functions correctly.

<link disabled media="screen and (max-width: 1px)" rel="stylesheet" href="...">

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: a7cee81

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[netlify]

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: a7cee81

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61e871db3dc1670008f50b66

[Rich-Harris]

Thank you! Mind adding a changeset so this triggers a release?

Also, would be great to have a test for this — you could add one after this test, perhaps:

kit/packages/kit/test/apps/options/test/test.js

Lines 45 to 69 in 49114b9

	test('inlines CSS', async ({ page, javaScriptEnabled }) => {
	await page.goto('/path-base/base/');
	if (process.env.DEV) {
	const ssr_style = await page.evaluate(() => document.querySelector('style[data-svelte]'));
	if (javaScriptEnabled) {
	// <style data-svelte> is removed upon hydration
	expect(ssr_style).toBeNull();
	} else {
	expect(ssr_style).not.toBeNull();
	}
	expect(
	await page.evaluate(() => document.querySelector('link[rel="stylesheet"]'))
	).toBeNull();
	} else {
	expect(await page.evaluate(() => document.querySelector('style'))).not.toBeNull();
	expect(
	await page.evaluate(() => document.querySelector('link[rel="stylesheet"][disabled]'))
	).not.toBeNull();
	expect(
	await page.evaluate(() => document.querySelector('link[rel="stylesheet"]:not([disabled])'))
	).not.toBeNull();
	}
	});

Example of capturing which network requests were made:

kit/packages/kit/test/apps/basics/test/test.js

Lines 776 to 778 in 49114b9

	/** @type {string[]} */
	const requests = [];
	page.on('request', (r) => requests.push(r.url()));

[Rich-Harris]

I wonder if we can make this even simpler — any reason we can't change media to none? That way we can get rid of the disabled attribute too. This works for me locally, what do you reckon?

Suggested change

	.map((dep) => `\n\t<link${styles.has(dep) ? ' disabled media="screen and (max-width: 1px)"' : ''} rel="stylesheet" href="${options.prefix + dep}">`)
	.map((dep) => `\n\t<link${styles.has(dep) ? ' media="none"' : ''} rel="stylesheet" href="${options.prefix + dep}">`)

[dwsmart]

Will add the changeset and tests later, sorry!

I mistakenly believed that disabled needed to be in there for Vite, don't know where I got that from!

The media="none" thing is a pedantic point, strictly speaking there is no none value (only all, screen and print), so it's relying on browsers to interpret a value they don't understand as 'don't download this'. so screen and (max-width: 1px) is actually "valid" and actively instructing according to spec.

That being said, I've not met a browser yet that wouldn't work as intended with media="none", and I do think it's more intuitive and readable, so perhaps we should just use that?

An alternative would be media="print" which would prevent the browser from downloading (unless the user printed the page!), but still be spec compliant?

[Rich-Harris]

Fair point — I'm not much of a purist but I did wonder about that, and it would be nice to generate HTML that validates.

print seems good — no real harm if the user does print and download the styles. media="(max-width: 0)" would also work I think, but print is shortest.

[dwsmart]

OK made the changes to print, and amended the test, however, it seems like print (and any non-matching media value) do still get downloaded. Just with a lowest network priority.

That's still an improvement in my book, but I retitled this PR to more accurately reflect that.

[Rich-Harris]

I did some testing in different browsers. Turns out media="print" or media="(max-width: 0)" doesn't disable downloading, as you noted. disabled does disable downloading in Chrome and Safari, but not Firefox. But combining them — disable media="..." — disables downloading in all three.

On reflection I think (max-width: 0) is preferable to print, since it kind of says 'hello! i am a deliberate hack' to anyone who happens to look at the HTML

Suggested change

	.map((dep) => `\n\t<link${styles.has(dep) ? ' media="print"' : ''} rel="stylesheet" href="${options.prefix + dep}">`)
	.join('');
	.map((dep) => `\n\t<link${styles.has(dep) ? ' disabled media="(max-width: 0)"' : ''} rel="stylesheet" href="${options.prefix + dep}">`)
	.join('');

[Rich-Harris]

After all that back-and-forth it turns out we need to keep disabled in there! I've updated the PR, per the comment above — basically landed on exactly what you said in #3307 😅

Thanks for the help/patience

[dwsmart]

Thank you for your patience and help with the PR, and thanks to y'all for SvelteKit.