From bd79f352c4bc628a7970aa99c6f7c9048b78c5f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Thu, 25 Jan 2024 12:15:03 +0000 Subject: [PATCH] Introduce data-turbo-track="dynamic" (#1140) To track stylesheets and styles that we can dynamically remove when navigating. We introduced this behaviour in https://github.com/hotwired/turbo/pull/1128 and thought it would be a good default to avoid full page reloads when styles change. However, it seems it's quite common for libraries to add stylesheets and styles to the head that they want to keep around. For example, see - https://github.com/hotwired/turbo/pull/1133 - https://github.com/hotwired/turbo/issues/1139 So let's make this behaviour opt-in by adding a `data-turbo-track="dynamic"` attribute to stylesheets and styles that we want to dynamically remove when they are no longer in a new page navigation. This avoids any breaking changes for existing apps. --- src/core/drive/page_renderer.js | 14 +++++--------- src/core/drive/progress_bar.js | 2 -- src/tests/fixtures/stylesheets/left.html | 11 ++++++++--- src/tests/fixtures/stylesheets/right.html | 6 +++--- .../functional/drive_stylesheet_merging_tests.js | 9 ++++++--- 5 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/core/drive/page_renderer.js b/src/core/drive/page_renderer.js index 7e6df4c4c..c2789639b 100644 --- a/src/core/drive/page_renderer.js +++ b/src/core/drive/page_renderer.js @@ -78,7 +78,7 @@ export class PageRenderer extends Renderer { await newStylesheetElements if (this.willRender) { - this.removeUnusedHeadStylesheetElements() + this.removeUnusedDynamicStylesheetElements() } } @@ -111,8 +111,8 @@ export class PageRenderer extends Renderer { } } - removeUnusedHeadStylesheetElements() { - for (const element of this.unusedHeadStylesheetElements) { + removeUnusedDynamicStylesheetElements() { + for (const element of this.unusedDynamicStylesheetElements) { document.head.removeChild(element) } } @@ -182,13 +182,9 @@ export class PageRenderer extends Renderer { await this.renderElement(this.currentElement, this.newElement) } - get unusedHeadStylesheetElements() { + get unusedDynamicStylesheetElements() { return this.oldHeadStylesheetElements.filter((element) => { - return !(element.hasAttribute("data-turbo-permanent") || - // Trix dynamically adds styles to the head that we want to keep around which have a - // `data-tag-name` attribute. Long term we should moves those styles to Trix's CSS file - // but for now we'll just skip removing them - element.hasAttribute("data-tag-name")) + return element.getAttribute("data-turbo-track") === "dynamic" }) } diff --git a/src/core/drive/progress_bar.js b/src/core/drive/progress_bar.js index 8ce398ca4..76e8e55a7 100644 --- a/src/core/drive/progress_bar.js +++ b/src/core/drive/progress_bar.js @@ -106,8 +106,6 @@ export class ProgressBar { createStylesheetElement() { const element = document.createElement("style") - element.id = ProgressBarID - element.setAttribute("data-turbo-permanent", "") element.type = "text/css" element.textContent = ProgressBar.defaultCSS if (this.cspNonce) { diff --git a/src/tests/fixtures/stylesheets/left.html b/src/tests/fixtures/stylesheets/left.html index 7da4b67f9..4931d33ec 100644 --- a/src/tests/fixtures/stylesheets/left.html +++ b/src/tests/fixtures/stylesheets/left.html @@ -4,13 +4,18 @@ Left - - + + - + + diff --git a/src/tests/fixtures/stylesheets/right.html b/src/tests/fixtures/stylesheets/right.html index 13001bbf5..2805568e1 100644 --- a/src/tests/fixtures/stylesheets/right.html +++ b/src/tests/fixtures/stylesheets/right.html @@ -4,10 +4,10 @@ Right - - + + - diff --git a/src/tests/functional/drive_stylesheet_merging_tests.js b/src/tests/functional/drive_stylesheet_merging_tests.js index 6e38c2d82..d914a146e 100644 --- a/src/tests/functional/drive_stylesheet_merging_tests.js +++ b/src/tests/functional/drive_stylesheet_merging_tests.js @@ -6,19 +6,22 @@ test.beforeEach(async ({ page }) => { await page.goto("/src/tests/fixtures/stylesheets/left.html") }) -test("navigating removes unused style elements", async ({ page }) => { - assert.ok(await hasSelector(page, 'style[id="turbo-progress-bar"]')) +test("navigating removes unused dynamically tracked style elements", async ({ page }) => { + assert.ok(await hasSelector(page, 'style[id="added-style"]')) + assert.ok(await hasSelector(page, 'link[id="added-link"]')) await page.locator("#go-right").click() await nextBody(page) - assert.ok(await hasSelector(page, 'style[id="turbo-progress-bar"]')) 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 hasSelector(page, 'style[id="added-style"]')) + assert.ok(await hasSelector(page, 'link[id="added-link"]')) + assert.ok(await cssClassIsDefined(page, "right")) assert.notOk(await cssClassIsDefined(page, "left")) assert.equal(await getComputedStyle(page, "body", "marginLeft"), "0px")