From 8dbbc7d4177da631479eab515c06ddc7655bc40a Mon Sep 17 00:00:00 2001 From: Kevin McConnell Date: Mon, 15 Jan 2024 13:20:12 +0000 Subject: [PATCH] Remove unused stylesheets when navigating (#1128) When navigating to another page, Turbo merges the `` contents from the current and new pages, which results in a `` 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. --- src/core/drive/page_renderer.js | 22 ++++++++++++++++- src/core/drive/progress_bar.js | 3 +++ src/tests/fixtures/additional_script.html | 14 +++++++++++ src/tests/fixtures/rendering.html | 1 + src/tests/fixtures/stylesheets/common.css | 5 ++++ src/tests/fixtures/stylesheets/left.css | 4 ++++ src/tests/fixtures/stylesheets/left.html | 20 ++++++++++++++++ src/tests/fixtures/stylesheets/right.css | 3 +++ src/tests/fixtures/stylesheets/right.html | 20 ++++++++++++++++ .../drive_stylesheet_merging_tests.js | 24 +++++++++++++++++++ src/tests/functional/rendering_tests.js | 6 ++--- src/tests/helpers/page.js | 22 +++++++++++++++++ 12 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 src/tests/fixtures/additional_script.html create mode 100644 src/tests/fixtures/stylesheets/common.css create mode 100644 src/tests/fixtures/stylesheets/left.css create mode 100644 src/tests/fixtures/stylesheets/left.html create mode 100644 src/tests/fixtures/stylesheets/right.css create mode 100644 src/tests/fixtures/stylesheets/right.html create mode 100644 src/tests/functional/drive_stylesheet_merging_tests.js diff --git a/src/core/drive/page_renderer.js b/src/core/drive/page_renderer.js index de9eb91ea..d9c78e5ac 100644 --- 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 index caff5f5da..bd0f6958b 100644 --- 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 new file mode 100644 index 000000000..0f67d936a --- /dev/null +++ b/src/tests/fixtures/additional_script.html @@ -0,0 +1,14 @@ + + + + + Additional assets + + + + + + +

Additional assets

+ + diff --git a/src/tests/fixtures/rendering.html b/src/tests/fixtures/rendering.html index ffaaa693a..f03e725a8 100644 --- a/src/tests/fixtures/rendering.html +++ b/src/tests/fixtures/rendering.html @@ -36,6 +36,7 @@

Rendering

Tracked nonce tag

Additional assets

+

Additional script

Head script

Body script

data-turbo-eval=false script

diff --git a/src/tests/fixtures/stylesheets/common.css b/src/tests/fixtures/stylesheets/common.css new file mode 100644 index 000000000..1f4632a21 --- /dev/null +++ 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 new file mode 100644 index 000000000..30f31ca46 --- /dev/null +++ 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 new file mode 100644 index 000000000..7da4b67f9 --- /dev/null +++ b/src/tests/fixtures/stylesheets/left.html @@ -0,0 +1,20 @@ + + + + + Left + + + + + + + + +

Left

+

go right

+ + diff --git a/src/tests/fixtures/stylesheets/right.css b/src/tests/fixtures/stylesheets/right.css new file mode 100644 index 000000000..38bdfb182 --- /dev/null +++ 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 new file mode 100644 index 000000000..13001bbf5 --- /dev/null +++ b/src/tests/fixtures/stylesheets/right.html @@ -0,0 +1,20 @@ + + + + + Right + + + + + + + + +

Right

+

go left

+ + diff --git a/src/tests/functional/drive_stylesheet_merging_tests.js b/src/tests/functional/drive_stylesheet_merging_tests.js new file mode 100644 index 000000000..c51e7421e --- /dev/null +++ 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 index d56f75e9a..9df7ffd04 100644 --- 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 index 886941e6a..760b6631f 100644 --- 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) }