Integration with Turbo

[davegudge]

Hi Chris,

First of all, great work on this project 👍🏼

I am looking into fixing an issue I've found whist using the dropdown component in a Turbo/Rails setup.

The dropdown is working as expected, the issue occurs when navigating back to the screen where the dropdown link was clicked (via the back button), the dropdown remains open:

dropdown.mov

Adding data-turbo="false" to the dropdown options solves the issue, so it's definitely a turbo related problem.

I was hoping that by adding a this.hide() call, as part of the disconnect() method, would achieve the desired solution:

diff --git a/src/dropdown.js b/src/dropdown.js
index de8a0bf..ff2c64e 100644
--- a/src/dropdown.js
+++ b/src/dropdown.js
@@ -49,6 +49,7 @@ export default class extends Controller {
     if (this.hasButtonTarget) {
       this.buttonTarget.removeEventListener("keydown", this._onMenuButtonKeydown)
     }
+    this.hide();
   }


@@ -125,7 +126,7 @@ export default class extends Controller {
   }

   hide(event) {
-    if (this.element.contains(event.target) === false && this.openValue) {
+    if ((event === undefined || this.element.contains(event.target) === false) && this.openValue) {
       this.openValue = false
     }
   }

However, the hide does not occur until the page is redisplayed:

dropdown2.mov

As a workaround, I've added a turbo:before-cache to hide the dropdown e.g:

$(document).on('turbo:before-cache', function() {
  $('[data-controller="dropdown"] [data-dropdown-target]').addClass('hidden');
});

If you have any ideas on how tailwindcss-stimulus-components could be modified to accommodate this issue, I'm happy to implement the change and open a PR. Perhaps it falls outside the scope of tailwindcss-stimulus-components, and using the turbo:before-cache is the best solution?

Thanks

[excid3]

The only thing I can think of would be adding and removing a turbo:before-cache callback listener in the controller automatically. I imagine that the before-cache event will typically fire first because the Stimulus controller won't disconnect until the page is replaced.

Probably all of the components would need this, since you probably want to reset all of them before-cache.

[excid3]

Found this on Turbolinks: turbolinks/turbolinks#390

I would assume Turbo is supposed to work similarly. Maybe something is wrong with this functionality.

[excid3]

Looks like it's an open issue with Turbo: hotwired/turbo#117

PR with a fix is in main but unreleased. Try testing against turbo's main branch and let me know if that fixes it!

[davegudge]

Thank you for the information. I’ll test against Turbo’s main branch tomorrow and I’ll post an update back here.

[davegudge]

I've tried both hotwired/turbo#169 and hotwired/turbo#162, neither seem to fix the issue described above. The page seems to be cached prior to the stimulus#disconnect logic being executed.

For now, I'll stick with the turbo:before-cache workaround.

$(document).on('turbo:before-cache', function() {
  $('[data-controller="dropdown"] [data-dropdown-target]').addClass('hidden');
});

[excid3]

It's probably a combination of the Turbo disconnect at the right time, and we need to make sure we close dropdowns, etc on disconnect (which I don't believe we do right now).

[davegudge]

From looking at turbolinks/turbolinks#390 which you linked to above, it looks like Turbolinks was updated to defer the snapshot caching until after the stimulus disconnect. Turbo does not seem to be working in the same way.

we need to make sure we close dropdowns, etc on disconnect (which I don't believe we do right now).

I have made this change locally, but as mentioned above, it's not quite working as expected with Turbo. I'll switch back to a Turbolinks setup and see what happens there, and depending on the outcome, I'll open a PR with the changes.

[davegudge]

Having thought about this some more, I think the issue can be resolved in tailwindcss-stimulus-components.

Turbo is doing the correct thing by snapshot'ing the state of the page upon departure, so we need to get the page into the correct state when a user is leaving the page via clicking a link in the dropdown menu, e.g. upon selecting a link in the dropdown, the dropdown should be closed. This appears to happen as a new page is rendered, however, the dropdown is actually still open.

Solution 1:

The user is responsible for adding Stimulus markup on the dropdown items: e.g: data-action="click->dropdown#toggle"

<div data-controller="dropdown" aria-haspopup="true" aria-expanded="false">
  <div aria-haspopup="true" aria-expanded="false">
    <button data-action="click->dropdown#toggle click@window->dropdown#hide">
      <span>Button label</span>
    </button>
  </div>
  <div data-dropdown-target="menu">
    <div>
      <div role="menu" aria-orientation="vertical">
        <a role="menuitem" data-action="click->dropdown#toggle" href="#">Option 1</a>
        <a role="menuitem" data-action="click->dropdown#toggle" href="#">Option 2</a>
        <a role="menuitem" data-action="click->dropdown#toggle" href="#">Option 3</a>
      </div>
    </div>
  </div>
</div>

Solution 2:

The Stimulus DropdownController#connect method binds a listener, so that the dropdown menu is closed when an option is clicked

    if (this.hasMenuTarget) {
      $(this.menuTarget).on("turbo:click", (event) => {
        this.hide()
      });
    }

Both of these solutions work as expected when using Turbo. If you let me know your thoughts I can look to get a PR opened.

[thanosbellos]

Hi @davegudge i was about to close the issue on Turbo's repository but i found out about your issues. I have some questions/notes:

1. Did adding this.hide() on your controller's disconnect method have the desired effect, while using the latest public version of Turbo(v.7.0.0.beta4) or some the pull requests mentioned in (Snapshot caching issue after upgrading from turbolinks to turbo hotwired/turbo#117)
2. Did adding this.hide() on disconnect work with Turbolinks?

Turbo is doing the correct thing by snapshot'ing the state of the page upon departure, so we need to get the page into the correct state when a user is leaving the page via clicking a link in the dropdown menu, e.g. upon selecting a link in the dropdown, the dropdown should be closed.

This is actually very spot on. My concern is that while using Turbolinks disconnect method was used in order to have a single and final spot where your controller would shape page's state, before it was cached. Before Turbo-v.7.0.0.beta4 it was definitely not the case for me.. But after this release my experience has improved vastly regarding this issue.

From your answers i have come to conclusion that the issue still persists even while using the main branch of Turbo. If my assumption is correct, i should keep my issue open at Turbo's repository.. if not i am gonna close it. Thanks in advance.

[davegudge]

Hi @thanosbellos,

For the issue I have outlined in this ticket, I came to the conclusion that the fix should be part of tailwindcss-stimulus-components rather than Turbo. Upon selecting an option in the dropdown, tailwindcss-stimulus-components should be responsible for closing the dropdown. This tends to happen naturally, as the links in the dropdown take the user to another screen, and traditionally, that would fully reload the page, thus the dropdown is closed upon rendering the new page.

Another way to think about this without involving Turbo. If one of the links in the dropdown was used to call a JavaScript function (e.g toggle light/dark mode) without navigating the user away from the page, upon click, I would expect the dropdown menu to close. This would not happen at the moment.

As a consequence of the dropdown being left open, this is the state which is captured by Turbo upon snapshoting as outlined in the issue above.

By tailwindcss-stimulus-components ensuring that the dropdown state is updated accordingly via one of the two methods outlined in #63 (comment), the dropdown is left in the correct state for Turbo to cached, thus using the back button presents the dropdown as expected.

However, all that said, I was expecting to be able to add this.hide() to the controllers disconnect method to achieve the same effect, albeit this approach would not cater for the JavaScript light/dark mode example above and this is why the responsible should lie with tailwindcss-stimulus-components.

So I'm pretty certain that Turbo is working slightly different to Turbolinks. If you look add the diagrams added on turbolinks/turbolinks#390

Turbolinks was updated to ensure the snapshot was saved to the cache after the controller's disconnect method has been called (I haven't tested the issue outlined on this thread using Turbolinks). I found that this was not happening in Turbo(v.7.0.0.beta4) or in the pull requests mentioned in (hotwired/turbo#117). However, it's worth noting that my dropdown was using the animations as outlined at https://github.com/excid3/tailwindcss-stimulus-components#dropdowns e.g:

data-dropdown-invisible-class="opacity-0 scale-95"
data-dropdown-visible-class="opacity-100 scale-100"
data-dropdown-entering-class="ease-out duration-300"
data-dropdown-enter-timeout="300"
data-dropdown-leaving-class="ease-in duration-300"
data-dropdown-leave-timeout="300"

So perhaps Turbo is calling controller#disconnect and before the animation to close the dropdown had finished, Turbo snapshots the page, thus the dropdown appearing open upon return. Would it be helpful for me to test this theory, by removing the animation (and adding some debug output into Turbo)?

[thanosbellos]

Thank you very much for your time and for your detailed response.

Hi @thanosbellos,
For the issue I have outlined in this ticket, I came to the conclusion that the fix should be part of tailwindcss-stimulus-components rather than Turbo. Upon selecting an option in the dropdown, tailwindcss-stimulus-components should be responsible for closing the dropdown. This tends to happen naturally, as the links in the dropdown take the user to another screen, and traditionally, that would fully reload the page, thus the dropdown is closed upon rendering the new page.
Another way to think about this without involving Turbo. If one of the links in the dropdown was used to call a JavaScript function (e.g toggle light/dark mode) without navigating the user away from the page, upon click, I would expect the dropdown menu to close. This would not happen at the moment.
As a consequence of the dropdown being left open, this is the state which is captured by Turbo upon snapshoting as outlined in the issue above.
By tailwindcss-stimulus-components ensuring that the dropdown state is updated accordingly via one of the two methods outlined in #63 (comment), the dropdown is left in the correct state for Turbo to cached, thus using the back button presents the dropdown as expected.

Definitely on board with this approach.. It's really logical to handle both the cases you described at will. i.e someone may want to find the dropdown open after pressing back button.

However, all that said, I was expecting to be able to add this.hide() to the controllers disconnect method to achieve the same effect, albeit this approach would not cater for the JavaScript light/dark mode example above and this is why the responsible should lie with tailwindcss-stimulus-components.

This is what caught my attention. That, regardless the desired intention and how it could be achieved by the library:

1. you would like to manipulate dom before caching
2. you expected that if you did so from the disconnect method
3. you expected the cached page would be the result of the manipulation.
   just like it was happening using turbolinks after the (Deferred snapshot caching turbolinks/turbolinks#390)

However, it's worth noting that my dropdown was using the animations as outlined at https://github.com/excid3/tailwindcss-stimulus-components#dropdowns e.g:
data-dropdown-invisible-class="opacity-0 scale-95"
data-dropdown-visible-class="opacity-100 scale-100"
data-dropdown-entering-class="ease-out duration-300"
data-dropdown-enter-timeout="300"
data-dropdown-leaving-class="ease-in duration-300"
data-dropdown-leave-timeout="300"
So perhaps Turbo is calling controller#disconnect and before the animation to close the dropdown had finished, Turbo snapshots the page, thus the dropdown appearing open upon return. Would it be helpful for me to test this theory, by removing the animation (and adding some debug output into Turbo)?

If you have the time i would be really grateful if you tried that scenario(no animations). I will proceed and test it using turbolinks.

[davegudge]

Adding the following to tailwindcss-stimulus-components's src/dropdown.js

  connect() {
    console.log("DropdownController#connect");

...

  disconnect() {
    console.log("DropdownController#disconnect");

and the following to my own project:

$(document).on('turbo:click', function() {
  return console.log('turbo:click');
});

$(document).on('turbo:before-visit', function() {
  return console.log('turbo:before-visit');
});

$(document).on('turbo:visit', function() {
  return console.log('turbo:visit');
});

$(document).on('turbo:submit-start', function() {
  return console.log('turbo:submit-start');
});

$(document).on('turbo:before-fetch-request', function() {
  return console.log('turbo:before-fetch-request');
});

$(document).on('turbo:before-fetch-response', function() {
  return console.log('turbo:before-fetch-response');
});

$(document).on('turbo:submit-end', function() {
  return console.log('turbo:submit-end');
});

$(document).on('turbo:before-cache', function() {
  return console.log('turbo:before-cache');
});

$(document).on('turbo:before-render', function() {
  return console.log('turbo:before-render');
});

$(document).on('turbo:before-stream-render', function() {
  return console.log('turbo:before-stream-render');
});

$(document).on('turbo:render', function() {
  return console.log('turbo:render');
});

$(document).on('turbo:load', function() {
  return console.log('turbo:load');
});

Using "@hotwired/turbo-rails": "^7.0.0-beta.5" defined via the package.json, the output is as follows:

Initial page load:

turbo:load
DropdownController#connect

Click a link in the dropdown (or simply navigate to another page)

turbo:click
turbo:before-visit
turbo:before-fetch-request
turbo:visit
turbo:before-fetch-response
turbo:before-cache
turbo:before-render
DropdownController#disconnect
DropdownController#connect
turbo:render
turbo:load

The turbo:before-cache is clearly happening before the DropdownController#disconnect is called.

[davegudge]

This is what caught my attention. That, regardless the desired intention and how it could be achieved by the library:
1. you would like to manipulate dom before caching
2. you expected that if you did so from the disconnect method
3. you expected the cached page would be the result of the manipulation. just like it was happening using turbolinks after the (turbolinks/turbolinks#390)

Yes, my expectation was that any manipulation via the controller disconnect method would be applied before the page was cached. That expectation was based upon turbolinks/turbolinks#390 (comment)

[davegudge]

Using "@hotwired/turbo-rails": "^7.0.0-beta.5" defined via the package.json, the output is as follows:

Would you like me to test against Turbo's main branch? Did you find an easy way to do this? I have both turbo-rails and turbo checked out locally. I'm assuming the easiest way is to copy the relevant files into my project's node_modules/@hotwired/turbo/dist/ (and node_modules/@hotwired/turbo-rails/app/assets/javascripts/turbo.js)?

[thanosbellos]

Using "@hotwired/turbo-rails": "^7.0.0-beta.5" defined via the package.json, the output is as follows:

Would you like me to test against Turbo's main branch? Did you find an easy way to do this? I have both turbo-rails and turbo checked out locally. I'm assuming the easiest way is to copy the relevant files into my project's node_modules/@hotwired/turbo/dist/ (and node_modules/@hotwired/turbo-rails/app/assets/javascripts/turbo.js)?

Yeah i use turbo-rails too. i didn't dig up more about a better way to test it out.. that's how i test it too. i just created a new project to test it out with turbolinks. If you have the time and you are in the mood you can proceed with your tests 😄 But you have helped a lot so don't feel obliged to do so :) I will report my finding back soon (i guess)

[davegudge]

The turbo:before-cache is clearly happening before the DropdownController#disconnect is called.

That said, in the 'after' diagram on turbolinks/turbolinks#390, turbo:before-cache is called before the controller's disconnect, but the snapshot'ing is deferred until after the controller's disconnect:

by deferring caching, it’s now possible for Stimulus controllers (and other code which uses MutationObserver) to prepare elements for caching in response to mutation records.

So the log output above is as expected I guess!

[thanosbellos]

Exactly. I guess that their goal is to maintain the same behaviour in Turbo too.. It makes perfect sense to have your controller's disconnect method, to reset external libraries(i.e uppy file uploader, lightbox, flatpickr).

What seems to be happening is that sometimes, if you manipulate the dom inside the disconnect method, the snapshot cache does not contain the changes. All of my custom controllers suffered from the issue before v.7.0.0.beta4.

I only have one controller now that still has the same issue.. although it didn't work well with turbolinks neither.. that's why i was about to close the issue and then i saw your use case.

[davegudge]

I've removed Turbo (and Turbo-rails) and switched the project back to "turbolinks": "^5.2.0". The same issue occurs when using the back button, i.e. the drop-down is still open. If I add this.hide() the tailwindcss-stimulus-components dropdown controller disconnect method, it's working exactly the same as the 2nd video in #63 (comment) i.e. click back, the page is displayed, then the hide occurs.

In summary, it seems like Turbo is working the same as Turbolinks, but it is not how I was expecting it to work based upon turbolinks/turbolinks#390

It’s natural to expect to be able to “tear down” a controller’s element in a Stimulus disconnect() callback, but under the current rendering process, any changes to the element are ignored since the callback happens after the document has been cached by Turbolinks.

In the revised rendering process, Turbolinks defers snapshot caching using a setTimeout callback, moving the expensive cloneNode() operation outside of the animation frame.

Additionally, by deferring caching, it’s now possible for Stimulus controllers (and other code which uses MutationObserver) to prepare elements for caching in response to mutation records.

---

For completeness, here's the output using Turbolinks:

Initial page load:

DropdownController#connect
turbolinks:load

Click a link in the dropdown (or simply navigate to another page)

turbolinks:click
turbolinks:before-visit
turbolinks:request-start
turbolinks:visit
turbolinks:request-end
turbolinks:before-cache
turbolinks:before-render
turbolinks:render
turbolinks:load
DropdownController#disconnect
DropdownController#connect

[thanosbellos]

Yeap tested it on new project and it seems to behave like what you have described on both occasions. Even without animations i see the menu closing rapidly after restoring from the cache. But it does close something that is crucial in general. As long as it has the same behaviour, i am gonna close it for now and maybe we start a new issue about whether the "expected" functionality could be achieved. Thanks a lot for your time and contribution.

[davegudge]

Even without animations i see the menu closing rapidly after restoring from the cache. But it does close something that is crucial in general.

Just to confirm, this doesn't happen when using tailwindcss-stimulus-components by default, the dropdown is always open after restoring from the cache?

Once this.hide() is added to the tailwindcss-stimulus-components dropdown controller disconnect method, I experience what you have described:

the menu closing rapidly after restoring from the cache.

[thanosbellos]

Even without animations i see the menu closing rapidly after restoring from the cache. But it does close something that is crucial in general.

Just to confirm, this doesn't happen when using tailwindcss-stimulus-components by default, the dropdown is always open after restoring from the cache?

Once this.hide() is added to the tailwindcss-stimulus-components dropdown controller disconnect method, I experience what you have described:

the menu closing rapidly after restoring from the cache.

Exactly. those were my findings using the modified version that calls hide when the controller disconnects and for either library(turbolinks and turbo)

[davegudge]

I will close this issue as I have opened #65 which resolves the problem outlined above.