hotwired / turbo

The speed of a single-page web application without having to write any JavaScript
https://turbo.hotwired.dev
MIT License
6.65k stars 421 forks source link

data-turbo-action is not respected on requestSubmit() event from javascript #747

Closed tbeemster closed 1 year ago

tbeemster commented 1 year ago

Hi there,

While updating turbo to version 7.2 today, I discovered a bug. Before upgrading to version 7.2, after calling requestSubmit() on the form via javascript, for example by clicking on the option "25" in the select box, Turbo would change the window location into something like /page?page_size=25 However, after performing the same steps with Turbo 7.2, the window location is not updated, and stays /page I've added a reduced snippet of HTML and JS to illustrate the problem.

If you need any additional information to debug the problem, I'm happy to supply extra information. Thanks for your great product! :-)

<form action="/page" method="GET" data-turbo-action="advance" id="filter-table"></form>

<select name="page_size" data-controller="select" form="filter-table">
<option value="10">10</option>
<option value="25">25</option>
<option value="50">50</option>
</select>

The select_controller.js has a small snippet of javascript that will invoke requestSubmit() when the value of the select has changed.

...
export default class SelectController extends Controller {
  connect () {
  ...
      this.htmlInput.listen('MDCSelect:change', () => {
        htmlInput.form.requestSubmit()
      })
    ...
  }

  disconnect () {
    ...
  }
}
seanpdoyle commented 1 year ago

Thank you for opening this issue @tbeemster!

I've done my best to reproduce this behavior in the Turbo test suite, but haven't been able to.

I've pushed up the code to https://github.com/hotwired/turbo/compare/seanpdoyle:fix-issue-747. Does that code feel representative of your use case?

The suite passes in both Chrome and Firefox. Is there something I'm missing?

tbeemster commented 1 year ago

Hi @seanpdoyle, Thanks for your quick reply. In my understanding the data-turbo-action="advance" property would trigger the GET parameter to be propagated to the window.location. However, I see that in your test you are using data-turbo-action="replace". Could that be the problem?

Additionally, I just discovered that the

also has a data-turbo-frame property as target.

So basically it is something like this:

     <form id="form-with-external-inputs" action="/src/tests/fixtures/one.html" data-turbo-action="advance" data-turbo-frame="external-content"></form>
      <select id="external-select" form="form-with-external-inputs" name="greeting">
        <option selected>Hello from a replace Visit</option>
      </select>
<turbo-frame id="external-content">
After submitting the external inputs form, the content of that fetch request will be placed here.
</turbo-frame>
seanpdoyle commented 1 year ago

@tbeemster thank you for that extra information. I've pushed up https://github.com/hotwired/turbo/commit/e68d1ff1b071106cfc4ff3861d8541d7a69dd8ca to https://github.com/hotwired/turbo/compare/seanpdoyle:fix-issue-747.

Unfortunately, I'm not able to reproduce the issue, and the test still passes without any implementation changes.

Is it possible that there are other overlapping side-effects in your application that are not reflected in the test?

tbeemster commented 1 year ago

Hi @seanpdoyle, Your effort is much appreciated, I'm sorry that the reproduction is not possible just yet. I will try to get turbo running locally and see if I can get a failing test. I'll get back to you. Thanks again

tbeemster commented 1 year ago

Hi @seanpdoyle,

After some fiddling around, I finally found the problem! Adding another <turbo-frame> around the form actually causes the problem.

I've added the problematic code here: https://github.com/hotwired/turbo/compare/main...tbeemster:turbo:747-bug-reproduction?expand=1

Hopefully this helps solving the problem, I'm sorry I can't help resolving the issue myself (I'm too inexperienced in the code just yet). The contribution readme was very helpful setting everything up locally, though.

seanpdoyle commented 1 year ago

@tbeemster Just to make sure I understand your use case, I'd like to play back and summarize the details thus far:

  1. A <form> element is annotated with [data-turbo-action="advance"]
  2. That <form> element has a <select> control that is not a direct descendant, but exists outside the <form> and belongs to the form through a [form] attribute
  3. The <form> has a <turbo-frame> element as its ancestor
  4. The <turbo-frame> ancestor is not significant, since the <form> element targets a different <turbo-frame> element elsewhere in the page by declaring a [data-turbo-frame] attribute
  5. There is JavaScript that listens for a custom MDCSelect:change event, then reaches through an HTMLSelectElement instance's .form property to access the <form>, and submits that form by calling HTMLFormElement.requestSubmit
  6. When your project depended on 7.1.0, changing the <select> element's option[selected] submitted the referenced <form> element, which drove a <turbo-frame> element that isn't the ancestor <turbo-frame>, and that frame's [src] was promoted to a page-wide navigation, updating the browser's URL bar

Is that an accurate outline?

tbeemster commented 1 year ago

Hi @seanpdoyle,

I think you are correct in your findings. I have attached a beautiful picture, which hopefully shows you how I'm using this.

(By changing the select box, the "filter-table-form" is submitted. Since that form has data-turbo-frame="table", the response of that form will be parsed in the turbo-frame table. Since that form also has data-turbo-action="advance" the URL of that form submission will also be placed in the browser's URL bar)

html-with-description

seanpdoyle commented 1 year ago

I've opened https://github.com/hotwired/turbo/pull/749 to try and resolve this issue.

tbeemster commented 1 year ago

Thanks a lot