Fix issue #604#793
Closed
manuelpuyol wants to merge 2 commits intohotwired:mainfrom
manuelpuyol:issue-604
Closed
Conversation
Member
|
Cc @seanpdoyle |
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 21, 2022
The issues outlined by [hotwired#793][] are resolved by the introduction of the `FrameVisit` object and its lifecycle. To ensure that behavior is fixed, this commit cherry-picks the test coverage introduced in that branch. [hotwired#793]: hotwired#793
Contributor
|
@manuelpuyol I've cherry-picked the test coverage you've introduced in this PR to #430, and the test suite passes. That pull request introduces the concept of a Having said that, I've applied the following diff to diff --git a/src/tests/functional/frame_navigation_tests.ts b/src/tests/functional/frame_navigation_tests.ts
index 3a2fadd..ac6eb03 100644
--- a/src/tests/functional/frame_navigation_tests.ts
+++ b/src/tests/functional/frame_navigation_tests.ts
@@ -1,5 +1,12 @@
import { test } from "@playwright/test"
-import { getFromLocalStorage, nextEventNamed, nextEventOnTarget, pathname, scrollToSelector } from "../helpers/page"
+import {
+ getFromLocalStorage,
+ nextBeat,
+ nextEventNamed,
+ nextEventOnTarget,
+ pathname,
+ scrollToSelector,
+} from "../helpers/page"
import { assert } from "chai"
test("test frame navigation with descendant link", async ({ page }) => {
@@ -97,3 +104,38 @@ test("test promoted frame navigations are cached", async ({ page }) => {
assert.equal(await page.getAttribute("#tab-frame", "src"), null, "caches one.html without #tab-frame[src]")
assert.equal(await page.getAttribute("#tab-frame", "complete"), null, "caches one.html without [complete]")
})
+
+test("test canceling frame requests don't mutate the history", async ({ page }) => {
+ await page.goto("/src/tests/fixtures/tabs.html")
+
+ await page.click("#tab-2")
+
+ await nextEventOnTarget(page, "tab-frame", "turbo:frame-load")
+ await nextEventNamed(page, "turbo:load")
+
+ assert.equal(await page.textContent("#tab-content"), "Two")
+ assert.equal(pathname((await page.getAttribute("#tab-frame", "src")) || ""), "/src/tests/fixtures/tabs/two.html")
+ assert.equal(await page.getAttribute("#tab-frame", "complete"), "", "sets [complete]")
+
+ // This request will be canceled
+ page.click("#tab-1")
+ await page.click("#tab-3")
+
+ await nextEventOnTarget(page, "tab-frame", "turbo:frame-load")
+ await nextEventNamed(page, "turbo:load")
+
+ assert.equal(await page.textContent("#tab-content"), "Three")
+ assert.equal(pathname((await page.getAttribute("#tab-frame", "src")) || ""), "/src/tests/fixtures/tabs/three.html")
+
+ await page.goBack()
+ await nextEventNamed(page, "turbo:load")
+
+ assert.equal(await page.textContent("#tab-content"), "Two")
+ assert.equal(pathname((await page.getAttribute("#tab-frame", "src")) || ""), "/src/tests/fixtures/tabs/two.html")
+
+ // Make sure the frame is not mutated after some time.
+ await nextBeat()
+
+ assert.equal(await page.textContent("#tab-content"), "Two")
+ assert.equal(pathname((await page.getAttribute("#tab-frame", "src")) || ""), "/src/tests/fixtures/tabs/two.html")
+})Were you able to force the tests to fail? |
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Nov 27, 2022
The issues outlined by [hotwired#793][] are resolved by the introduction of the `FrameVisit` object and its lifecycle. To ensure that behavior is fixed, this commit cherry-picks the test coverage introduced in that branch. [hotwired#793]: hotwired#793
Contributor
Author
|
@seanpdoyle hmm maybe the test is not ideal, since I still can see the bug in main Screen.Recording.2022-11-30.at.12.59.42.PM.mov |
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Dec 18, 2022
The issues outlined by [hotwired#793][] are resolved by the introduction of the `FrameVisit` object and its lifecycle. To ensure that behavior is fixed, this commit cherry-picks the test coverage introduced in that branch. [hotwired#793]: hotwired#793
seanpdoyle
added a commit
to seanpdoyle/turbo
that referenced
this pull request
Dec 18, 2022
The issues outlined by [hotwired#793][] are resolved by the introduction of the `FrameVisit` object and its lifecycle. To ensure that behavior is fixed, this commit cherry-picks the test coverage introduced in that branch. [hotwired#793]: hotwired#793
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #604
When quickly clicking between frames, even though the fetch request is canceled, the page's HTML is mutated, setting a new
srcto the frame element. This change will cause thepageSnapshotto be incorrect, so when we navigate back, theturbo-framewill have an incorrectsrccausing a new fetch and the bug shown in the issue.This PR updates frames to know what was their
previousSrcso they can revert back in case the request is canceled.