From af16eeed2c6f13021c998e5043d85788f0ef9fa8 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Thu, 14 Dec 2023 10:55:42 -0500 Subject: [PATCH 01/16] Dispatch `turbo:before-fetch-{request,response}` during preloading (#1034) Closes [#963][] Replace the raw call to `fetch` with a new `FetchRequest` instance that treats the `Preloaded` instances as its delegate. During that request's lifecycle, dispatch the `turbo:before-fetch-request` and `turbo:before-fetch-response` events with the `` element as its target. Prepare the request with the [Sec-Purpose][] header in the `prepareRequest` delegate callback. Write to the snapshot cache from within the `requestSucceededWithResponse` delegate callback. [#963]: https://github.com/hotwired/turbo/issues/963 [Sec-Purpose]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-Purpose#prefetch --- src/core/drive/preloader.js | 30 ++++++++++++++++++++----- src/tests/fixtures/hot_preloading.html | 1 + src/tests/fixtures/preloaded.html | 1 + src/tests/fixtures/preloading.html | 1 + src/tests/functional/preloader_tests.js | 14 ++++++++++++ 5 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/core/drive/preloader.js b/src/core/drive/preloader.js index b93d9b7ee..7deca1857 100644 --- a/src/core/drive/preloader.js +++ b/src/core/drive/preloader.js @@ -1,5 +1,5 @@ import { PageSnapshot } from "./page_snapshot" -import { fetch } from "../../http/fetch" +import { FetchMethod, FetchRequest } from "../../http/fetch_request" export class Preloader { selector = "a[data-turbo-preload]" @@ -36,17 +36,37 @@ export class Preloader { return } + const fetchRequest = new FetchRequest(this, FetchMethod.get, location, new URLSearchParams(), link) + await fetchRequest.perform() + } + + // Fetch request delegate + + prepareRequest(fetchRequest) { + fetchRequest.headers["Sec-Purpose"] = "prefetch" + } + + async requestSucceededWithResponse(fetchRequest, fetchResponse) { try { - const response = await fetch(location.toString(), { headers: { "Sec-Purpose": "prefetch", Accept: "text/html" } }) - const responseText = await response.text() - const snapshot = PageSnapshot.fromHTMLString(responseText) + const responseHTML = await fetchResponse.responseHTML + const snapshot = PageSnapshot.fromHTMLString(responseHTML) - this.snapshotCache.put(location, snapshot) + this.snapshotCache.put(fetchRequest.url, snapshot) } catch (_) { // If we cannot preload that is ok! } } + requestStarted(fetchRequest) {} + + requestErrored(fetchRequest) {} + + requestFinished(fetchRequest) {} + + requestPreventedHandlingResponse(fetchRequest, fetchResponse) {} + + requestFailedWithResponse(fetchRequest, fetchResponse) {} + #preloadAll = () => { this.preloadOnLoadLinksForView(document.body) } diff --git a/src/tests/fixtures/hot_preloading.html b/src/tests/fixtures/hot_preloading.html index 0a9d511c5..029fdda8e 100644 --- a/src/tests/fixtures/hot_preloading.html +++ b/src/tests/fixtures/hot_preloading.html @@ -5,6 +5,7 @@ Page That Links to Preloading Page + diff --git a/src/tests/fixtures/preloaded.html b/src/tests/fixtures/preloaded.html index 9b34768fb..99de99152 100644 --- a/src/tests/fixtures/preloaded.html +++ b/src/tests/fixtures/preloaded.html @@ -5,6 +5,7 @@ Preloaded Page + diff --git a/src/tests/fixtures/preloading.html b/src/tests/fixtures/preloading.html index 9bdacc1aa..db1a57534 100644 --- a/src/tests/fixtures/preloading.html +++ b/src/tests/fixtures/preloading.html @@ -5,6 +5,7 @@ Preloading Page + diff --git a/src/tests/functional/preloader_tests.js b/src/tests/functional/preloader_tests.js index d4c750b93..853e00b5c 100644 --- a/src/tests/functional/preloader_tests.js +++ b/src/tests/functional/preloader_tests.js @@ -12,6 +12,20 @@ test("preloads snapshot on initial load", async ({ page }) => { assert.ok(await urlInSnapshotCache(page, href)) }) +test("preloading dispatch turbo:before-fetch-{request,response} events", async ({ page }) => { + await page.goto("/src/tests/fixtures/preloading.html") + + const link = await page.locator("#preload_anchor") + const href = await link.evaluate((link) => link.href) + + const { url, fetchOptions } = await nextEventOnTarget(page, "preload_anchor", "turbo:before-fetch-request") + const { fetchResponse } = await nextEventOnTarget(page, "preload_anchor", "turbo:before-fetch-response") + + assert.equal(href, url, "dispatches request during preloading") + assert.equal(fetchOptions.headers.Accept, "text/html, application/xhtml+xml") + assert.equal(fetchResponse.response.url, href) +}) + test("preloads snapshot on page visit", async ({ page }) => { // contains `a[rel="preload"][href="http://localhost:9000/src/tests/fixtures/preloading.html"]` await page.goto("/src/tests/fixtures/hot_preloading.html") From df7f982bec1ed17d5d6b32ee3c2d34941f8ef9a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Mon, 18 Dec 2023 16:37:29 +0000 Subject: [PATCH 02/16] Set aria-busy on the form element during a form submission (#1110) This is useful for styling the form while it is submitting. Before this change, we were only setting aria-busy on frames while they were loading and on the html element while Turbo was processing a visit. --- src/core/drive/form_submission.js | 4 +++- src/tests/fixtures/form.html | 2 +- src/tests/functional/form_submission_tests.js | 19 +++++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/core/drive/form_submission.js b/src/core/drive/form_submission.js index e2fa643b2..a5007d2c5 100644 --- a/src/core/drive/form_submission.js +++ b/src/core/drive/form_submission.js @@ -1,6 +1,6 @@ import { FetchRequest, FetchMethod, fetchMethodFromString, fetchEnctypeFromString, isSafe } from "../../http/fetch_request" import { expandURL } from "../url" -import { dispatch, getAttribute, getMetaContent, hasAttribute } from "../../util" +import { clearBusyState, dispatch, getAttribute, getMetaContent, hasAttribute, markAsBusy } from "../../util" import { StreamMessage } from "../streams/stream_message" export const FormSubmissionState = { @@ -117,6 +117,7 @@ export class FormSubmission { this.state = FormSubmissionState.waiting this.submitter?.setAttribute("disabled", "") this.setSubmitsWith() + markAsBusy(this.formElement) dispatch("turbo:submit-start", { target: this.formElement, detail: { formSubmission: this } @@ -155,6 +156,7 @@ export class FormSubmission { this.state = FormSubmissionState.stopped this.submitter?.removeAttribute("disabled") this.resetSubmitterText() + clearBusyState(this.formElement) dispatch("turbo:submit-end", { target: this.formElement, detail: { formSubmission: this, ...this.result } diff --git a/src/tests/fixtures/form.html b/src/tests/fixtures/form.html index 707640393..aa46bcd2b 100644 --- a/src/tests/fixtures/form.html +++ b/src/tests/fixtures/form.html @@ -15,7 +15,7 @@

Form

-
+ diff --git a/src/tests/functional/form_submission_tests.js b/src/tests/functional/form_submission_tests.js index 987038448..e7447944f 100644 --- a/src/tests/functional/form_submission_tests.js +++ b/src/tests/functional/form_submission_tests.js @@ -131,6 +131,25 @@ test("standard POST form submission with redirect response", async ({ page }) => ) }) + +test("sets aria-busy on the form element during a form submission", async ({ page }) => { + await page.click("#standard form.redirect input[type=submit]") + + await nextEventNamed(page, "turbo:submit-start") + assert.equal( + await nextAttributeMutationNamed(page, "standard-form", "aria-busy"), + "true", + "sets [aria-busy] on the form element" + ) + + await nextEventNamed(page, "turbo:submit-end") + assert.equal( + await nextAttributeMutationNamed(page, "standard-form", "aria-busy"), + null, + "removes [aria-busy] from the form element" + ) +}) + test("standard POST form submission events", async ({ page }) => { await page.click("#standard-post-form-submit") From 2a638e037ceda39d94f29d9912f4947935bda448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Tue, 19 Dec 2023 09:43:53 +0000 Subject: [PATCH 03/16] Turbo 8.0.0-beta.2 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index c74caab8d..2bd3c2401 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@hotwired/turbo", - "version": "8.0.0-beta.1", + "version": "8.0.0-beta.2", "description": "The speed of a single-page web application without having to write any JavaScript", "module": "dist/turbo.es2017-esm.js", "main": "dist/turbo.es2017-umd.js", From f92bfa5df8b1c7c26da6aee8a684325914c5cfec Mon Sep 17 00:00:00 2001 From: Dennis Kamau <97116157+devcamke@users.noreply.github.com> Date: Wed, 3 Jan 2024 16:07:09 +0300 Subject: [PATCH 04/16] Update copyright year to 2024 (#1118) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 154c2698d..ed74ef6da 100644 --- a/README.md +++ b/README.md @@ -15,4 +15,4 @@ Read more on [turbo.hotwired.dev](https://turbo.hotwired.dev). Please read [CONTRIBUTING.md](./CONTRIBUTING.md). -© 2023 37signals LLC. +© 2024 37signals LLC. From 2147a143adaca5414c27b7e333845c0d0a4ede2f Mon Sep 17 00:00:00 2001 From: Bruno Prieto Date: Mon, 4 Dec 2023 05:10:29 -0300 Subject: [PATCH 05/16] Debounce page refreshes triggered via Turbo streams Fix Turbo 8 refresh debounce in frontend #1093 --- src/core/session.js | 14 +++++++++- .../page_refresh_stream_action_tests.js | 26 ++++++++++++++----- src/tests/helpers/page.js | 6 +++++ src/tests/unit/stream_element_tests.js | 6 ++--- src/util.js | 10 +++++++ 5 files changed, 51 insertions(+), 11 deletions(-) diff --git a/src/core/session.js b/src/core/session.js index 5921571f9..bf653539a 100644 --- a/src/core/session.js +++ b/src/core/session.js @@ -12,7 +12,7 @@ import { ScrollObserver } from "../observers/scroll_observer" import { StreamMessage } from "./streams/stream_message" import { StreamMessageRenderer } from "./streams/stream_message_renderer" import { StreamObserver } from "../observers/stream_observer" -import { clearBusyState, dispatch, findClosestRecursively, getVisitAction, markAsBusy } from "../util" +import { clearBusyState, dispatch, findClosestRecursively, getVisitAction, markAsBusy, debounce } from "../util" import { PageView } from "./drive/page_view" import { FrameElement } from "../elements/frame_element" import { Preloader } from "./drive/preloader" @@ -40,10 +40,13 @@ export class Session { progressBarDelay = 500 started = false formMode = "on" + #pageRefreshDebouncePeriod = 150 constructor(recentRequests) { this.recentRequests = recentRequests this.preloader = new Preloader(this, this.view.snapshotCache) + this.debouncedRefresh = this.refresh + this.pageRefreshDebouncePeriod = this.pageRefreshDebouncePeriod } start() { @@ -138,6 +141,15 @@ export class Session { return this.history.restorationIdentifier } + get pageRefreshDebouncePeriod() { + return this.#pageRefreshDebouncePeriod + } + + set pageRefreshDebouncePeriod(value) { + this.refresh = debounce(this.debouncedRefresh.bind(this), value) + this.#pageRefreshDebouncePeriod = value + } + // Preloader delegate shouldPreloadLink(element) { diff --git a/src/tests/functional/page_refresh_stream_action_tests.js b/src/tests/functional/page_refresh_stream_action_tests.js index cbdeb272a..0bc865922 100644 --- a/src/tests/functional/page_refresh_stream_action_tests.js +++ b/src/tests/functional/page_refresh_stream_action_tests.js @@ -1,6 +1,6 @@ import { test } from "@playwright/test" import { assert } from "chai" -import { nextBeat } from "../helpers/page" +import { nextPageRefresh, readEventLogs } from "../helpers/page" test.beforeEach(async ({ page }) => { await page.goto("/src/tests/fixtures/page_refresh_stream_action.html") @@ -9,11 +9,11 @@ test.beforeEach(async ({ page }) => { test("test refreshing the page", async ({ page }) => { assert.match(await textContent(page), /Hello/) - await page.locator("#content").evaluate((content)=>content.innerHTML = "") + await page.locator("#content").evaluate((content) => content.innerHTML = "") assert.notMatch(await textContent(page), /Hello/) await page.click("#refresh button") - await nextBeat() + await nextPageRefresh(page) assert.match(await textContent(page), /Hello/) }) @@ -21,12 +21,12 @@ test("test refreshing the page", async ({ page }) => { test("don't refresh the page on self-originated request ids", async ({ page }) => { assert.match(await textContent(page), /Hello/) - await page.locator("#content").evaluate((content)=>content.innerHTML = "") - page.evaluate(()=> { window.Turbo.session.recentRequests.add("123") }) + await page.locator("#content").evaluate((content) => content.innerHTML = "") + page.evaluate(() => { window.Turbo.session.recentRequests.add("123") }) - await page.locator("#request-id").evaluate((input)=>input.value = "123") + await page.locator("#request-id").evaluate((input) => input.value = "123") await page.click("#refresh button") - await nextBeat() + await nextPageRefresh(page) assert.notMatch(await textContent(page), /Hello/) }) @@ -42,6 +42,18 @@ test("fetch injects a Turbo-Request-Id with a UID generated automatically", asyn } }) +test("debounce stream page refreshes", async ({ page }) => { + await page.click("#refresh button") + await page.click("#refresh button") + await nextPageRefresh(page) + await page.click("#refresh button") + await nextPageRefresh(page) + + const eventLogs = await readEventLogs(page) + const requestLogs = eventLogs.filter(([name]) => name == "turbo:visit") + assert.equal(requestLogs.length, 2) +}) + async function textContent(page) { const messages = await page.locator("#content") return await messages.textContent() diff --git a/src/tests/helpers/page.js b/src/tests/helpers/page.js index a7a96ac52..886941e6a 100644 --- a/src/tests/helpers/page.js +++ b/src/tests/helpers/page.js @@ -68,6 +68,12 @@ export function nextBody(_page, timeout = 500) { return sleep(timeout) } +export async function nextPageRefresh(page, timeout = 500) { + const pageRefreshDebouncePeriod = await page.evaluate(() => window.Turbo.session.pageRefreshDebouncePeriod) + + return sleep(pageRefreshDebouncePeriod + timeout) +} + export async function nextEventNamed(page, eventName, expectedDetail = {}) { let record while (!record) { diff --git a/src/tests/unit/stream_element_tests.js b/src/tests/unit/stream_element_tests.js index 8ca8c48f3..21a9ca8aa 100644 --- a/src/tests/unit/stream_element_tests.js +++ b/src/tests/unit/stream_element_tests.js @@ -2,7 +2,7 @@ import { StreamElement } from "../../elements" import { nextAnimationFrame } from "../../util" import { DOMTestCase } from "../helpers/dom_test_case" import { assert } from "@open-wc/testing" -import { nextBeat } from "../helpers/page" +import { sleep } from "../helpers/page" import * as Turbo from "../../index" function createStreamElement(action, target, templateElement) { @@ -177,7 +177,7 @@ test("test action=refresh", async () => { const element = createStreamElement("refresh") subject.append(element) - await nextBeat() + await sleep(250) assert.notOk(document.body.hasAttribute("data-modified")) }) @@ -192,7 +192,7 @@ test("test action=refresh discarded when matching request id", async () => { element.setAttribute("request-id", "123") subject.append(element) - await nextBeat() + await sleep(250) assert.ok(document.body.hasAttribute("data-modified")) }) diff --git a/src/util.js b/src/util.js index 1dcf943fb..772e9896c 100644 --- a/src/util.js +++ b/src/util.js @@ -215,3 +215,13 @@ export async function around(callback, reader) { return [before, after] } + +export function debounce(fn, delay) { + let timeoutId = null + + return (...args) => { + const callback = () => fn.apply(this, args) + clearTimeout(timeoutId) + timeoutId = setTimeout(callback, delay) + } +} From 0741543f082cec008e0ecc228648c50240d82348 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Mon, 8 Jan 2024 13:23:36 +0000 Subject: [PATCH 06/16] Upgrade idiomorph to 0.3.0 (#1122) Use the upstream version that now includes a ESM dist file since https://github.com/bigskysoftware/idiomorph/commit/6cc541f4fe1bd341b16003c6560d243928f00b91 --- package.json | 2 +- src/core/drive/morph_renderer.js | 2 +- yarn.lock | 7 ++++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 2bd3c2401..afc8aded4 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ "chai": "~4.3.4", "eslint": "^8.13.0", "express": "^4.18.2", - "idiomorph": "https://github.com/basecamp/idiomorph#rollout-build", + "idiomorph": "^0.3.0", "multer": "^1.4.2", "rollup": "^2.35.1" }, diff --git a/src/core/drive/morph_renderer.js b/src/core/drive/morph_renderer.js index 2269e9ed4..beb94bfb1 100644 --- a/src/core/drive/morph_renderer.js +++ b/src/core/drive/morph_renderer.js @@ -1,4 +1,4 @@ -import Idiomorph from "idiomorph" +import { Idiomorph } from "idiomorph/dist/idiomorph.esm.js" import { dispatch } from "../../util" import { Renderer } from "../renderer" diff --git a/yarn.lock b/yarn.lock index b0e316fa0..45b1ad28f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1865,9 +1865,10 @@ iconv-lite@0.4.24: dependencies: safer-buffer ">= 2.1.2 < 3" -"idiomorph@https://github.com/basecamp/idiomorph#rollout-build": - version "0.0.8" - resolved "https://github.com/basecamp/idiomorph#e906820368e4c9c52489a3336b8c3826b1bf6de5" +idiomorph@^0.3.0: + version "0.3.0" + resolved "https://registry.yarnpkg.com/idiomorph/-/idiomorph-0.3.0.tgz#f6675bc5bef1a72c94021e43141a3f605d2d6288" + integrity sha512-UhV1Ey5xCxIwR9B+OgIjQa+1Jx99XQ1vQHUsKBU1RpQzCx1u+b+N6SOXgf5mEJDqemUI/ffccu6+71l2mJUsRA== ieee754@^1.1.13: version "1.2.1" From a73f6f16b00bb2c0ea3e64ee3f3860b1c779901d Mon Sep 17 00:00:00 2001 From: Kevin McConnell Date: Mon, 15 Jan 2024 13:20:12 +0000 Subject: [PATCH 07/16] 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) } From 32b8c3057d82ac004a1ebce1f45463d8493cbe93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Mon, 22 Jan 2024 10:04:40 +0000 Subject: [PATCH 08/16] Keep Trix dynamic styles in the head (#1133) * Keep Trix dynamic styles in the head Trix dynamically adds some styles to the head that we need to keep around. Otherwise, the editor will not render correctly after a page change. We can detect those styles because Trix also adds a `data-tag-name` attribute to the style elements it adds. Ideally, we would move those styles to Trix's CSS file, but for now we'll just skip removing them. We don't want to force everyone upgrading to Turbo v8 to also upgrade Trix. Ref: - https://github.com/basecamp/trix/blob/06d8b1db5fb682d007c5ca041884f6297674c8b7/src/trix/elements/trix_editor_element.js#L108-L162 - https://github.com/basecamp/trix/blob/06d8b1db5fb682d007c5ca041884f6297674c8b7/src/trix/core/helpers/custom_elements.js#L11 * Use data-turbo-permanent to keep styles around And use it to keep the progress bar styles around, instead of relying on an id. --- src/core/drive/page_renderer.js | 9 +++++++-- src/core/drive/progress_bar.js | 1 + src/tests/functional/drive_stylesheet_merging_tests.js | 3 +++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/core/drive/page_renderer.js b/src/core/drive/page_renderer.js index d9c78e5ac..76072638f 100644 --- a/src/core/drive/page_renderer.js +++ b/src/core/drive/page_renderer.js @@ -1,6 +1,5 @@ import { activateScriptElement, waitForLoad } from "../../util" import { Renderer } from "../renderer" -import { ProgressBarID } from "./progress_bar" export class PageRenderer extends Renderer { static renderElement(currentElement, newElement) { @@ -184,7 +183,13 @@ export class PageRenderer extends Renderer { } get unusedHeadStylesheetElements() { - return this.oldHeadStylesheetElements.filter((element) => element.id !== ProgressBarID) + 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-page-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-page-name")) + }) } get oldHeadStylesheetElements() { diff --git a/src/core/drive/progress_bar.js b/src/core/drive/progress_bar.js index bd0f6958b..8ce398ca4 100644 --- a/src/core/drive/progress_bar.js +++ b/src/core/drive/progress_bar.js @@ -107,6 +107,7 @@ 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/functional/drive_stylesheet_merging_tests.js b/src/tests/functional/drive_stylesheet_merging_tests.js index c51e7421e..6e38c2d82 100644 --- a/src/tests/functional/drive_stylesheet_merging_tests.js +++ b/src/tests/functional/drive_stylesheet_merging_tests.js @@ -7,9 +7,12 @@ test.beforeEach(async ({ page }) => { }) test("navigating removes unused style elements", async ({ page }) => { + assert.ok(await hasSelector(page, 'style[id="turbo-progress-bar"]')) + 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"]')) From bb735bf96ff3226e716e18a4ce8e0a0672714aa3 Mon Sep 17 00:00:00 2001 From: Dom Christie Date: Mon, 22 Jan 2024 10:33:35 +0000 Subject: [PATCH 09/16] Revert #926. (#1126) It is not necessary to pass along isPreview through various methods in order to determine whether a render is a preview since that can be determined via the data-turbo-preview attribute. (As discussed in https://github.com/hotwired/turbo/pull/926#issuecomment-1692292545) --- src/core/frames/frame_controller.js | 2 +- src/core/session.js | 16 ++++++++-------- src/core/view.js | 2 +- src/tests/functional/rendering_tests.js | 10 ---------- 4 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/core/frames/frame_controller.js b/src/core/frames/frame_controller.js index 5940ba761..02358d123 100644 --- a/src/core/frames/frame_controller.js +++ b/src/core/frames/frame_controller.js @@ -258,7 +258,7 @@ export class FrameController { // View delegate - allowsImmediateRender({ element: newFrame }, _isPreview, options) { + allowsImmediateRender({ element: newFrame }, options) { const event = dispatch("turbo:before-frame-render", { target: this.element, detail: { newFrame, ...options }, diff --git a/src/core/session.js b/src/core/session.js index bf653539a..8dafa6f13 100644 --- a/src/core/session.js +++ b/src/core/session.js @@ -298,8 +298,8 @@ export class Session { } } - allowsImmediateRender({ element }, isPreview, options) { - const event = this.notifyApplicationBeforeRender(element, isPreview, options) + allowsImmediateRender({ element }, options) { + const event = this.notifyApplicationBeforeRender(element, options) const { defaultPrevented, detail: { render } @@ -312,9 +312,9 @@ export class Session { return !defaultPrevented } - viewRenderedSnapshot(_snapshot, isPreview, renderMethod) { + viewRenderedSnapshot(_snapshot, _isPreview, renderMethod) { this.view.lastRenderedLocation = this.history.location - this.notifyApplicationAfterRender(isPreview, renderMethod) + this.notifyApplicationAfterRender(renderMethod) } preloadOnLoadLinksForView(element) { @@ -370,15 +370,15 @@ export class Session { return dispatch("turbo:before-cache") } - notifyApplicationBeforeRender(newBody, isPreview, options) { + notifyApplicationBeforeRender(newBody, options) { return dispatch("turbo:before-render", { - detail: { newBody, isPreview, ...options }, + detail: { newBody, ...options }, cancelable: true }) } - notifyApplicationAfterRender(isPreview, renderMethod) { - return dispatch("turbo:render", { detail: { isPreview, renderMethod } }) + notifyApplicationAfterRender(renderMethod) { + return dispatch("turbo:render", { detail: { renderMethod } }) } notifyApplicationAfterPageLoad(timing = {}) { diff --git a/src/core/view.js b/src/core/view.js index d077e9a24..8c5ae2e82 100644 --- a/src/core/view.js +++ b/src/core/view.js @@ -65,7 +65,7 @@ export class View { const renderInterception = new Promise((resolve) => (this.#resolveInterceptionPromise = resolve)) const options = { resume: this.#resolveInterceptionPromise, render: this.renderer.renderElement } - const immediateRender = this.delegate.allowsImmediateRender(snapshot, isPreview, options) + const immediateRender = this.delegate.allowsImmediateRender(snapshot, options) if (!immediateRender) await renderInterception await this.renderSnapshot(renderer) diff --git a/src/tests/functional/rendering_tests.js b/src/tests/functional/rendering_tests.js index 9df7ffd04..b4705331c 100644 --- a/src/tests/functional/rendering_tests.js +++ b/src/tests/functional/rendering_tests.js @@ -36,16 +36,6 @@ test("triggers before-render and render events", async ({ page }) => { assert.equal(await newBody, await page.evaluate(() => document.body.outerHTML)) }) -test("includes isPreview in render event details", async ({ page }) => { - await page.click("#same-origin-link") - - const { isPreview } = await nextEventNamed(page, "turbo:before-render") - assert.equal(isPreview, false) - - await nextEventNamed(page, "turbo:render") - assert.equal(await isPreview, false) -}) - test("triggers before-render, render, and load events for error pages", async ({ page }) => { await page.click("#nonexistent-link") const { newBody } = await nextEventNamed(page, "turbo:before-render") From 623a9df1ef1ee19f1d439a71b8987d831032e697 Mon Sep 17 00:00:00 2001 From: David Alejandro <15317732+davidalejandroaguilar@users.noreply.github.com> Date: Mon, 22 Jan 2024 03:58:49 -0700 Subject: [PATCH 10/16] Add InstantClick behavior (#1101) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Move doesNotTargetIFrame to util.js * Move findLinkFromClickTarget to util.js * Move getLocationForLink to util.js * Allow request to be intercepted and overriden on turbo:before-fetch-request * Add instantclick behavior * Allow customizing the event that triggers prefetching * Allow customizing the cache time for prefetching * Rename LinkPrefetchOnMouseoverObserver to LinkPrefetchObserver Because it is not only triggered on mouseover, but could also be on mousedown, or eventually touchstart. * Use private methods in LinkPrefetchObserver * Reorganize methods on LinkPrefetchObserver * Require a shorter sleep time in the test Since turbo-prefetch-cache-time is set to 1 millisecond in the html fixture * Standardize anchor IDs in link_prefetch_observer_tests anchor_ prefix is used for all anchors in the tests * Don't try traverse DOM to determine if the target is a link This is not necessary, since we can just check if the target is an anchor element with an href attribute. We were just using findLinkFromClickTarget because it had the selector we needed, but we can just use the selector directly. * Keep the closing tag on the same line as the rest of the tag * Remove unnecessary nesting in tests * Add missing newline at end of file * Check for prefetch meta tag before prefetching (on hover event) * Use FetchRequest to build request for LinkPrefetchObserver * LinkPrefetchObserver implements the FetchRequest interface, so it can be used to build a request. * It also adds this.response to FetchRequest to store the non-awaited `fetch` response, because we need to FetchRequest#receive() a `fetch` response, not a FetchRequest. * Add Turbo Stream header to Accept header when link has data-turbo-stream * Bring back prefetching links with inner elements * Add cancelable delay to prefetching links on hover * Fix clearing cache on every prefetch after b9e82f2 * Add tests for the delay on the meta tag * Use mouseenter and mouseleave instead of mouseover and mouseout To avoid having to traverse the DOM to find the link element * Remove unneeded comment * Use double quotes instead of single quotes for consistency * Move link variable declaration inside if statement Since target is only a link if isLink is true * Use correct key name for mouseenter event on LinkPrefetchObserver.triggerEvents On 5078e0b we started using the `mouseenter` event instead of the `mouseover` event to trigger prefetching. However, we forgot to update the key name on the `LinkPrefetchObserver.triggerEvents` object. * Allow prefetching when visiting page without meta, then visiting one with it * Allow create and delete posts with comments on the test server * Clear prefetch cache after form submission * Add test for nested data-turbo-prefetch=true within data-turbo-prefetch=false * No longer allow customizing the prefetch trigger event * No longer allow customizing the prefetch delay * Add touchstart event to prefetch observer * Fix flaky tests This commit fixes the flaky tests by ensuring that each worker has its own database file. This is done by adding a `worker_id` query parameter to the URLs of the pages that are being tested. This `worker_id` is passed to the database functions, which then use it to determine the name of the database file. It's necessary because the tests are running in parallel, and the database file is shared between all the workers. This means that if one worker creates a post, the other workers will see that post, and the tests will fail. * Use double quotes instead of single quotes * Only cache the link you're currently hovering Instead of maintaining a cache of all the links that have been hovered in the last 10 seconds. This solves issues where the user hovers a link, then performs a non-safe action and then later clicks the link. In this case, we would be showing stale content from before the action was performed. * Remove unused files after ETA template rendered removal * Remove unused variable * Clear prefetch cache when the link is no longer hovered This avoids a flurry of requests when casually scrolling down a page * Style changes --------- Co-authored-by: Alberto Fernández-Capel --- playwright.config.js | 6 +- src/core/drive/form_submission.js | 10 +- src/core/drive/prefetch_cache.js | 34 ++ src/core/session.js | 13 + src/http/fetch_request.js | 13 +- src/observers/form_link_click_observer.js | 10 + src/observers/link_click_observer.js | 27 +- src/observers/link_prefetch_observer.js | 179 +++++++++++ src/tests/fixtures/hover_to_prefetch.html | 45 +++ .../hover_to_prefetch_custom_cache_time.html | 14 + .../fixtures/hover_to_prefetch_disabled.html | 13 + .../fixtures/hover_to_prefetch_iframe.html | 13 + ...t_meta_tag_with_link_to_with_meta_tag.html | 19 ++ src/tests/fixtures/prefetched.html | 12 + .../link_prefetch_observer_tests.js | 303 ++++++++++++++++++ src/util.js | 20 ++ 16 files changed, 702 insertions(+), 29 deletions(-) create mode 100644 src/core/drive/prefetch_cache.js create mode 100644 src/observers/link_prefetch_observer.js create mode 100644 src/tests/fixtures/hover_to_prefetch.html create mode 100644 src/tests/fixtures/hover_to_prefetch_custom_cache_time.html create mode 100644 src/tests/fixtures/hover_to_prefetch_disabled.html create mode 100644 src/tests/fixtures/hover_to_prefetch_iframe.html create mode 100644 src/tests/fixtures/hover_to_prefetch_without_meta_tag_with_link_to_with_meta_tag.html create mode 100644 src/tests/fixtures/prefetched.html create mode 100644 src/tests/functional/link_prefetch_observer_tests.js diff --git a/playwright.config.js b/playwright.config.js index aae8335d2..4b4dfdc22 100644 --- a/playwright.config.js +++ b/playwright.config.js @@ -8,7 +8,8 @@ const config = { ...devices["Desktop Chrome"], contextOptions: { timeout: 60000 - } + }, + hasTouch: true } }, { @@ -17,7 +18,8 @@ const config = { ...devices["Desktop Firefox"], contextOptions: { timeout: 60000 - } + }, + hasTouch: true } } ], diff --git a/src/core/drive/form_submission.js b/src/core/drive/form_submission.js index a5007d2c5..c2ac1a0db 100644 --- a/src/core/drive/form_submission.js +++ b/src/core/drive/form_submission.js @@ -2,6 +2,7 @@ import { FetchRequest, FetchMethod, fetchMethodFromString, fetchEnctypeFromStrin import { expandURL } from "../url" import { clearBusyState, dispatch, getAttribute, getMetaContent, hasAttribute, markAsBusy } from "../../util" import { StreamMessage } from "../streams/stream_message" +import { prefetchCache } from "./prefetch_cache" export const FormSubmissionState = { initialized: "initialized", @@ -126,13 +127,20 @@ export class FormSubmission { } requestPreventedHandlingResponse(request, response) { + prefetchCache.clear() + this.result = { success: response.succeeded, fetchResponse: response } } requestSucceededWithResponse(request, response) { if (response.clientError || response.serverError) { this.delegate.formSubmissionFailedWithResponse(this, response) - } else if (this.requestMustRedirect(request) && responseSucceededWithoutRedirect(response)) { + return + } + + prefetchCache.clear() + + if (this.requestMustRedirect(request) && responseSucceededWithoutRedirect(response)) { const error = new Error("Form responses must redirect to another location") this.delegate.formSubmissionErrored(this, error) } else { diff --git a/src/core/drive/prefetch_cache.js b/src/core/drive/prefetch_cache.js new file mode 100644 index 000000000..ecca297b7 --- /dev/null +++ b/src/core/drive/prefetch_cache.js @@ -0,0 +1,34 @@ +const PREFETCH_DELAY = 100 + +class PrefetchCache { + #prefetchTimeout = null + #prefetched = null + + get(url) { + if (this.#prefetched && this.#prefetched.url === url && this.#prefetched.expire > Date.now()) { + return this.#prefetched.request + } + } + + setLater(url, request, ttl) { + this.clear() + + this.#prefetchTimeout = setTimeout(() => { + request.perform() + this.set(url, request, ttl) + this.#prefetchTimeout = null + }, PREFETCH_DELAY) + } + + set(url, request, ttl) { + this.#prefetched = { url, request, expire: new Date(new Date().getTime() + ttl) } + } + + clear() { + if (this.#prefetchTimeout) clearTimeout(this.#prefetchTimeout) + this.#prefetched = null + } +} + +export const cacheTtl = 10 * 1000 +export const prefetchCache = new PrefetchCache() diff --git a/src/core/session.js b/src/core/session.js index 8dafa6f13..7bfb20ca2 100644 --- a/src/core/session.js +++ b/src/core/session.js @@ -3,6 +3,7 @@ import { CacheObserver } from "../observers/cache_observer" import { FormSubmitObserver } from "../observers/form_submit_observer" import { FrameRedirector } from "./frames/frame_redirector" import { History } from "./drive/history" +import { LinkPrefetchObserver } from "../observers/link_prefetch_observer" import { LinkClickObserver } from "../observers/link_click_observer" import { FormLinkClickObserver } from "../observers/form_link_click_observer" import { getAction, expandURL, locationIsVisitable } from "./url" @@ -26,6 +27,7 @@ export class Session { pageObserver = new PageObserver(this) cacheObserver = new CacheObserver() + linkPrefetchObserver = new LinkPrefetchObserver(this, document) linkClickObserver = new LinkClickObserver(this, window) formSubmitObserver = new FormSubmitObserver(this, document) scrollObserver = new ScrollObserver(this) @@ -53,6 +55,7 @@ export class Session { if (!this.started) { this.pageObserver.start() this.cacheObserver.start() + this.linkPrefetchObserver.start() this.formLinkClickObserver.start() this.linkClickObserver.start() this.formSubmitObserver.start() @@ -74,6 +77,7 @@ export class Session { if (this.started) { this.pageObserver.stop() this.cacheObserver.stop() + this.linkPrefetchObserver.stop() this.formLinkClickObserver.stop() this.linkClickObserver.stop() this.formSubmitObserver.stop() @@ -199,6 +203,15 @@ export class Session { submittedFormLinkToLocation() {} + // Link hover observer delegate + + canPrefetchRequestToLocation(link, location) { + return ( + this.elementIsNavigatable(link) && + locationIsVisitable(location, this.snapshot.rootLocation) + ) + } + // Link click observer delegate willFollowLinkToLocation(link, location, event) { diff --git a/src/http/fetch_request.js b/src/http/fetch_request.js index 3d79f24ae..f4ff5adbe 100644 --- a/src/http/fetch_request.js +++ b/src/http/fetch_request.js @@ -121,10 +121,17 @@ export class FetchRequest { async perform() { const { fetchOptions } = this this.delegate.prepareRequest(this) - await this.#allowRequestToBeIntercepted(fetchOptions) + const event = await this.#allowRequestToBeIntercepted(fetchOptions) try { this.delegate.requestStarted(this) - const response = await fetch(this.url.href, fetchOptions) + + if (event.detail.fetchRequest) { + this.response = event.detail.fetchRequest.response + } else { + this.response = fetch(this.url.href, fetchOptions) + } + + const response = await this.response return await this.receive(response) } catch (error) { if (error.name !== "AbortError") { @@ -186,6 +193,8 @@ export class FetchRequest { }) this.url = event.detail.url if (event.defaultPrevented) await requestInterception + + return event } #willDelegateErrorHandling(error) { diff --git a/src/observers/form_link_click_observer.js b/src/observers/form_link_click_observer.js index 55b94bb64..c6471fba4 100644 --- a/src/observers/form_link_click_observer.js +++ b/src/observers/form_link_click_observer.js @@ -15,6 +15,16 @@ export class FormLinkClickObserver { this.linkInterceptor.stop() } + // Link hover observer delegate + + canPrefetchRequestToLocation(link, location) { + return false + } + + prefetchAndCacheRequestToLocation(link, location) { + return + } + // Link click observer delegate willFollowLinkToLocation(link, location, originalEvent) { diff --git a/src/observers/link_click_observer.js b/src/observers/link_click_observer.js index e6bd2fcaf..24c6aa235 100644 --- a/src/observers/link_click_observer.js +++ b/src/observers/link_click_observer.js @@ -1,5 +1,4 @@ -import { expandURL } from "../core/url" -import { findClosestRecursively } from "../util" +import { doesNotTargetIFrame, findLinkFromClickTarget, getLocationForLink } from "../util" export class LinkClickObserver { started = false @@ -31,9 +30,9 @@ export class LinkClickObserver { clickBubbled = (event) => { if (event instanceof MouseEvent && this.clickEventIsSignificant(event)) { const target = (event.composedPath && event.composedPath()[0]) || event.target - const link = this.findLinkFromClickTarget(target) + const link = findLinkFromClickTarget(target) if (link && doesNotTargetIFrame(link)) { - const location = this.getLocationForLink(link) + const location = getLocationForLink(link) if (this.delegate.willFollowLinkToLocation(link, location, event)) { event.preventDefault() this.delegate.followedLinkToLocation(link, location) @@ -53,24 +52,4 @@ export class LinkClickObserver { event.shiftKey ) } - - findLinkFromClickTarget(target) { - return findClosestRecursively(target, "a[href]:not([target^=_]):not([download])") - } - - getLocationForLink(link) { - return expandURL(link.getAttribute("href") || "") - } -} - -function doesNotTargetIFrame(anchor) { - if (anchor.hasAttribute("target")) { - for (const element of document.getElementsByName(anchor.target)) { - if (element instanceof HTMLIFrameElement) return false - } - - return true - } else { - return true - } } diff --git a/src/observers/link_prefetch_observer.js b/src/observers/link_prefetch_observer.js new file mode 100644 index 000000000..de08e05da --- /dev/null +++ b/src/observers/link_prefetch_observer.js @@ -0,0 +1,179 @@ +import { + doesNotTargetIFrame, + getLocationForLink, + getMetaContent, + findClosestRecursively +} from "../util" + +import { FetchMethod, FetchRequest } from "../http/fetch_request" +import { prefetchCache, cacheTtl } from "../core/drive/prefetch_cache" + +export class LinkPrefetchObserver { + started = false + hoverTriggerEvent = "mouseenter" + touchTriggerEvent = "touchstart" + + constructor(delegate, eventTarget) { + this.delegate = delegate + this.eventTarget = eventTarget + } + + start() { + if (this.started) return + + if (this.eventTarget.readyState === "loading") { + this.eventTarget.addEventListener("DOMContentLoaded", this.#enable, { once: true }) + } else { + this.#enable() + } + } + + stop() { + if (!this.started) return + + this.eventTarget.removeEventListener(this.hoverTriggerEvent, this.#tryToPrefetchRequest, { + capture: true, + passive: true + }) + this.eventTarget.removeEventListener(this.touchTriggerEvent, this.#tryToPrefetchRequest, { + capture: true, + passive: true + }) + this.eventTarget.removeEventListener("turbo:before-fetch-request", this.#tryToUsePrefetchedRequest, true) + this.started = false + } + + #enable = () => { + this.eventTarget.addEventListener(this.hoverTriggerEvent, this.#tryToPrefetchRequest, { + capture: true, + passive: true + }) + this.eventTarget.addEventListener(this.touchTriggerEvent, this.#tryToPrefetchRequest, { + capture: true, + passive: true + }) + this.eventTarget.addEventListener("turbo:before-fetch-request", this.#tryToUsePrefetchedRequest, true) + this.started = true + } + + #tryToPrefetchRequest = (event) => { + if (getMetaContent("turbo-prefetch") !== "true") return + + const target = event.target + const isLink = target.matches && target.matches("a[href]:not([target^=_]):not([download])") + + if (isLink && this.#isPrefetchable(target)) { + const link = target + const location = getLocationForLink(link) + + if (this.delegate.canPrefetchRequestToLocation(link, location)) { + const fetchRequest = new FetchRequest( + this, + FetchMethod.get, + location, + new URLSearchParams(), + target + ) + + prefetchCache.setLater(location.toString(), fetchRequest, this.#cacheTtl) + + link.addEventListener("mouseleave", () => prefetchCache.clear(), { once: true }) + } + } + } + + #tryToUsePrefetchedRequest = (event) => { + if (event.target.tagName !== "FORM" && event.detail.fetchOptions.method === "get") { + const cached = prefetchCache.get(event.detail.url.toString()) + + if (cached) { + // User clicked link, use cache response + event.detail.fetchRequest = cached + } + + prefetchCache.clear() + } + } + + prepareRequest(request) { + const link = request.target + + request.headers["Sec-Purpose"] = "prefetch" + + if (link.dataset.turboFrame && link.dataset.turboFrame !== "_top") { + request.headers["Turbo-Frame"] = link.dataset.turboFrame + } else if (link.dataset.turboFrame !== "_top") { + const turboFrame = link.closest("turbo-frame") + + if (turboFrame) { + request.headers["Turbo-Frame"] = turboFrame.id + } + } + + if (link.hasAttribute("data-turbo-stream")) { + request.acceptResponseType("text/vnd.turbo-stream.html") + } + } + + // Fetch request interface + + requestSucceededWithResponse() {} + + requestStarted(fetchRequest) {} + + requestErrored(fetchRequest) {} + + requestFinished(fetchRequest) {} + + requestPreventedHandlingResponse(fetchRequest, fetchResponse) {} + + requestFailedWithResponse(fetchRequest, fetchResponse) {} + + get #cacheTtl() { + return Number(getMetaContent("turbo-prefetch-cache-time")) || cacheTtl + } + + #isPrefetchable(link) { + const href = link.getAttribute("href") + + if (!href || href === "#" || link.dataset.turbo === "false" || link.dataset.turboPrefetch === "false") { + return false + } + + if (link.origin !== document.location.origin) { + return false + } + + if (!["http:", "https:"].includes(link.protocol)) { + return false + } + + if (link.pathname + link.search === document.location.pathname + document.location.search) { + return false + } + + if (link.dataset.turboMethod && link.dataset.turboMethod !== "get") { + return false + } + + if (targetsIframe(link)) { + return false + } + + if (link.pathname + link.search === document.location.pathname + document.location.search) { + return false + } + + const turboPrefetchParent = findClosestRecursively(link, "[data-turbo-prefetch]") + + if (turboPrefetchParent && turboPrefetchParent.dataset.turboPrefetch === "false") { + return false + } + + return true + } +} + +const targetsIframe = (link) => { + return !doesNotTargetIFrame(link) +} diff --git a/src/tests/fixtures/hover_to_prefetch.html b/src/tests/fixtures/hover_to_prefetch.html new file mode 100644 index 000000000..e0748fe0e --- /dev/null +++ b/src/tests/fixtures/hover_to_prefetch.html @@ -0,0 +1,45 @@ + + + + + Hover to Prefetch + + + + + + Hover to prefetch me + Hover to prefetch me + + Hover to prefetch me + + Hover to prefetch me + + + Won't prefetch when hovering me + Won't prefetch when hovering me + Won't prefetch when hovering me + Hover to prefetch me + Won't prefetch when hovering me + Hover to prefetch me + Won't prefetch when hovering me + Won't prefetch when hovering me + Won't prefetch when hovering me + Won't prefetch when hovering me + + + diff --git a/src/tests/fixtures/hover_to_prefetch_custom_cache_time.html b/src/tests/fixtures/hover_to_prefetch_custom_cache_time.html new file mode 100644 index 000000000..00ed35fd2 --- /dev/null +++ b/src/tests/fixtures/hover_to_prefetch_custom_cache_time.html @@ -0,0 +1,14 @@ + + + + + Hover to Prefetch + + + + + + + Hover to prefetch me + + diff --git a/src/tests/fixtures/hover_to_prefetch_disabled.html b/src/tests/fixtures/hover_to_prefetch_disabled.html new file mode 100644 index 000000000..689041d39 --- /dev/null +++ b/src/tests/fixtures/hover_to_prefetch_disabled.html @@ -0,0 +1,13 @@ + + + + + Hover to Prefetch + + + + + + Hover to prefetch me + + diff --git a/src/tests/fixtures/hover_to_prefetch_iframe.html b/src/tests/fixtures/hover_to_prefetch_iframe.html new file mode 100644 index 000000000..046fefbd8 --- /dev/null +++ b/src/tests/fixtures/hover_to_prefetch_iframe.html @@ -0,0 +1,13 @@ + + + + + Hover to Prefetch + + + + + + Hover to prefetch me + + diff --git a/src/tests/fixtures/hover_to_prefetch_without_meta_tag_with_link_to_with_meta_tag.html b/src/tests/fixtures/hover_to_prefetch_without_meta_tag_with_link_to_with_meta_tag.html new file mode 100644 index 000000000..75a7265aa --- /dev/null +++ b/src/tests/fixtures/hover_to_prefetch_without_meta_tag_with_link_to_with_meta_tag.html @@ -0,0 +1,19 @@ + + + + + Hover to Prefetch Not Enabled + + + + + + + Click to go to page with prefetch meta tag + + + diff --git a/src/tests/fixtures/prefetched.html b/src/tests/fixtures/prefetched.html new file mode 100644 index 000000000..492cd9bc8 --- /dev/null +++ b/src/tests/fixtures/prefetched.html @@ -0,0 +1,12 @@ + + + + + Prefetched Page + + + + + Prefetched Page Content + + diff --git a/src/tests/functional/link_prefetch_observer_tests.js b/src/tests/functional/link_prefetch_observer_tests.js new file mode 100644 index 000000000..1d0139986 --- /dev/null +++ b/src/tests/functional/link_prefetch_observer_tests.js @@ -0,0 +1,303 @@ +import { test } from "@playwright/test" +import { assert } from "chai" +import { nextBeat, sleep } from "../helpers/page" +import fs from "fs" +import path from "path" + +// eslint-disable-next-line no-undef +const fixturesDir = path.join(process.cwd(), "src", "tests", "fixtures") + +test.afterEach(() => { + fs.readdirSync(fixturesDir).forEach(file => { + if (file.startsWith("volatile_posts_database")) { + fs.unlinkSync(path.join(fixturesDir, file)) + } + }) +}) + +test("it prefetches the page", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) +}) + +test("it doesn't follow the link", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await hoverSelector({ page, selector: "#anchor_for_prefetch" }) + + assert.equal(await page.title(), "Hover to Prefetch") +}) + +test("prefetches the page when link has a whole valid url as a href", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_with_whole_url" }) +}) + +test("it prefetches the page when link has the same location but with a query string", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_for_same_location_with_query" }) +}) + +test("it doesn't prefetch the page when link is inside an element with data-turbo=false", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_turbo_false_parent" }) +}) + +test("it doesn't prefetch the page when link is inside an element with data-turbo-prefetch=false", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_turbo_prefetch_false_parent" }) +}) + +test("it does prefech the page when link is inside a container with data-turbo-prefetch=true, that is within an element with data-turbo-prefetch=false", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_with_turbo_prefetch_true_parent_within_turbo_prefetch_false_parent" }) +}) + +test("it doesn't prefetch the page when link has data-turbo-prefetch=false", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_turbo_prefetch_false" }) +}) + +test("it doesn't prefetch the page when link has data-turbo=false", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_turbo_false" }) +}) + +test("it doesn't prefetch the page when link has the same location", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_for_same_location" }) +}) + +test("it doesn't prefetch the page when link has a different origin", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_for_different_origin" }) +}) + +test("it doesn't prefetch the page when link has a hash as a href", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_hash" }) +}) + +test("it doesn't prefetch the page when link has a ftp protocol", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_ftp_protocol" }) +}) + +test("it doesn't prefetch the page when links is valid but it's inside an iframe", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_iframe_target" }) +}) + +test("it doesn't prefetch the page when link has a POST data-turbo-method", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_post_method" }) +}) + +test("it doesn't prefetch the page when turbo-prefetch meta tag is set to false", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch_disabled.html" }) + await assertNotPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) +}) + +test("it doesn't prefetch the page when turbo-prefetch meta tag is set to true, but is later set to false", async ({ + page +}) => { + await goTo({ page, path: "/hover_to_prefetch_custom_cache_time.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) + + await page.evaluate(() => { + const meta = document.querySelector('meta[name="turbo-prefetch"]') + meta.setAttribute("content", "false") + }) + + await sleep(10) + await page.mouse.move(0, 0) + + await assertNotPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) +}) + +test("it prefetches when visiting a page without the meta tag, then visiting a page with it", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch_without_meta_tag_with_link_to_with_meta_tag.html" }) + + await clickSelector({ page, selector: "#anchor_for_page_with_meta_tag" }) + + await assertPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) +}) + +test("it prefetches the page when turbo-prefetch-cache-time is set to 1", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch_custom_cache_time.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) +}) + +test("it caches the request for 1 millisecond when turbo-prefetch-cache-time is set to 1", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch_custom_cache_time.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) + + await sleep(10) + await page.mouse.move(0, 0) + + await assertPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) +}) + +test("it adds text/vnd.turbo-stream.html header to the Accept header when link has data-turbo-stream", async ({ + page +}) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_with_turbo_stream", callback: (request) => { + const headers = request.headers()["accept"].split(",").map((header) => header.trim()) + + assert.includeMembers(headers, ["text/vnd.turbo-stream.html", "text/html", "application/xhtml+xml"]) + }}) +}) + +test("it prefetches links with inner elements", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertPrefetchedOnHover({ page, selector: "#anchor_with_inner_elements" }) +}) + +test("it prefetches links with a delay", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + + let requestMade = false + page.on("request", async (request) => (requestMade = true)) + + await page.hover("#anchor_for_prefetch") + await sleep(75) + + assertRequestNotMade(requestMade) + + await sleep(100) + + assertRequestMade(requestMade) +}) + +test("it cancels the prefetch request if the link is no longer hovered", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + + let requestMade = false + page.on("request", async (request) => (requestMade = true)) + + await page.hover("#anchor_for_prefetch") + await sleep(75) + + assertRequestNotMade(requestMade) + + await page.mouse.move(0, 0) + + await sleep(100) + + assertRequestNotMade(requestMade) +}) + +test("it resets the cache when a link is hovered", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + + let requestCount = 0 + page.on("request", async () => (requestCount++)) + + await page.hover("#anchor_for_prefetch") + await sleep(200) + + assert.equal(requestCount, 1) + await page.mouse.move(0, 0) + + await page.hover("#anchor_for_prefetch") + await sleep(200) + + assert.equal(requestCount, 2) +}) + +test("it prefetches page on touchstart", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await assertPrefetchedOnTouchstart({ page, selector: "#anchor_for_prefetch" }) +}) + +test("it does not make a network request when clicking on a link that has been prefetched", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await hoverSelector({ page, selector: "#anchor_for_prefetch" }) + + await sleep(100) + + await assertNotPrefetchedOnHover({ page, selector: "#anchor_for_prefetch" }) +}) + +test("it follows the link using the cached response when clicking on a link that has been prefetched", async ({ + page +}) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + await hoverSelector({ page, selector: "#anchor_for_prefetch" }) + + await clickSelector({ page, selector: "#anchor_for_prefetch" }) + assert.equal(await page.title(), "Prefetched Page") +}) + +const assertPrefetchedOnTouchstart = async ({ page, selector, callback }) => { + let requestMade = false + + page.on("request", (request) => { + callback && callback(request) + requestMade = true + }) + + const selectorXY = await page.$eval(selector, (el) => { + const { x, y } = el.getBoundingClientRect() + return { x, y } + }) + + await page.touchscreen.tap(selectorXY.x, selectorXY.y) + + await sleep(100) + + assertRequestMade(requestMade) +} + +const assertPrefetchedOnHover = async ({ page, selector, callback }) => { + let requestMade = false + + page.on("request", (request) => { + callback && callback(request) + requestMade = true + }) + + await hoverSelector({ page, selector }) + + await sleep(100) + + assertRequestMade(requestMade) +} + +const assertNotPrefetchedOnHover = async ({ page, selector, callback }) => { + let requestMade = false + + page.on("request", (request) => { + callback && callback(request) + requestMade = true + }) + + await hoverSelector({ page, selector }) + + await sleep(100) + + assert.equal(requestMade, false, "Network request was made when it should not have been.") +} + +const assertRequestMade = (requestMade) => { + assert.equal(requestMade, true, "Network request wasn't made when it should have been.") +} + +const assertRequestNotMade = (requestMade) => { + assert.equal(requestMade, false, "Network request was made when it should not have been.") +} + +const goTo = async ({ page, path }) => { + await page.goto(`/src/tests/fixtures${path}`) + await nextBeat() +} + +const hoverSelector = async ({ page, selector }) => { + await page.hover(selector) + await nextBeat() +} + +const clickSelector = async ({ page, selector }) => { + await page.click(selector) + await nextBeat() +} diff --git a/src/util.js b/src/util.js index 772e9896c..dcb15c31b 100644 --- a/src/util.js +++ b/src/util.js @@ -1,3 +1,5 @@ +import { expandURL } from "./core/url" + export function activateScriptElement(element) { if (element.getAttribute("data-turbo-eval") == "false") { return element @@ -216,6 +218,24 @@ export async function around(callback, reader) { return [before, after] } +export function doesNotTargetIFrame(anchor) { + if (anchor.hasAttribute("target")) { + for (const element of document.getElementsByName(anchor.target)) { + if (element instanceof HTMLIFrameElement) return false + } + } + + return true +} + +export function findLinkFromClickTarget(target) { + return findClosestRecursively(target, "a[href]:not([target^=_]):not([download])") +} + +export function getLocationForLink(link) { + return expandURL(link.getAttribute("href") || "") +} + export function debounce(fn, delay) { let timeoutId = null From a7a727f382ea3ee240b58dbbd34ed03ef49d323d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Mon, 22 Jan 2024 12:55:34 +0000 Subject: [PATCH 11/16] Fix attribute name (#1134) The attribute name is `data-tag-name` not `data-page-name`. Ref. https://github.com/basecamp/trix/blob/06d8b1db5fb682d007c5ca041884f6297674c8b7/src/trix/core/helpers/custom_elements.js#L10-L11 --- src/core/drive/page_renderer.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/drive/page_renderer.js b/src/core/drive/page_renderer.js index 76072638f..7e6df4c4c 100644 --- a/src/core/drive/page_renderer.js +++ b/src/core/drive/page_renderer.js @@ -186,9 +186,9 @@ export class PageRenderer extends Renderer { 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-page-name` attribute. Long term we should moves those styles to Trix's CSS file + // `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-page-name")) + element.hasAttribute("data-tag-name")) }) } From 95c98d4b4be604e6b4849e8d3d262c811b3117a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Mon, 22 Jan 2024 12:57:45 +0000 Subject: [PATCH 12/16] Turbo 8.0.0-beta.3 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index afc8aded4..8254a9edb 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@hotwired/turbo", - "version": "8.0.0-beta.2", + "version": "8.0.0-beta.3", "description": "The speed of a single-page web application without having to write any JavaScript", "module": "dist/turbo.es2017-esm.js", "main": "dist/turbo.es2017-umd.js", From 5902f3bfd684686713a93b4d1653a724ba967c55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Wed, 24 Jan 2024 15:02:22 +0000 Subject: [PATCH 13/16] Ensure that the turbo-frame header is not sent when the turbo-frame has a target of _top (#1138) * Ensure that the turbo-frame header is not sent when the turbo-frame has a target of _top Given an HTML structure like this: ```html Hover to prefetch me ``` When the user hovers over the link, the turbo-frame header should not be sent in a prefetch request because the wrapping turbo-frame has a target of `_top`. If we don't respect this turbo-rails sees the turbo-frame header and renders a turbo-frame response with a minimal layout that doesn't include any assets in the head. When turbo processes the response it may find a missmatch in tracked assets and cause a reload. * Prefer `getAttribute` over `dataset` for `data-turbo-frame` attribute --- src/observers/link_prefetch_observer.js | 11 +++----- src/tests/fixtures/hover_to_prefetch.html | 8 ++++++ .../link_prefetch_observer_tests.js | 28 +++++++++++++++++-- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/observers/link_prefetch_observer.js b/src/observers/link_prefetch_observer.js index de08e05da..d204cb2a5 100644 --- a/src/observers/link_prefetch_observer.js +++ b/src/observers/link_prefetch_observer.js @@ -100,14 +100,11 @@ export class LinkPrefetchObserver { request.headers["Sec-Purpose"] = "prefetch" - if (link.dataset.turboFrame && link.dataset.turboFrame !== "_top") { - request.headers["Turbo-Frame"] = link.dataset.turboFrame - } else if (link.dataset.turboFrame !== "_top") { - const turboFrame = link.closest("turbo-frame") + const turboFrame = link.closest("turbo-frame") + const turboFrameTarget = link.getAttribute("data-turbo-frame") || turboFrame?.getAttribute("target") || turboFrame?.id - if (turboFrame) { - request.headers["Turbo-Frame"] = turboFrame.id - } + if (turboFrameTarget && turboFrameTarget !== "_top") { + request.headers["Turbo-Frame"] = turboFrameTarget } if (link.hasAttribute("data-turbo-stream")) { diff --git a/src/tests/fixtures/hover_to_prefetch.html b/src/tests/fixtures/hover_to_prefetch.html index e0748fe0e..89b94f1bb 100644 --- a/src/tests/fixtures/hover_to_prefetch.html +++ b/src/tests/fixtures/hover_to_prefetch.html @@ -41,5 +41,13 @@ Won't prefetch when hovering me + + + Hover to prefetch me + + + + Hover to prefetch me + diff --git a/src/tests/functional/link_prefetch_observer_tests.js b/src/tests/functional/link_prefetch_observer_tests.js index 1d0139986..584d673ef 100644 --- a/src/tests/functional/link_prefetch_observer_tests.js +++ b/src/tests/functional/link_prefetch_observer_tests.js @@ -153,6 +153,25 @@ test("it prefetches links with inner elements", async ({ page }) => { await assertPrefetchedOnHover({ page, selector: "#anchor_with_inner_elements" }) }) +test("it prefetches links inside a turbo frame", async ({ page }) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + + await assertPrefetchedOnHover({ page, selector: "#anchor_for_prefetch_in_frame", callback: (request) => { + const turboFrameHeader = request.headers()["turbo-frame"] + assert.equal(turboFrameHeader, "frame_for_prefetch") + }}) +}) + + +test("doesn't include a turbo-frame header when the link is inside a turbo frame with a target=_top", async ({ page}) => { + await goTo({ page, path: "/hover_to_prefetch.html" }) + + await assertPrefetchedOnHover({ page, selector: "#anchor_for_prefetch_in_frame_target_top", callback: (request) => { + const turboFrameHeader = request.headers()["turbo-frame"] + assert.equal(undefined, turboFrameHeader) + }}) +}) + test("it prefetches links with a delay", async ({ page }) => { await goTo({ page, path: "/hover_to_prefetch.html" }) @@ -253,15 +272,18 @@ const assertPrefetchedOnHover = async ({ page, selector, callback }) => { let requestMade = false page.on("request", (request) => { - callback && callback(request) - requestMade = true + requestMade = request }) await hoverSelector({ page, selector }) await sleep(100) - assertRequestMade(requestMade) + if (callback) { + await callback(requestMade) + } + + assertRequestMade(!!requestMade) } const assertNotPrefetchedOnHover = async ({ page, selector, callback }) => { From 814099c14cb61160bec4c9473036bd17787c5e17 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 14/16] 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") From 617e6d03eca143942756d8fb6e1b8586c18672a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Fern=C3=A1ndez-Capel?= Date: Thu, 25 Jan 2024 14:07:58 +0000 Subject: [PATCH 15/16] Turbo 8.0.0-beta.4 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 8254a9edb..1db81757f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@hotwired/turbo", - "version": "8.0.0-beta.3", + "version": "8.0.0-beta.4", "description": "The speed of a single-page web application without having to write any JavaScript", "module": "dist/turbo.es2017-esm.js", "main": "dist/turbo.es2017-umd.js", From abab1cf0e000c333457e764e556e0301d7f95f2f Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Sat, 2 Dec 2023 19:07:35 -0500 Subject: [PATCH 16/16] Introduce `turbo:{before-,}morph-{element,attribute}` events Follow-up to [9944490][] Related to [#1083] Related to [@hotwired/turbo-rails#533][] The problem --- Some client-side plugins are losing their state when elements are morphed. Without resorting to `MutationObserver` instances to determine when a node is morphed, uses of those plugins don't have the ability to prevent (without `[data-turbo-permanent]`) or respond to the morphing. The proposal --- This commit introduces a `turbo:before-morph-element` event that'll dispatch as part of the Idiomorph `beforeNodeMorphed` callback. It'll give interested parties access to the nodes before and after a morph. If that event is cancelled via `event.preventDefault()`, it'll skip the morph as if the element were marked with `[data-turbo-permanent]`. Along with `turbo:before-morph-element`, this commit also introduces a `turbo:before-morph-attribute` to correspond to the `beforeAttributeUpdated` callback that Idiomorph provides. When listeners (like an `HTMLDetailsElement`, an `HTMLDialogElement`, or a Stimulus controller) want to preserve the state of an attribute, they can cancel the `turbo:before-morph-attribute` event that corresponds with the attribute name (through `event.detail.attributeName`). Similarly, this commit adds a new `turbo:morph-element` event to be dispatched for every morphed node (via Idiomorph's `afterNodeMorphed` callback). The original implementation dispatched the event for the `` element as part of `MorphRenderer`'s lifecycle. That event will still be dispatched, since `` is the first element the callback will fire for. In addition to that event, each individual morphed node will dispatch one. This commit re-introduced test coverage for a Stimulus controller to demonstrate how an interested party might respond. It isn't immediately clear with that code should live, but once we iron out the details, it could be part of a `@hotwired/turbo/stimulus` package, or a `@hotwired/stimulus/turbo` package that users (or `@hotwired/turbo-rails`) could opt-into. [9944490]: https://github.com/hotwired/turbo/pull/1019/commits/9944490a3c8aec0c5060401125cc8932e93a32df [#1083]: https://github.com/hotwired/turbo/issues/1083 [@hotwired/turbo-rails#533]: https://github.com/hotwired/turbo-rails/issues/533 --- src/core/drive/morph_renderer.js | 37 ++++++++++++-- src/core/view.js | 2 +- src/tests/fixtures/page_refresh.html | 46 ++++++++++++++++- src/tests/fixtures/test.js | 4 ++ src/tests/functional/page_refresh_tests.js | 59 +++++++++++++++++++++- src/tests/functional/rendering_tests.js | 2 +- 6 files changed, 141 insertions(+), 9 deletions(-) diff --git a/src/core/drive/morph_renderer.js b/src/core/drive/morph_renderer.js index beb94bfb1..87401ee23 100644 --- a/src/core/drive/morph_renderer.js +++ b/src/core/drive/morph_renderer.js @@ -33,7 +33,9 @@ export class MorphRenderer extends Renderer { callbacks: { beforeNodeAdded: this.#shouldAddElement, beforeNodeMorphed: this.#shouldMorphElement, - beforeNodeRemoved: this.#shouldRemoveElement + beforeAttributeUpdated: this.#shouldUpdateAttribute, + beforeNodeRemoved: this.#shouldRemoveElement, + afterNodeMorphed: this.#didMorphElement } }) } @@ -44,9 +46,36 @@ export class MorphRenderer extends Renderer { #shouldMorphElement = (oldNode, newNode) => { if (oldNode instanceof HTMLElement) { - return !oldNode.hasAttribute("data-turbo-permanent") && (this.isMorphingTurboFrame || !this.#isFrameReloadedWithMorph(oldNode)) - } else { - return true + if (!oldNode.hasAttribute("data-turbo-permanent") && (this.isMorphingTurboFrame || !this.#isFrameReloadedWithMorph(oldNode))) { + const event = dispatch("turbo:before-morph-element", { + cancelable: true, + target: oldNode, + detail: { + newElement: newNode + } + }) + + return !event.defaultPrevented + } else { + return false + } + } + } + + #shouldUpdateAttribute = (attributeName, target, mutationType) => { + const event = dispatch("turbo:before-morph-attribute", { cancelable: true, target, detail: { attributeName, mutationType } }) + + return !event.defaultPrevented + } + + #didMorphElement = (oldNode, newNode) => { + if (newNode instanceof HTMLElement) { + dispatch("turbo:morph-element", { + target: oldNode, + detail: { + newElement: newNode + } + }) } } diff --git a/src/core/view.js b/src/core/view.js index 8c5ae2e82..4a0c0d7fd 100644 --- a/src/core/view.js +++ b/src/core/view.js @@ -64,7 +64,7 @@ export class View { await this.prepareToRenderSnapshot(renderer) const renderInterception = new Promise((resolve) => (this.#resolveInterceptionPromise = resolve)) - const options = { resume: this.#resolveInterceptionPromise, render: this.renderer.renderElement } + const options = { resume: this.#resolveInterceptionPromise, render: this.renderer.renderElement, renderMethod: this.renderer.renderMethod } const immediateRender = this.delegate.allowsImmediateRender(snapshot, options) if (!immediateRender) await renderInterception diff --git a/src/tests/fixtures/page_refresh.html b/src/tests/fixtures/page_refresh.html index d9523c6b1..f7f577d56 100644 --- a/src/tests/fixtures/page_refresh.html +++ b/src/tests/fixtures/page_refresh.html @@ -9,6 +9,45 @@ +