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.

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

FIX: Turbo stream link setting the src of a turbo frame container #968

Merged
merged 1 commit into from Sep 13, 2023

Conversation

BakiVernes
Copy link
Contributor

@BakiVernes BakiVernes commented Sep 5, 2023

Bug Overview

This PR addresses a specific issue that arises when using a Turbo Stream link within a Turbo Frame in a Rails 7 application. Clicking on such a link currently sets the src attribute of the Turbo Frame element. Although Turbo saves this state for browser history navigation, it fails to fetch the content correctly. This is because the src attribute is intended to be used for Turbo Stream responses, leading to unexpected behavior.

Steps to Reproduce

  1. Create a new Rails 7 application.
  2. Add a Turbo Stream link inside a Turbo Frame.
  3. Click the Turbo Stream link.
  4. Observe that the src attribute of the Turbo Frame element is set.
  5. Navigate the browser history.
  6. Turbo fails to fetch the content.

Visual Reference

Illustrative example in a new Rails 7 application

Proposed Solution

This PR adds a conditional check for the data-turbo-stream attribute. If the attribute exists, the src attribute of the Turbo Frame element will not be modified, thus preserving the expected behavior.

By implementing this check, the PR aims to make the behavior of Turbo Stream links within Turbo Frames more predictable and consistent with user expectations.

This was referenced Sep 8, 2023
@BakiVernes BakiVernes changed the title Fix turbo stream link setting the src of a turbo frame container FIX: Turbo stream link setting the src of a turbo frame container Sep 11, 2023
@BakiVernes
Copy link
Contributor Author

I have rebased this PR after the Typescript changes and would love some feedback. This bug has been a pain in our production app for some time now.

@BakiVernes BakiVernes force-pushed the turbo_stream_history branch 2 times, most recently from ac9d24a to 179b603 Compare September 12, 2023 15:09
@BakiVernes
Copy link
Contributor Author

Might need a rerun on this. My tests are green.

Copy link
Collaborator

@afcapel afcapel left a comment

Choose a reason for hiding this comment

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

I think the test failures are legit, I can reproduce them locally too.

See my comment about the conditional in willFollowLinkToLocation for a possible solution.

src/observers/form_link_click_observer.js Outdated Show resolved Hide resolved
@BakiVernes
Copy link
Contributor Author

@afcapel I have made the suggested changes, good catch.

What I am unsure about is the tests. I checked out the main branch and the tests are failing locally for me. Checked out the last couple of commits. The latest commit on which all test are green is 4c006dc

@BakiVernes
Copy link
Contributor Author

@afcapel I have made the suggested changes, good catch.

What I am unsure about is the tests. I checked out the main branch and the tests are failing locally for me. Checked out the last couple of commits. The latest commit on which all test are green is 4c006dc

Nevermind 😅, the tests passed 🤷‍♀️

Copy link
Collaborator

@afcapel afcapel left a comment

Choose a reason for hiding this comment

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

Yes, all good. Thanks @BakiVernes @shiftyp 🙏

@afcapel afcapel merged commit 750f649 into hotwired:main Sep 13, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants