Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lift the frame morphing logic up to FrameController.reload #1192

Merged
merged 25 commits into from
Sep 11, 2024
+65 −18
Merged

Lift the frame morphing logic up to FrameController.reload #1192

Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c03c893
Add new frame morphing refresh tests which fail
krschacht Feb 24, 2024
047a479
Get tests passing
krschacht Feb 25, 2024
e1fde68
Refactor code to remove duplication
krschacht Feb 25, 2024
5427b77
Remove comment
krschacht Feb 25, 2024
9fd1e9d
remove logging
krschacht Feb 25, 2024
a5a87e4
Remove testing server randNum hack
krschacht Feb 27, 2024
87c9553
Refactor to extract method & revert to class
krschacht Feb 29, 2024
07c3577
Refactor morphElements class
krschacht Mar 5, 2024
699f66f
Merge branch 'main' into frame-reload-uses-morphing
krschacht Aug 22, 2024
6abb120
wip
krschacht Aug 22, 2024
38e4d2c
Remove extraction added in another PR
krschacht Aug 22, 2024
8344e4d
Update reference to use new MorphingFrameRenderer
krschacht Aug 22, 2024
a342115
Reword test names
krschacht Aug 22, 2024
1ed1dc1
fixed typo
krschacht Aug 23, 2024
f738bdb
Only morph frame when using reload()
krschacht Aug 28, 2024
e956dd4
wip
krschacht Aug 28, 2024
fceb2cb
Fix a broken test
krschacht Aug 28, 2024
3af219d
Rewrite test fix to use playwright API
krschacht Aug 30, 2024
a0bf9a3
Fix unused reference
krschacht Aug 30, 2024
f45697c
Flip failing test to reflect product decision
krschacht Aug 31, 2024
55266c7
Merge branch 'main' into frame-reload-uses-morphing
krschacht Aug 31, 2024
8f0a949
Merge branch 'main' into frame-reload-uses-morphing
krschacht Aug 31, 2024
6801493
Revert "Flip failing test to reflect product decision"
krschacht Sep 3, 2024
795ab37
Correct logic for frame reloading
krschacht Sep 3, 2024
61bd384
revert unintended files
krschacht Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/tests/functional/page_refresh_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ test("frames marked with refresh='morph' are excluded from full page morphing",
await expect(page.locator("#refresh-morph")).toHaveText("Loaded morphed frame")
})

test("navigated frames without refresh attribute are reset after morphing", async ({ page }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry if I explained wrong. Turbo frames that aren't reloaded by morphing have to be reset correctly. This test should be fine.

Turbo frames that don't have the conditions to be reloaded with morphing are affected in the first round of morphing, therefore, they are morphed according to the changes coming from the server. Turbo frames that can be reloaded with morphing due to the presence of the src attribute and refresh="morph" are ignored in the first round, and then refreshed using morphing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just about to send a long final comment saying I finished. :) Happy to do this either way. This test is a bit confusing so it took me a bit to fully make sense of. In your longer explanation, you said what my expectation would be. Specifically:

This is because we are interested in preserving the URL where the Turbo frame is located if the user has navigated through it. Otherwise, each page refresh would reset the Turbo frame to the initial state of the page, and that isn't what we want.

This test was enforcing the opposite, so I flipped it to reflect what you said here. ^ And it fits my intuition that if a user navigations a turbo frame, and it's implicitly reload="refresh" (not morph) then I would expect that navigation to stay. But you think we should revert it back since it started with no src when the page loaded?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct, but only for Turbo frames marked with refresh="morph" attribute. Sorry if I wasn't clear enough.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that the goal of the test as it was before was correct. Turbo frames that don't contain the refresh="morph" attribute are reset. We only retain the state of Turbo frames that are reloaded with morphing. The rest are morphed in the first round according to what the server returns in the request made at the time of the page refresh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test has been reverted (so that it fails again) and the logic has been corrected (so now the test passes).

I think that's it and things are now ready. 🤞

test("navigated frames without refresh attribute are not reset after morphing", async ({ page }) => {
await page.goto("/src/tests/fixtures/page_refresh.html")

await page.click("#refresh-after-navigation-link")
Expand All @@ -216,17 +216,20 @@ test("navigated frames without refresh attribute are reset after morphing", asyn
"navigates theframe"
)

//await new Promise(resolve => setTimeout(resolve, 300000))

await page.click("#form-submit")

await nextEventNamed(page, "turbo:render", { renderMethod: "morph" })
await nextBeat()

assert.ok(

assert.notOk(
await hasSelector(page, "#refresh-after-navigation-link"),
"resets the frame"
)

assert.notOk(
assert.ok(
await hasSelector(page, "#refresh-after-navigation-content"),
"does not reload the frame"
)
Expand Down
Loading