Remove unused stylesheets when navigating (hotwired#1128)

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 would be 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.
domchristie · Jul 15, 2024 · 8dbbc7d · 8dbbc7d
1 parent 4d5ceaa
commit 8dbbc7d
Show file tree

Hide file tree

Showing 12 changed files with 140 additions and 4 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) {
@@ -73,8 +74,13 @@ export class PageRenderer extends Renderer {
     const mergedHeadElements = this.mergeProvisionalElements()
     const newStylesheetElements = this.copyNewHeadStylesheetElements()
     this.copyNewHeadScriptElements()
+
     await mergedHeadElements
     await newStylesheetElements
+
+    if (this.willRender) {
+      this.removeUnusedHeadStylesheetElements()
+    }
   }
 
   async replaceBody() {
@@ -106,6 +112,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 +183,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_script.html b/src/tests/fixtures/additional_script.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8">
+    <title>Additional assets</title>
+    <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>
+  </head>
+  <body>
+    <h1>Additional assets</h1>
+  </body>
+</html>
diff --git a/src/tests/fixtures/rendering.html b/src/tests/fixtures/rendering.html
@@ -36,6 +36,7 @@ <h1>Rendering</h1>
       <form id="tracked-asset-change-form" action="/__turbo/notfound" method="post"><button type="submit">Submit</button></form>
       <p><a id="tracked-nonce-tag-link" href="/src/tests/fixtures/tracked_nonce_tag.html">Tracked nonce tag</a></p>
       <p><a id="additional-assets-link" href="/src/tests/fixtures/additional_assets.html">Additional assets</a></p>
+      <p><a id="additional-script-link" href="/src/tests/fixtures/additional_script.html">Additional script</a></p>
       <p><a id="head-script-link" href="/src/tests/fixtures/head_script.html">Head script</a></p>
       <p><a id="body-script-link" href="/src/tests/fixtures/body_script.html">Body script</a></p>
       <p><a id="eval-false-script-link" href="/src/tests/fixtures/eval_false_script.html">data-turbo-eval=false script</a></p>

diff --git a/src/tests/fixtures/stylesheets/common.css b/src/tests/fixtures/stylesheets/common.css
@@ -0,0 +1,5 @@
+body {
+  background-color: rgb(0, 0, 128);
+  color: rgb(0, 128, 0);
+  margin: 0;
+}
diff --git a/src/tests/fixtures/stylesheets/left.css b/src/tests/fixtures/stylesheets/left.css
@@ -0,0 +1,4 @@
+body {
+  background-color: rgb(128, 0, 0);
+  color: rgb(128, 0, 0);
+}
diff --git a/src/tests/fixtures/stylesheets/left.html b/src/tests/fixtures/stylesheets/left.html
@@ -0,0 +1,20 @@
+<!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>
+      body { margin-left: 20px; }
+      .left { margin-left: 20px; }
+    </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: rgb(0, 128, 0);
+}
diff --git a/src/tests/fixtures/stylesheets/right.html b/src/tests/fixtures/stylesheets/right.html
@@ -0,0 +1,20 @@
+<!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>
+      body { margin-right: 20px; }
+      .right { margin-right: 20px; }
+    </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,24 @@
+import { test } from "@playwright/test"
+import { assert } from "chai"
+import { cssClassIsDefined, 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, "body", "backgroundColor"), "rgb(0, 128, 0)")
+  assert.equal(await getComputedStyle(page, "body", "color"), "rgb(0, 128, 0)")
+
+  assert.ok(await cssClassIsDefined(page, "right"))
+  assert.notOk(await cssClassIsDefined(page, "left"))
+  assert.equal(await getComputedStyle(page, "body", "marginLeft"), "0px")
+  assert.equal(await getComputedStyle(page, "body", "marginRight"), "20px")
+})
+
diff --git a/src/tests/functional/rendering_tests.js b/src/tests/functional/rendering_tests.js
@@ -222,11 +222,11 @@ 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")
+  await page.click("#additional-script-link")
   await nextBody(page)
   const newElements = await assetElements()
   assert.notOk(await deepElementsEqual(page, newElements, originalElements))

diff --git a/src/tests/helpers/page.js b/src/tests/helpers/page.js
@@ -23,6 +23,28 @@ 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 cssClassIsDefined(page, className) {
+  return page.evaluate((className) => {
+    for (const stylesheet of document.styleSheets) {
+      for (const rule of stylesheet.cssRules) {
+        if (rule instanceof CSSStyleRule && rule.selectorText == `.${className}`) {
+          return true
+        }
+      }
+    }
+  }, className)
+}
+
 export function getFromLocalStorage(page, key) {
   return page.evaluate((storageKey) => localStorage.getItem(storageKey), key)
 }