Don't include Turbo-Frame header in promoted frame navigations

[afcapel]

This PR changes the behavior of frame navigations promoted with the [data-turbo-action] attribute so that they don't include the Turbo-Frame header in their requests. This mimics the existing behavior of frame navigations promoted with [data-turbo-target=_top].

If we don't do this turbo-rails will see the Turbo-Frame header and render the response as a frame update, with a minimal layout that doesn't include any assets. When Turbo receives the response it believes it's a full page response in which the assets have gone stale and it issues a full page reload with tracked_element_mismatch as the reason.

Fixes #1047

This is similar to #1138

//cc @seanpdoyle @domchristie

[domchristie]

I think this still triggers a tracked_element_mismatch reload when testing with https://gist.github.com/domchristie/d143a6e7ae1807bd1f8323ecc8a4978b (and building Turbo from this branch).

This mimics the existing behavior of frame navigations promoted with [data-turbo-target=_top].

I was under the impression that a frame visit with an action should only replace a part of the page and update the history (not target the whole page). Am I misunderstanding?

[afcapel]

I was under the impression that a frame visit with an action should only replace a part of the page and update the history (not target the whole page). Am I misunderstanding?

Oh, you're right. Not sure if that's a big issue, as far as I know we only use the Turbo-Frame header to skip the full layout rendering. But it'd be better if we could keep the header more semantic.

[kevinmcconnell]

Possibly I'm misunderstanding, but I think the current behaviour was correct. As @domchristie says, these are frame visits that are configured to also update the history. So it's correct that these requests send the Turbo-Frame header in order to fetch a frame-optimised response where that's supported.

The bug you describe in the description sounds more like an error in treating a frame response as if it was a full page response. Turbo should know that the frame request is not responsible for populating the assets in head, and so a difference in those assets is not a tracked_element_mismatch. Or in other words, the response was correct, but the way Turbo handled it was wrong.

In which case, removing the Turbo-Frame header from this type of frame request would introduce a semantic inconsistency.

Do I have that right?

[domchristie]

The bug … sounds more like an error in treating a frame response as if it was a full page response.

If I'm understanding the behaviour correctly, Turbo processes the frame response as expected, but then performs another visit with willRender: false, updateHistory: false (

turbo/src/core/frames/frame_controller.js

Line 373 in b7187f1

session.visit(frame.src, options)

). This then triggers the tracked_element_mismatch

Turbo should know that the frame request is not responsible for populating the assets in head

Am I correct in thinking that frame requests can populate content in head e.g. hotwired/turbo-rails#428?

[kevinmcconnell]

@domchristie yes, exactly. It's when performing the visit that Turbo incorrectly treats the frame response as a full page response with regard to the asset version checking. It's a step that shouldn't happen for frame responses, even those with actions. Avoiding that would solve this bug.

Am I correct in thinking that frame requests can populate content in head

Frame requests shouldn't be able to render anything outside of their target frame, as I understand it. The responses can return content in head because there are cases where Turbo will make use of that in determining what to do with the response (things like triggering a reload via a meta tag). But that content isn't rendered onto the page, due to it being outside of the target frame.

[afcapel]

I found a better fix in #1144