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

data-turbo-action="advance" doesn't update browser URL #489

Closed
wonderix opened this issue Dec 2, 2021 · 17 comments · Fixed by #1135
Closed

data-turbo-action="advance" doesn't update browser URL #489

wonderix opened this issue Dec 2, 2021 · 17 comments · Fixed by #1135

Comments

@wonderix
Copy link

wonderix commented Dec 2, 2021

The documentation says that the browser URL will be updated if data-turbo-action="advance" is set on a link or a turbo frame.
Unfortunately, only the src attribute of the turbo frame is updated.
When using Turbo.visit(), the src attribute and the browser url are updated.

I'm using version 7.0.1.

Is this related to #468?

@dhh
Copy link
Member

dhh commented Dec 2, 2021

That feature was included in 7.1.0: https://github.com/hotwired/turbo/releases/tag/v7.1.0

@dhh dhh closed this as completed Dec 2, 2021
@luglio7
Copy link

luglio7 commented Jan 31, 2022

when i manually update the src attribute of a frame with "data-turbo-action" set to "advance" the url doesn't change. I'm missing something ? When i click a link inside a frame everything is working as expected.

@leoplct
Copy link

leoplct commented Jul 7, 2022

Solved changing package.json to "@hotwired/turbo-rails": "^7.1.0" and run yarn upgrade turbo

@walterhorstman
Copy link

Are we sure this is working? I'm creating a fresh Rails 7 application, have included stimulus-rails and turbo-rails in Gemfile and package.json contains "@hotwired/turbo-rails": "latest" under "dependencies". yarn.lock indicates I'm using 7.1.3 of @hotwired/turbo.

My view contains <turbo-frame data-turbo-action="advance" id="header" target="main"><a href="..."></a></turbo-frame> and further a <turbo-frame id="main"></turbo-frame>. I would expect the page's URL to change when clicking the link.

@dmitrue
Copy link

dmitrue commented Mar 20, 2023

Same for me, I'm using "@hotwired/turbo-rails": "^7.3.0", and Turbo.visit('/...', { frame: 'results', action: 'advance' }) doesn't update URL of the page

@cmalven
Copy link

cmalven commented Mar 27, 2023

May or not be related, but I'm experiencing a similar issue even without manually setting action: 'advance':

    Turbo.visit(url, {
      frame: 'my-frame',
    });

Updates the src attribute on the frame but does not update the URL. "@hotwired/turbo": "^7.3.0"

@perlun
Copy link

perlun commented Apr 11, 2023

For me, with Turbo 7.1.0, I see that it's working when navigating to other content within a turbo frame. When trying to combine it with target="content" (similar to in your example @walterhorstman), it refuses to update the URL. 🤔

I'll try updating to 7.3.0 to see if I can reproduce it there as well.

Update: the problem persists with 7.3.0. Also, the "working" part above seems irregular to me:

  1. If you first navigate using a target="foo" link, the history/URL is not updated.
  2. If you then navigate "inside" a frame, it still won't update the history.

However, if you do like this:

  1. Reload the browser window.
  2. Navigate inside a frame

...it works as intended for me; history and URL is updated as it should be.

@perlun
Copy link

perlun commented Apr 11, 2023

@seanpdoyle - this is of course a bit premature, but it feels like this could be a limitation in the initial implementation in #398. 🤔 Would you like an isolated test case or could you give it a look regardless?

To make things reasonably clear, we're talking about scenarios like this:

<body>
  <turbo-frame id="nav" target="content" data-turbo-action="advance">
    <!-- Links inside this frame are broken; the data-turbo-action properly is not properly honored -->
    <a href="/link-one">Link one</a>
    <a href="/link-two">Link two</a>
  </turbo-frame>

  <turbo-frame id="content" data-turbo-action="advance">
    <!-- Links inside this frame works correctly re. URL/history, with the limitations described in
         earlier comment -->
    <a href="/link-three">Link three</a>
  </turbo-frame>
</body>

@bopm
Copy link

bopm commented Apr 21, 2023

After debugging it, I am seeing that this.action here is null no matter Turbo.visit configuration.

@bopm
Copy link

bopm commented Apr 21, 2023

I suspect this issue was introduced here, as before this change options were taken into account, but not any longer after that change. And Turbo.visit lands here.
cc @seanpdoyle

@louiswdavis
Copy link

louiswdavis commented May 24, 2023

As a workaround, I added a controller to my rendered html that when connected passes values for the page name and path I want to set

 <div data-controller="url" data-url-page-value="Page Name" data-url-path-value="/appended_path"></div>
export default class extends Controller {
  static values = {
    page: String,
    path: String,
  }

  connect() {
    window.history.replaceState(window.history.state, this.pageValue, this.pathValue)
  }
}

@jtreitz
Copy link

jtreitz commented Jun 8, 2023

I'd expect the following to update my URL but it doesn't :(
Turbo.visit(url, { frame: 'a', action: 'advance' })

@bopm
Copy link

bopm commented Jun 9, 2023

@dhh @seanpdoyle this issue is regressed since 7.3.0, can we get this issue back open?

@dhh dhh reopened this Jun 18, 2023
@leonvogt
Copy link

I'd expect the following to update my URL but it doesn't :(
Turbo.visit(url, { frame: 'a', action: 'advance' })

Same. Would be really nice if that worked.

@jasonswett
Copy link

I had this issue and upgrading to Rails 7.1.3 and Turbo 1.5 seemed to fix it for me. (I don't know which upgrade was responsible for the fix.)

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 22, 2024
Closes [hotwired#489][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on hotwired#489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

[hotwired#489]: hotwired#489
[comment on hotwired#489]: hotwired#489 (comment)
@seanpdoyle
Copy link
Contributor

May or not be related, but I'm experiencing a similar issue even without manually setting action: 'advance':

    Turbo.visit(url, {
      frame: 'my-frame',
    });

Updates the src attribute on the frame but does not update the URL. "@hotwired/turbo": "^7.3.0"

I've opened #1135 to resolve this issue.

@seanpdoyle
Copy link
Contributor

@seanpdoyle - this is of course a bit premature, but it feels like this could be a limitation in the initial implementation in #398. 🤔 Would you like an isolated test case or could you give it a look regardless?

To make things reasonably clear, we're talking about scenarios like this:

<body>
  <turbo-frame id="nav" target="content" data-turbo-action="advance">
    <!-- Links inside this frame are broken; the data-turbo-action properly is not properly honored -->
    <a href="/link-one">Link one</a>
    <a href="/link-two">Link two</a>
  </turbo-frame>

  <turbo-frame id="content" data-turbo-action="advance">
    <!-- Links inside this frame works correctly re. URL/history, with the limitations described in
         earlier comment -->
    <a href="/link-three">Link three</a>
  </turbo-frame>
</body>

I've added test coverage to #1135 to attempt to resolve this issue. Unfortunately, I was able to pass the test without any implementation changes. Could you review that diff (mainly https://github.com/hotwired/turbo/pull/1135/files#diff-1e84a1526802b27615a24efc0f9b75c27b2a9197d855a8ccc5475db14609ed6f and https://github.com/hotwired/turbo/pull/1135/files#diff-4706705ec52e666c36d3cd9410d3aad96354c69f61a18f880b40291836485f36R655-R663 to help me reproduce the underlying issue more closely?

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 22, 2024
Closes [hotwired#489][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on hotwired#489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

To support the implementation changes necessary to pass the tests, this
commit makes one change to the public interface, and another to an
internal interface.

* `FrameElement.getElementById(id: string)` - unify the process to
  internally resolve a `<turbo-frame>` element by its `[id]`. Update
  call sites to use this new method

* `FrameController.proposeVisitIfNavigatedWithAction(frame, elements =
  [], options = {})` - flatten the variable arguments into a single
  `Array`, then extend the interface to support Visit options like `{
  action: }`

[hotwired#489]: hotwired#489
[comment on hotwired#489]: hotwired#489 (comment)
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 22, 2024
Closes [hotwired#489][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on hotwired#489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

To support the implementation changes necessary to pass the tests, this
commit makes one change to the public interface, and another to an
internal interface.

* `FrameElement.getElementById(id: string)` - unify the process to
  internally resolve a `<turbo-frame>` element by its `[id]`. Update
  call sites to use this new method

* `FrameController.proposeVisitIfNavigatedWithAction(frame, elements =
  [], options = {})` - flatten the variable arguments into a single
  `Array`, then extend the interface to support Visit options like `{
  action: }`

[hotwired#489]: hotwired#489
[comment on hotwired#489]: hotwired#489 (comment)
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 22, 2024
Closes [hotwired#489][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on hotwired#489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

To support the implementation changes necessary to pass the tests, this
commit makes one change to the public interface, and another to an
internal interface.

* `FrameElement.getElementById(id: string)` - unify the process to
  internally resolve a `<turbo-frame>` element by its `[id]`. Update
  call sites to use this new method

* `FrameController.proposeVisitIfNavigatedWithAction(frame, elements =
  [], options = {})` - flatten the variable arguments into a single
  `Array`, then extend the interface to support Visit options like `{
  action: }`

[hotwired#489]: hotwired#489
[comment on hotwired#489]: hotwired#489 (comment)
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 22, 2024
Closes [hotwired#489][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on hotwired#489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

To support the implementation changes necessary to pass the tests, this
commit makes one change to the public interface, and another to an
internal interface.

* `FrameElement.getElementById(id: string)` - unify the process to
  internally resolve a `<turbo-frame>` element by its `[id]`. Update
  call sites to use this new method

* `FrameController.proposeVisitIfNavigatedWithAction(frame, action)` -
  flatten the variable arguments into a single `action` argument. Move
  the `getVisitAction` call to the call sites that require it, and pass in
  the `action` directly when its available

[hotwired#489]: hotwired#489
[comment on hotwired#489]: hotwired#489 (comment)
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 22, 2024
Closes [hotwired#489][]
Closes [hotwired#468][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on hotwired#489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

To support the implementation changes necessary to pass the tests, this
commit makes one change to the public interface, and another to an
internal interface.

* `FrameElement.getElementById(id: string)` - unify the process to
  internally resolve a `<turbo-frame>` element by its `[id]`. Update
  call sites to use this new method

* `FrameController.proposeVisitIfNavigatedWithAction(frame, action)` -
  flatten the variable arguments into a single `action` argument. Move
  the `getVisitAction` call to the call sites that require it, and pass in
  the `action` directly when its available

[hotwired#489]: hotwired#489
[hotwired#468]: hotwired#468
[comment on hotwired#489]: hotwired#489 (comment)
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 22, 2024
Closes [hotwired#489][]
Closes [hotwired#468][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on hotwired#489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

To support the implementation changes necessary to pass the tests, this
commit changes the `proposeVisitIfNavigatedWithAction(frame, action)`
interface by flattening the variable arguments into a single `action`
argument, then moving the `getVisitAction` call to the call sites that
require it. The call sites that know their `action` pass it in directly
when its available.

[hotwired#489]: hotwired#489
[hotwired#468]: hotwired#468
[comment on hotwired#489]: hotwired#489 (comment)
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Jan 30, 2024
Closes [hotwired#489][]
Closes [hotwired#468][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on hotwired#489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

To support the implementation changes necessary to pass the tests, this
commit changes the `proposeVisitIfNavigatedWithAction(frame, action)`
interface by flattening the variable arguments into a single `action`
argument, then moving the `getVisitAction` call to the call sites that
require it. The call sites that know their `action` pass it in directly
when its available.

[hotwired#489]: hotwired#489
[hotwired#468]: hotwired#468
[comment on hotwired#489]: hotwired#489 (comment)
afcapel pushed a commit that referenced this issue Feb 2, 2024
Closes [#489][]
Closes [#468][]

First, add test coverage to exercise the `<turbo-frame id="frame">` and
`<turbo-frame id="hello" target="frame">` as outlined in a
[comment on #489][].

Next, add coverage to support driving a `<turbo-frame
data-turbo-action="advance">` through the `Turbo.visit` method. To pass
that test coverage, invoke
`frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)`
prior to modifying the element's `[src]` attribute.

To support the implementation changes necessary to pass the tests, this
commit changes the `proposeVisitIfNavigatedWithAction(frame, action)`
interface by flattening the variable arguments into a single `action`
argument, then moving the `getVisitAction` call to the call sites that
require it. The call sites that know their `action` pass it in directly
when its available.

[#489]: #489
[#468]: #468
[comment on #489]: #489 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.