LiveView disconnects when clicking a download link

dustinfarris commented 1 year ago

Environment

Elixir version (elixir -v):

Erlang/OTP 25 [erts-13.2] [source] [64-bit] [smp:16:16] [ds:16:16:10] [async-threads:1] [jit:ns] 
Elixir 1.14.3 (compiled with Erlang/OTP 25)

Phoenix version (mix deps):

phoenix 1.6.16 (Hex package) (mix)
locked at 1.6.16 (phoenix) e15989ff

Phoenix LiveView version (mix deps):

phoenix_live_view 0.18.18 (Hex package) (mix)
locked at 0.18.18 (phoenix_live_view) a5810d04

Operating system:
```
MacOS Ventura
```
Browsers you attempted to reproduce this bug on (the more the merrier):
```
Safari
Firefox
```
Does the problem persist after removing "assets/node_modules" and trying again? Yes/no:
```
Yes
```

Actual behavior

The LiveView disconnects when clicking a download link, e.g.

<a href="google.com" download="google.html">Download Google</a>

Causes the LiveView to disconnect in a manner similar to #2542:

destroyed: the child has been removed from the parent

Expected behavior

Interacting with the link does not cause LiveView to disconnect.

chrismccord commented 1 year ago

target="_blank" will resolve the issue, but I will add a check to avoid unloading for download

sherbondy commented 1 year ago

Along these lines, we would still really love a more declarative explicit way to convey to liveview to not futz with certain links.

Relating back to this issue: https://github.com/phoenixframework/phoenix_live_view/issues/2394

Would love to better understand the motivation behind the breaking change in behavior from 0.18.4 onwards wrt how LiveView intercepts anchor tag click events. Understand we can opt out in an imperative way via stop(Immediate)Propagation, but would love a more reliable bulletproof way to declaratively signal to liveview to do the same, re: ignoring and never responding to click events for certain anchor tags.

Feel like it’s hard (and a huge hassle for you and the other active maintainers of the framework!) to play whack-a-mole with all the weird esoteric things people can do with anchor tags in a browser, and keen to better grok why this is necessary for newer versions of LiveView to function as expected, with the disconnect handling logic.

josevalim commented 1 year ago

Perhaps an attribute called phx-no-disconnect or similar?

sherbondy commented 1 year ago

Perhaps an attribute called phx-no-disconnect or similar?

Yeah something like this would be amazing for us as a peace-of-mind bit!

sherbondy commented 1 year ago

Hey folks, just wanted to share, have a PR up with a fix for this + implementation of the proposed phx-no-disconnect attribute: https://github.com/phoenixframework/phoenix_live_view/pull/2611

We're testing this out in a large project, and the fix + new attribute are working great.

Here's my fork with a branch where the JS assets have been pre-compiled with mix assets.build if you want to test it out yourself locally: https://github.com/sherbondy/phoenix_live_view/commit/7a9c99a65d3e91f52633ebc9d573aa7ad454fd1c

Can reference the SHA in your mix.exs like so:

defp deps do
  [
    # your other deps...
    {:phoenix_live_view,
     git: "https://github.com/sherbondy/phoenix_live_view",
     ref: "7a9c99a65d3e91f52633ebc9d573aa7ad454fd1c",
     override: true}
  ]
end

Hope this helps, @dustinfarris!

Would be great to get these changes incorporated in an upcoming patch release.

chrismccord commented 1 year ago

I don't think we need a new phx attribute. Once we add a check for download, I think we cover all use-cases.

sherbondy commented 1 year ago

Ok have updated the PR to just include the download bit: https://github.com/phoenixframework/phoenix_live_view/pull/2611

All the best!

Definitely a little bit more hassle for us to update all the call-sites where we do client-side routing to include new click event handlers or hooks to stop propagation so LiveView doesn't disconnect, but understand this is an edge case and that mixing client-side routing and live navigation in a single page is probably ill-advised anyway.

I appreciate that adding a new attribute is something not to be taken lightly, and we definitely do have escape hatches in the form of hooks / arbitrary JS event handlers to work around this issue. Thanks for the follow-up and consideration.

jfabre commented 1 year ago

I have the same kind of issue, but for links that use protocols directed at specific applications.

In my case:

<a href="talespire://attack/1d20+2">Attack</a>

I added target="_blank" for now, but it opens up a new tab.

rktjmp commented 1 year ago

It also seems to disconnect for tel: and mailto:. It also fails to remove the phx-connected class from the parent after disconnection.

Application.put_env(:sample, Example.Endpoint,
  http: [ip: {127, 0, 0, 1}, port: 5001],
  server: true,
  live_view: [signing_salt: "aaaaaaaa"],
  secret_key_base: String.duplicate("a", 64)
)

Mix.install([
  {:plug_cowboy, "~> 2.5"},
  {:jason, "~> 1.0"},
  {:phoenix, "~> 1.7.2"},
  {:phoenix_live_view, "~> 0.18.18"}
])

defmodule Example.ErrorView do
  def render(template, _), do: Phoenix.Controller.status_message_from_template(template)
end

defmodule Example.HomeLive do
  use Phoenix.LiveView, layout: {__MODULE__, :live}

  def mount(_params, _session, socket) do
    {:ok, assign(socket, :count, 0)}
  end

  def render("live.html", assigns) do
    ~H"""
    <script src="https://cdn.jsdelivr.net/npm/phoenix@1.7.2/priv/static/phoenix.min.js"></script>
    <script src="https://cdn.jsdelivr.net/npm/phoenix_live_view@0.18.18/priv/static/phoenix_live_view.min.js"></script>
    <script>
      let liveSocket = new window.LiveView.LiveSocket("/live", window.Phoenix.Socket)
      liveSocket.connect()
      setInterval(() => {
        const c = liveSocket.isConnected()
        document
          .querySelectorAll("#connected-indicator")
          .forEach(el => el.setAttribute("socket-connected", c))
      }, 200)
    </script>
    <style>
      * { font-size: 1.1em; }

      #connected-indicator::after { content: attr(socket-connected); }
      #connected-indicator[socket-connected=false]{ color: red; }
      #connected-indicator[socket-connected=true]{ color: green; }

      .indicator { color: red; }
      .indicator::after{ content: ' .phx-connected not present'; }
      .phx-connected .indicator { color: green; content: 'connected' }
      .phx-connected .indicator::after{ content: ' .phx-connected present'; }
    </style>

    <div id="connected-indicator" class="">liveSocket.isConnected() = </div>
    <%= @inner_content %>
    """
  end

  def render(assigns) do
    ~H"""

    <div class="indicator">.phx-connected .indicator -> </div>

    <a href="tel:000000">tel</a>
    <a href="mailto:000000">mailto</a>
    <a href="talespire://attack/1d20+2">Attack</a>

    <button phx-click="inc">send event <%= @count %></button>
    """
  end

  def handle_event("inc", params, socket) do
    {:noreply, assign(socket, :count, socket.assigns.count + 1)}
  end
end

defmodule Example.Router do
  use Phoenix.Router
  import Phoenix.LiveView.Router

  pipeline :browser do
    plug(:accepts, ["html"])
  end

  scope "/", Example do
    pipe_through(:browser)

    live("/", HomeLive, :index)
  end
end

defmodule Example.Endpoint do
  use Phoenix.Endpoint, otp_app: :sample
  socket("/live", Phoenix.LiveView.Socket)
  plug(Example.Router)
end

{:ok, _} = Supervisor.start_link([Example.Endpoint], strategy: :one_for_one)
Process.sleep(:infinity)

sherbondy commented 1 year ago

I guess could likewise do another exclusion in the JS code to avoid disconnect if the URL protocol doesn’t match http(s)? I can help prepare another patch PR for this. Anchor tags in the browser are full of weird possibilities! Thus the original motivation for the phx-no-disconnect data attribute idea. But think this is a relatively straightforward additional exclusion to add one-liner fix around.

chrismccord commented 1 year ago

LV main considers download links now FYI

dkulchenko commented 1 year ago

Would love to better understand the motivation behind the breaking change in behavior from 0.18.4 onwards wrt how LiveView intercepts anchor tag click events. Understand we can opt out in an imperative way via stop(Immediate)Propagation, but would love a more reliable bulletproof way to declaratively signal to liveview to do the same, re: ignoring and never responding to click events for certain anchor tags.

Feel like it’s hard (and a huge hassle for you and the other active maintainers of the framework!) to play whack-a-mole with all the weird esoteric things people can do with anchor tags in a browser, and keen to better grok why this is necessary for newer versions of LiveView to function as expected, with the disconnect handling logic.

Agreed. With issue #2670 adding to the edge cases brought up here and in #2394, I'm wondering if there are more uncovered stones with the auto-unload behavior.

Maybe there is merit to the idea of somehow instructing the unloading to ignore certain links. In my scenario, a phx-no-disconnect attribute unfortunately wouldn't work as the links aren't directly under my control (they're in a rich text editor).

dkulchenko commented 1 year ago

Unless there's something I'm missing, I think an ideal solution would be just calling the socket's unload() from the beforeunload event on the window, instead of trying to manually exclude all non-navigation uses of the click event.

beforeunload will correctly only and always fire when actual page-to-page navigation is happening, not downloads or mailto or # or whathaveyou. This would also obviate the need for any manual escape hatches.

phoenixframework / phoenix_live_view