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

Turbo should not perform a visit() when tapping same-page anchors #539

Open keithamus opened 2 years ago

keithamus commented 2 years ago

I'm pretty sure this is a regression, since this issue is effectively a dupe of https://github.com/hotwired/turbo/pull/298 and https://github.com/hotwired/turbo/issues/42.

When clicking a link with an href that is an anchor on the page, even if the anchor is present in the URL, Turbo will initiate a navigation, causing the page to "reload". Here's a minimal reproduction:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <script type="module">
      import {session} from "https://cdn.skypack.dev/@hotwired/turbo@7.1.0";
      session.drive = !new URLSearchParams(location.search).has('nodrive')
      const a = document.createElement('a')
      a.href = '#foo'
      a.textContent = 'I should be visible!'
      document.body.append(a)
      setTimeout(() => a.click())
    </script>
  </head>
  <body>
    <h1 id="foo">Below should be a visible link</h1>
  </body>
</html>

Steps to reproduce

  1. Copy the above code snippet and save as index.html on your local filesystem
  2. Spin up a web server (I used npx http-server, you could also use python -m http-server 8080, or ruby -run -e httpd -- -p 8080 .)
  3. Visit "http://127.0.0.1:8080/#foo"
  4. Observe the link "I should be visible" flashes for a second but disappears.
    • Expected: The link is visible
    • Actual: The link disappears.
  5. Visit "http://127.0.0.1:8080/?nodrive#foo"
  6. Observe the browsers default behaviour, that the link is visible and the page did not reload.

It is also possible to create an infinite reload loop with this:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <script type="module">
      import {session} from "https://cdn.skypack.dev/@hotwired/turbo@7.1.0";
      session.drive = !new URLSearchParams(location.search).has('nodrive')
      customElements.define('lazy-click', class extends HTMLElement {
        connectedCallback() {
          setTimeout(() => this.querySelector('a').click())
        }
      })
    </script>
  </head>
  <body>
    <h1 id="foo">This page will cause an infinite load loop!</h1>
    <turbo-frame id="infinite-loop">
      <lazy-click><a href="#foo">I get clicked!</a></lazy-click>
    </turbo-frame>
  </body>
</html>

Steps to reproduce

  1. Copy the above code snippet and save as index.html on your local filesystem
  2. Spin up a web server (I used npx http-server, you could also use python -m http-server 8080, or ruby -run -e httpd -- -p 8080 .)
  3. Visit "http://127.0.0.1:8080/#foo"
  4. Watch the web server logs. The browser is repeatedly requesting the page over and over.
    • Expected: No infinite load loop
    • Actual: An infinite load loop
  5. Visit "http://127.0.0.1:8080/?nodrive#foo"
  6. Observe the browsers default behaviour, that the link is visible and the page did not reload.
dhh commented 2 years ago

cc @domchristie

domchristie commented 2 years ago

It's a bit odd, but in this case, it's sort of the desired behaviour as set out in: https://github.com/hotwired/turbo/pull/324. The first click to"#foo won't hit the server; subsequent clicks will. This enables refreshing the on-screen page with a Turbo visit (i.e. without a hard browser refresh).

However, I think this was only intended for replace visits, so the logic might be tweakable to adjust the conditions for the anchor -> currentAnchor check in https://github.com/hotwired/turbo/blob/8221c1e5f2021c41c3ecada0dde55bae9ee18380/src/core/drive/navigator.ts#L143

koddsson commented 2 years ago

It's surprising to me that Turbo will navigate on anchor links since that's not how browsers treat clicks to intra-page navigation links.

We call .click() on an anchor element to retrigger the :target selector for an element since :target doesn't play well with dynamically loaded content.

But that's not what :target is designed as. It's instead designed to match a specific element which is identified at a specific time very early during page load, or after fragment navigation. It doesn't update whenever the DOM changes, and it doesn't update whenever the URL fragment changes. Instead it always a pointer to a specific element, similar to :focus.

https://github.com/WICG/app-history/issues/162#issuecomment-1010162749

This reproduction here shows the interplay between these two issues. If you are clicking a link to trigger the :target selector, Turbo will loop endlessly since it considers the click on an anchor to be inter-page navigation as opposed to intra-page navigation.

With ?nodrive the browser doesn't navigate and highlights the correct element whereas with Turbo we'll loop.

Is it intentional that Turbo diverges from browser behaviours in this way? We think that Turbo should not treat intra-page anchor links as navigations.

And just to be clear, our handling of :target is a hack that is causing issues and we'll look into solving that separately. It's a happenstance that we've hit this issue with Turbo but as we've discovered we think that Turbo should emulate browsers more accurately here.

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8">
    <script type="module">
      import {session} from "https://cdn.skypack.dev/@hotwired/turbo@7.1.0";
      session.drive = !new URLSearchParams(location.search).has('nodrive')

      function turboLoad(event) {
        if (window.location.hash) {
          const target = event.target.querySelector(window.location.hash)
          const anchor = document.querySelector(`[href="${window.location.hash}"`)
          if (target && anchor) {
            anchor.click()
          }
        }
      }

      document.addEventListener('turbo:load', turboLoad)
    </script>
    <style>
      :target {
         border: 1px solid hotpink;
      }
    </style>
  </head>
  <body>
    <turbo-frame loading="lazy">
      <h1 id="foo">This page will cause an infinite load loop!</h1>
    </turbo-frame>
    <a href="#foo">Go to #foo</a>
  </body>
</html>
domchristie commented 2 years ago

It's surprising to me that Turbo will navigate on anchor links since that's not how browsers treat clicks to intra-page navigation links … Is it intentional that Turbo diverges from browser behaviours in this way?

In most cases, Turbo follows browser behaviour for same-page links. For example, it won't issue a request when navigating from / to /#main. See also the tests for this feature.

However, when the action is replace, or if the current location and the proposed location are identical, it will make a request, as proposed in https://github.com/hotwired/turbo/pull/324. This enables a Turbo equivalent to window.location.reload(): Turbo.visit(location.toString()). I may have overlooked something to keep the conditions reasonable, so as I mentioned above, you may want to try tweaking the conditions in navigator#locationWithActionIsSamePage.

We call .click() on an anchor element to retrigger the :target selector for an element since :target doesn't play well with dynamically loaded content.

You could try temporarily disabling Turbo for these .click() calls. Not tested it, but something like:

function triggerTarget (a) {
  const turbo = a.dataset.turbo
  a.dataset.turbo = false
  a.click()
  a.dataset.turbo = turbo
}

You might also be interested in the hashchange event. Turbo triggers this after a same-page visit. The event data includes the new and previous URLs.