Support back button (restore) even if page loaded in frame does not contain matching HEAD

[tpaulshippy]

Purpose

Resolves #1241

Approach

Set the location on the session even if the PageView does not render because the trackedElementsAreIdentical is false.

Test

This test illustrates the issue, once you remove line 6 from three.html npx playwright test -g "promoted frame navigations are cached"

[tleish]

Would a better option for testing this would be the src/tests/functional/frame_navigation_tests.js?

Something like: https://github.com/hotwired/turbo/compare/main...tleish:turbo:turbo-frame-back-navigation?expand=1

[tpaulshippy]

Would a better option for testing this would be the src/tests/functional/frame_navigation_tests.js?

I think we both tested with that same file. It looks like you added goBack testing to frame navigation with data-turbo-action whereas I removed the <script> tag from three.html and tested with promoted frame navigations are cached.

I think I do like your testing approach a bit better. Will try to switch to it when I get a chance. The only piece I didn't quite understand is where you removed lines 114-117. Maybe you could help me understand that change?

Have you found a better way to resolve the issue?

[tleish]

The only piece I didn't quite understand is where you removed lines 114-117. Maybe you could help me understand that change?

Ignore those changes,I must’ve cut-paste instead of copy-paste.

Have you found a better way to resolve the issue?

The test is as far as I got.

[tpaulshippy]

Confirmed that your testing approach accurately reproduced the issue and was resolved by the proposed implementation. Pushed. Thanks @tleish Looking forward to seeing any alternative implementation ideas. I'm not convinced this is the best possible way to resolve the issue.

[tleish]

I believe the fix is a bit simpler. It appears with a turbo navigation that includes an action, turbo does the following:

1. Calls FrameController#loadFrameResponse where it loads and updates the frame on page
2. During this method, it calls await this.fetchResponseLoaded(fetchResponse); which is defined inside of FrameController#proposeVisitIfNavigatedWithAction
3. frame.delegate.fetchResponseLoaded defined within FrameController#proposeVisitIfNavigatedWithActionsimulates a "Turbo#visit" (last line) wherein the PageSnapshot is updated with the HTML from the mocked response object. Instead of setting the HTML to be the frame html, we could set it to be the current/updated page HTML (which has the frame contents loaded at this point). Doing this means that the PageSnapshot.headSnapshot.trackedElementSignature will always match when the logic compares the history for back navigation.

see: https://github.com/hotwired/turbo/compare/main...tleish:turbo:turbo-frame-back-navigation?expand=1

[tpaulshippy]

That's a revert of part of https://github.com/hotwired/turbo/commit/ed72ba16bc23dcccd88e0ab76ba72cfef82cd599 I guess I wonder why that change was made. There's a bit of an explanation there but it sounds more like "this is why this works" rather than "this is why we need this." @seanpdoyle maybe you can help?

[tpaulshippy]

I'm happy to switch to that implementation in this PR if you think it's a better approach.

[tleish]

That's a revert of part of ed72ba1

Hah! You mentioned this before on the issue thread. I didn't look at it, but you are right. It's a revert of this commit.

[tpaulshippy]

Looks like this was discussed here: https://github.com/hotwired/turbo/pull/887#discussion_r1376602627