phoenixframework / phoenix_live_view

Rich, real-time user experiences with server-rendered HTML
https://hex.pm/packages/phoenix_live_view
MIT License
6.14k stars 921 forks source link

Assigning Page Titles incorrectly propogates the title across live navigation #3271

Open probably-not opened 4 months ago

probably-not commented 4 months ago

Environment

Actual behavior

According to the docs, we can set the page title per LiveView by assigning the :page_title assigns in a LiveView. However, assigning the :page_title in any LiveView will propagate the title to all LiveViews that are Live-navigated to if the :page_title isn't actively assigned to again.

Even worse - if the :page_title assigns is actively set to nil, this is ignored, and the title is not replaced.

This means that the user has to actively set the :page_title assigns in each LiveView that they have if they've set them at least once, and the user must also maintain the default title in each place that doesn't have a specific title, because the title cannot be set to nil.

I've created a minimal reproduction here: https://github.com/probably-not/phx-page-title-repro

Expected behavior

When assigning a title, the title should be scoped only to the current LiveView, and when navigating away from that LiveView, if a title is not assigned by the new LiveView the title should be reset. Additionally, if the :page_title is set to nil by a LiveView, the title should be reset.

SteffenDE commented 4 months ago

So I'm not sure about this. The behavior you're describing is how it has always worked and it seems like people are fine with it. Changing this would be a breaking change, so there must be very good arguments to do it. I'm not convinced.

If you want the title to change when navigating, the new LiveView should update it accordingly. If you don't want a title, you can set it to an empty string instead of nil. In JavaScript, if no title is set, document.title is an empty string as well.

There are of course ways to make your life much easier if you don't want to set a title manually in each LiveView, although it would probably be good for UX to do it anyway. I'd recommend using a hook for this:

defmodule FoobarWeb.MyTitleHook do
  import Phoenix.Component, only: [assign: 3]

  def on_mount(:reset_title, _params, _session, socket) do
    {:cont, assign(socket, :page_title, "")}
  end
end

The hook can either be added to your live_session, or in the def live_view do section of your_app_web.ex:

live_session :default, on_mount: [{FoobarWeb.MyTitleHook, :reset_title}] do
  live "/", ...
  ...
end
  def live_view do
    quote do
      use Phoenix.LiveView,
        layout: {FoobarWeb.Layouts, :app}

      on_mount {FoobarWeb.MyTitleHook, :reset_title}

      unquote(html_helpers())
    end
  end

It is then automatically run for each LiveView and will reset the title on mount. LiveViews that set their own title in mount override it, as the LiveView module's mount runs after the hook.

probably-not commented 4 months ago

Yeah, the hook direction is what I've done with my own projects.

The behavior you're describing is how it has always worked and it seems like people are fine with it

I'd be interested to know if people are fine with it, or if they've implemented a hook to just solve the issue... the hook itself is pretty trivial, I'd say it should probably be implemented in the actual library so that one can easily use it (like the many great suggestions that LiveView sprinkles throughout the code on a freshly generated project), and the behavior should definitely be documented because it currently is not and there's no mention that setting the title once means it must be set everywhere.

If you don't want a title, you can set it to an empty string instead of nil.

This doesn't work from what I'm seeing in my reproduction. I've updated my reproduction to include a page that sets the title to an empty string, and it doesn't get set to an empty string if the page title has been set by a different page, same as when trying to set to nil. It looks like there's some bug with unsetting the title in general - it can't be set to nil, or an empty string. I even tried setting it to false... maybe something is doing a general "falsey-ness" check instead of setting it to empty when it is set to a falsey value?

Attaching a video of the behavior here:

https://github.com/phoenixframework/phoenix_live_view/assets/41164052/191c754e-b77d-46b0-b805-2caecb73e3ef

probably-not commented 4 months ago

A note about setting the page title to falsey values - I've tried setting it to a string with just a space, and that does work. I've also tried setting it to 0, and that does not work, although setting it to 1 does work.

So it seems as though there's an issue with setting the page title to any falsey value.

SteffenDE commented 4 months ago

See the attached PR. Indeed we had a if (title) in JS.

Note that with

    <.live_title suffix=" · Phoenix Framework">
      <%= assigns[:page_title] || "PageTitleRepro" %>
    </.live_title>

in your root.html.heex an empty title will actually render as "PageTitleRepo · Phoenix Framework", so your hook would probably want to set the title to "PageTitleRepo" for consistency (or adapt the root template as necessary).

probably-not commented 4 months ago

See the attached PR. Indeed we had a if (title) in JS.

Excellent! Does this also solve setting it to nil (i.e. unsetting)? Or do we still need to set it to an empty string to unset?

so your hook would probably want to set the title to "PageTitleRepo" for consistency

This is one of the reasons I think the hook should be part of the generated code that LiveView provides - the generated code already provides this default title implementation, but it is completely unusable once you set a title on any of your LiveViews, because then you have to know both the default title and to set it (or, you have to know that you need to write a hook).

I think it would be very beneficial and trivial to add the simple hook implementation to the generator - it would provide a hook in the generated code to showcase how they work, it would be generated with the default title for the user, and it would provide a good place to fully document the currently undocumented behavior of page titles persisting.

SteffenDE commented 4 months ago

Excellent! Does this also solve setting it to nil (i.e. unsetting)? Or do we still need to set it to an empty string to unset?

It still needs to be set to an empty string.

The problem with "unsetting" the title is that there are cases where we don't know what an "unset" title is. If you first navigate to a page that has a title set, the <title> element will be rendered with the filled title. It won't fall back to "PageTitleRepro". Subsequent live navigations don't re-render the root layout, therefore on the client we don't know what to do when @page_title is updated to nil.

Yes, we could generate a hook for this that duplicates the fallback and updates the title, but I feel like if we indeed decided that this is should be the default behavior, it shouldn't need a hook (?).

I definitely think it's good to discuss this topic, but in the end I don't think that I'm the one to decide what to do here. The least we should do is document the current behavior if we decide to keep it as is.

Maybe @chrismccord or @josevalim have some input as well.

probably-not commented 4 months ago

Yes, we could generate a hook for this that duplicates the fallback and updates the title, but I feel like if we indeed decided that this is should be the default behavior, it shouldn't need a hook (?).

I think this is why making it as part of the generated code is a good compromise - you get the hook, and you can remove/change it if necessary. The same as the CoreComponents that are currently generated as part of each project - they're generated so that users who want to change them can change them, but they're written with best practices already in mind so that you don't need to change them if you think the best practices are good enough for you.

The least we should do is document the current behavior if we decide to keep it as is.

Definitely agreed 👍 documenting the behavior (whatever it is decided it should be) should be a must.