Remove unused stylesheets when navigating

When navigating to another page, Turbo merges the <head> contents from the current and new pages, which results in a <head> containing the superset of both. For certain items, like scripts, this makes sense. We have no way to remove a running script. But that's not the case for styles: styles can be unloaded easily, and for the page to display properly, we need to do so. Otherwise styles kept in scope from a previous page could cause a page to render incorrectly. The common way to avoid this has been to use `data-turbo-track="reload"` to force a reload if styles change. This works, but it's a bit heavy-handed, causing full page reloads that could have been avoided. There are a couple of common cases where updating styles on the fly is useful: - Deploying a CSS change. Clients should be able to pick up the change without having to reload. - Allowing pages to include their own specific styles, rather than bundle them all together for the whole site. This can reduce the size of the loaded CSS, and make it easier to avoid style conflicts.
hotwired · Jan 12, 2024 · 517b646 · 517b646
1 parent 0741543
commit 517b646
Show file tree

Hide file tree

Showing 11 changed files with 102 additions and 3 deletions.
diff --git a/src/core/drive/page_renderer.js b/src/core/drive/page_renderer.js
@@ -1,5 +1,6 @@
-import { Renderer } from "../renderer"
 import { activateScriptElement, waitForLoad } from "../../util"
+import { Renderer } from "../renderer"
+import { ProgressBarID } from "./progress_bar"
 
 export class PageRenderer extends Renderer {
   static renderElement(currentElement, newElement) {
@@ -35,6 +36,7 @@ export class PageRenderer extends Renderer {
 
   async render() {
     if (this.willRender) {
+      this.removeUnusedHeadStylesheetElements()
       await this.replaceBody()
     }
   }
@@ -106,6 +108,12 @@ export class PageRenderer extends Renderer {
     }
   }
 
+  removeUnusedHeadStylesheetElements() {
+    for (const element of this.unusedHeadStylesheetElements) {
+      document.head.removeChild(element)
+    }
+  }
+
   async mergeProvisionalElements() {
     const newHeadElements = [...this.newHeadProvisionalElements]
 
@@ -171,6 +179,14 @@ export class PageRenderer extends Renderer {
     await this.renderElement(this.currentElement, this.newElement)
   }
 
+  get unusedHeadStylesheetElements() {
+    return this.oldHeadStylesheetElements.filter((element) => element.id !== ProgressBarID)
+  }
+
+  get oldHeadStylesheetElements() {
+    return this.currentHeadSnapshot.getStylesheetElementsNotInSnapshot(this.newHeadSnapshot)
+  }
+
   get newHeadStylesheetElements() {
     return this.newHeadSnapshot.getStylesheetElementsNotInSnapshot(this.currentHeadSnapshot)
   }

diff --git a/src/core/drive/progress_bar.js b/src/core/drive/progress_bar.js
@@ -1,5 +1,7 @@
 import { unindent, getMetaContent } from "../../util"
 
+export const ProgressBarID = "turbo-progress-bar"
+
 export class ProgressBar {
   static animationDuration = 300 /*ms*/
 
@@ -104,6 +106,7 @@ export class ProgressBar {
 
   createStylesheetElement() {
     const element = document.createElement("style")
+    element.id = ProgressBarID
     element.type = "text/css"
     element.textContent = ProgressBar.defaultCSS
     if (this.cspNonce) {

diff --git a/src/tests/fixtures/additional_assets.html b/src/tests/fixtures/additional_assets.html
@@ -6,6 +6,7 @@
     <link rel="stylesheet" type="text/css" href="/src/tests/fixtures/test.css">
     <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
     <script src="/src/tests/fixtures/test.js"></script>
+    <script id="additional" src="/src/tests/fixtures/test.js"></script>
     <noscript>
       <link rel="stylesheet" type="text/css" href="/src/tests/fixtures/noscript.css">
     </noscript>

diff --git a/src/tests/fixtures/stylesheets/common.css b/src/tests/fixtures/stylesheets/common.css
@@ -0,0 +1,5 @@
+body {
+  font-size: 32px;
+  font-weight: 200;
+  font-family: sans-serif;
+}
diff --git a/src/tests/fixtures/stylesheets/left.css b/src/tests/fixtures/stylesheets/left.css
@@ -0,0 +1,3 @@
+body {
+  background-color: red;
+}
diff --git a/src/tests/fixtures/stylesheets/left.html b/src/tests/fixtures/stylesheets/left.html
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <title>Left</title>
+    <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
+    <link rel="stylesheet" type="text/css" href="/src/tests/fixtures/stylesheets/common.css">
+    <link rel="stylesheet" type="text/css" href="/src/tests/fixtures/stylesheets/left.css">
+
+    <style>
+      p { font-weight: 800; }
+    </style>
+  </head>
+
+  <body></body>
+    <h1>Left</h1>
+    <p><a id="go-right" href="/src/tests/fixtures/stylesheets/right.html">go right</a></p>
+  </body>
+</html>
diff --git a/src/tests/fixtures/stylesheets/right.css b/src/tests/fixtures/stylesheets/right.css
@@ -0,0 +1,3 @@
+body {
+  background-color: yellow;
+}
diff --git a/src/tests/fixtures/stylesheets/right.html b/src/tests/fixtures/stylesheets/right.html
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <title>Right</title>
+    <script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
+    <link rel="stylesheet" type="text/css" href="/src/tests/fixtures/stylesheets/common.css">
+    <link rel="stylesheet" type="text/css" href="/src/tests/fixtures/stylesheets/right.css">
+
+    <style>
+      p { font-family: serif; }
+    </style>
+  </head>
+
+  <body></body>
+    <h1>Right</h1>
+    <p><a id="go-left" href="/src/tests/fixtures/stylesheets/left.html">go left</a></p>
+  </body>
+</html>
diff --git a/src/tests/functional/drive_stylesheet_merging_tests.js b/src/tests/functional/drive_stylesheet_merging_tests.js
@@ -0,0 +1,20 @@
+import { test } from "@playwright/test"
+import { assert } from "chai"
+import { getComputedStyle, hasSelector, nextBody } from "../helpers/page"
+
+test.beforeEach(async ({ page }) => {
+  await page.goto("/src/tests/fixtures/stylesheets/left.html")
+})
+
+test("navigating removes unused style elements", async ({ page }) => {
+  await page.locator("#go-right").click()
+  await nextBody(page)
+
+  assert.ok(await hasSelector(page, 'link[rel=stylesheet][href="/src/tests/fixtures/stylesheets/common.css"]'))
+  assert.ok(await hasSelector(page, 'link[rel=stylesheet][href="/src/tests/fixtures/stylesheets/right.css"]'))
+  assert.notOk(await hasSelector(page, 'link[rel=stylesheet][href="/src/tests/fixtures/stylesheets/left.css"]'))
+
+  assert.equal(await getComputedStyle(page, "p", "font-weight"), "200")
+  assert.equal(await getComputedStyle(page, "p", "font-family"), "serif")
+})
+
diff --git a/src/tests/functional/rendering_tests.js b/src/tests/functional/rendering_tests.js
@@ -222,8 +222,8 @@ test("changes the html[lang] attribute", async ({ page }) => {
   assert.equal(await page.getAttribute("html", "lang"), "es")
 })
 
-test("accumulates asset elements in head", async ({ page }) => {
-  const assetElements = () => page.$$('script, style, link[rel="stylesheet"]')
+test("accumulates script elements in head", async ({ page }) => {
+  const assetElements = () => page.$$('script')
   const originalElements = await assetElements()
 
   await page.click("#additional-assets-link")

diff --git a/src/tests/helpers/page.js b/src/tests/helpers/page.js
@@ -23,6 +23,16 @@ export function disposeAll(...handles) {
   return Promise.all(handles.map((handle) => handle.dispose()))
 }
 
+export function getComputedStyle(page, selector, propertyName) {
+  return page.evaluate(
+    ([selector, propertyName]) => {
+      const element = document.querySelector(selector)
+      return getComputedStyle(element)[propertyName]
+    },
+    [selector, propertyName]
+  )
+}
+
 export function getFromLocalStorage(page, key) {
   return page.evaluate((storageKey) => localStorage.getItem(storageKey), key)
 }