LiveView container option doesn't handle lists of classes

[markquezada]

When using the container: option when useing liveview, we mistakenly used a list for multiple class names, like so:

use Phoenix.LiveView,
  layout: {MyAppWeb.Layouts, :app},
  container: {:div, class: ~w(h-full overflow-hidden print:overflow-visible)}

The docs show it as a string but does not explicitly state it has to be. Since heex accept lists of classes for tags, we mistakenly used a list instead of a string and saw odd results.

Environment

• Elixir version (elixir -v): Elixir 1.14.0 (compiled with Erlang/OTP 25)
• Phoenix version (mix deps): phoenix 1.7.0-rc.2
• Phoenix LiveView version (mix deps): phoenix_live_view 0.18.11
• Operating system: mac os 12.2.1
• Browsers you attempted to reproduce this bug on (the more the merrier): Chrome
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: N/A

Actual behavior

When the dom is updated via a push_navigate, the classes were joined with a comma on the live view container. E.g.: h-full,overflow-hidden,print:overflow-visible

Expected behavior

The classes should be rendered with spaces, e.g.: "h-full overflow-hidden print:overflow-visible"

Or, the docs should be updated to explicitly state a string is required.

[josevalim]

Can you please update the code to check it is a string and raise otherwise? And update the docs too? PR welcome. :)

[begedin]

I made a stab at this and ran into several problems, mostly due to my not reading through it fully.

This is only happening on push_navigate, not on initial page load

...so it might be warranted an actual "fix"

The initial page load actually renders a class list space-separated as one would expect. I verified this with a test, and manually. More details on what I actually did (nothing worth committing) further down.

It's only when you push_navigate or click a link with a redirect, when this happens

It's actually in the .js

More specifically, the issue gets created in dom.js, in replaceRootContainer

https://github.com/phoenixframework/phoenix_live_view/blob/master/assets/js/phoenix_live_view/dom.js#L405-L426

container.setAttribute(attrs, ['foo','bar']) will convert the array to a comma-separated string

I assume that's not where we want to fix it (or do we?), so we'd have to be converting to string from list on the backend. Some surface level investigation shows this could be done in channel.ex, in put_container/3

https://github.com/phoenixframework/phoenix_live_view/blob/8b60fa8c7d9b7618869d18ed0b3330a04a82498b/lib/phoenix_live_view/channel.ex#L1122-L1129

Or, possibly, some caller of that function.

A quick and dirty fix would look something like, but I'm sure it could be nicer.

defp put_container(%Session{} = session, %Route{} = route, %{} = diff) do
  if container = session.redirected? && Route.container(route) do
    {tag, attrs} = container

    attrs = Enum.into(attrs, %{})

    attrs =
      case attrs[:class] do
        nil -> attrs
        c when is_binary(c) -> attrs
        c when is_list(c) -> Map.put(attrs, :class, Enum.join(c, " "))
      end

    Map.put(diff, :container, [tag, attrs])
  else
    diff
  end
end

To me, this kind of makes sense, as providing class lists is kind of a thing in some frameworks out there, and Phoenix.HTML.tag/content_tag, already accepts a list as class attribute, as does the initial backend-based renderer for live view, as a consequence.

If we do want to validate the class is always given as a string, where to put it?

The one place where I see the container option getting validated is maybe a bit too generic and we'd have to get quite specific with the class

https://github.com/phoenixframework/phoenix_live_view/blob/d90f6915fdb4fa020c9f82f55e9bbf89e60b8a1b/lib/phoenix_live_view/router.ex#L388-L394

But then, would a string value have to be enforced on all attributes here, or only for the class, or a subset?

Alternatively, something could maybe be done in the before_compile of phoenix_live_view.ex, but there is no such validation there currently.

Extra 1: DOM/Floki and the js do not match behavior, but exhibit a similar "bug"

DOM.to_html({"span", [{"class", ["foo", "bar"]}], []})

this outputs <span class="foobar"></span>. So it's not comma-separated but rather joined using empty string. This is directly calling floki and I'm not really sure if it's standard or should be considered a bug.

Extra 2: How I did the testing

Click to expand

I added this to `support/general.ex` ```Elixir defmodule Phoenix.LiveViewTest.ClassListLive do use Phoenix.LiveView, container: {:span, class: ~w(foo bar)} def render(assigns), do: ~H|Some content| end ``` then this to `support/router.ex` first at root, later under the `:test` session, to confirm what's going on on navigation ```Elixir live "/classlist", ClassListLive ``` Then, finally, added this test to `live_view_test.exs` ```elixir test "live render with container giving class as list", %{conn: conn} do {:ok, _view, html} = live(conn, "/classlist") assert html =~ ~s|class="foo bar"| end ``` and it passes. The classes are rendered as space-separated I moved the above special route into the :test session and tried to write a test inside `navigation_test.exs`, in the `"live_redirect"` describe ```elixir test "to classlist", %{conn: conn} do assert {:ok, thermo_live, _} = live(conn, "/thermo-live-session") assert {:ok, classlist_live, html} = live_redirect(thermo_live, to: "/classlist") assert html =~ ~s|class="foo bar"| end ``` And this one fails. So only on redirect, it seems the class list is rendered joined with an empty string, so a `["foo", "bar"]` becomes "foobar". Note that I'm not seeing a join by comma separated strings. This is because we "fake" the update via Floki, rather than running jos.-