Some containers do not receive the phx-no-feedback class even though they have the phx-feedback-for properly defined

[DaTrader]

Environment

• Elixir version (elixir -v): 1.14.4
• Phoenix version (mix deps): 1.7.10
• Phoenix LiveView version (mix deps): 0.20.1 b2c8999aaccf07509bf2f119f7a23d14269bfa26 vs bed2ff05bb024b2927fb9c1523e42ca55627715f "Speed up patching 5-10x (Speed up patching 5-10x #2845)"
• Operating system: Linux Mint
• Browsers you attempted to reproduce this bug on (the more the merrier): Chrome, FF
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

Actual behavior

Some elements (containers in phx-feedback-for parlance) no longer receive the phx-no-feedback class when they should i.e. when the phx-feedback-for is present with the proper form field name.

I've tried hard to detect the pattern i.e. to find in what way these elements are defined differently than those that still receive the class but so far without success - meaning that at this moment I can't write a demo app replicating this, that is for as long as I can't tell what I am supposed to replicate.

So far I've tried the following (both isolated and in combo with others) and no combo managed to circumvent the bug :

• removing the element id
• render element unconditionally
• make the element class a static string (as opposed to composed dynamically)
• make the phx-feedback-for unconditional static string (as opposed conditioned by the assigns)

I've noticed the bug first when migrated from 0.20.1 to 0.20.3 and then dived into finding the exact commit. If starting from checking out b2c8999aaccf07509bf2f119f7a23d14269bfa26 which is the last commit without the bug , the bug will not manifest until the "Bump build" in 5a3368da026ea011c2f6790bdd7045ce216402ec but when going in reverse it will be there in bed2ff05bb024b2927fb9c1523e42ca55627715f where the relevant changes to phoenix_live_view.js were first made.

Expected behavior

All elements with the phx-feedback-for attribute properly set should receive the phx-no-feedback class.

[SteffenDE]

Hi, have you tried the latest main? There were some changes recently concerning the phx-no-feedback handling that are not released yet.
If it happens there as well we'd need some minimal example to reproduce. https://github.com/wojtekmach/mix_install_examples/blob/main/phoenix_live_view.exs is a good starting point. Thank you!

[DaTrader]

@SteffenDE Putting up a one-file application is not an issue here. The issue is I did try the latest main and the bug is there. But the worst part is, as I said in the report above, I don't know how to replicate it because I can't see what's different in the elements that receive the class as opposed to ones that don't (in my templates).

[josevalim]

@DaTrader you should be able to reproduce it by yanking elements from the page and the LiveView, one by one while the error occurs, until it stays minimal, and then copy that to the single file app. :)

[SteffenDE]

Just an idea, but maybe it's an escaping problem of the feedback names; can you try the following?

{:phoenix_live_view, github: "SteffenDE/phoenix_live_view", branch: "escape_feedback_names_assets", override: true}

[DaTrader]

Just an idea, but maybe it's an escaping problem of the feedback names; can you try the following?

Unfortunately, it's not.

[DaTrader]

@DaTrader you should be able to reproduce it by yanking elements from the page and the LiveView, one by one while the error occurs, until it stays minimal, and then copy that to the single file app. :)

I'm going to try, although it's always the same elements that don't receive the class.

[DaTrader]

Ok, I found what it takes to replicate the bug. It's the presence of the new data-phx-id in a <.input type="hidden"/>.

The form element in question (the one that won't receive phx-no-feedback) is the parent of the child for which I am using the following function component added to CoreComponents:

def input( %{ type: "hidden"} = assigns) do
  ~H"""
  <input
    type={@type}
    name={@name}
    id={@id}
    value={Phoenix.HTML.Form.normalize_value(@type, @value)}
    {@rest}
  />
  """
end

This overrides the default <.input type="hidden"/> which would otherwise end up wrapping up the input field into a container with a label, the opinionated styling and errors which I found unnecessary. Besides, all this is not so important because I tried your default CoreComponents implementation and it does not work either.

So, in my case the following is the problematic child of the element that should have received the phx-no-feedback:

<.input type="hidden" field={@post_form[:media_url]}/>

rendering as:

<input data-phx-id="ResourceLive-115" type="hidden" name="post_composition[media_url]" id="post_composition_media_url">

which does not work. However, if I replace the component with the rendered code but without the data-phx-id, it does work again:

<input type="hidden" name="post_composition[media_url]" id="post_composition_media_url">

The thing is I prefer to keep on leveraging the field attribute and not resolve the name and the id manually for hidden inputs.

[chrismccord]

When you trigger the bug, what does the DOM look like for the input element?

[DaTrader]

As I wrote above:

<input data-phx-id="ResourceLive-115" type="hidden" name="post_composition[media_url]" id="post_composition_media_url">

[chrismccord]

Can you wrap this up in a minimal reproduction? Tracing our code right now it's not clear to me how to reproduce the issue. Thanks!

[DaTrader]

Ok, will do it tomorrow (now it's getting late on the side of the world :)

[chrismccord]

I haven't reproduced, but I have a guess on what might be the cause. Can you try the cm-fix-feedback branch? Thanks!

[DaTrader]

YES! It works. Thanks, Chris!

[DaTrader]

I hate to break the party, but now I see it works only in one case. The other is still not working properly.

Will study it now to see what's different

[DaTrader]

Ok, the first difference I can see is that in this other case the <.input type="hidden"/> is a child of a button that is a child of the element that should have received the phx-no-feedback, so no longer a direct child.

Yup, all else is the same. Virtually identical input element in the DOM like in the previous example. The only other difference being that in this case the ancestor element that should've received the 'phx-no-feedback' class does not have its id defined by me, but is provided a data-phx-id by the LiveView for it too is a function component.

[DaTrader]

Just upgraded to 0.20.4 and this last issue with a non-direct child is still there.

The first one with a direct child stays fixed - thank you for that.

[chrismccord]

Yeah 0.20.4 only has the partial fix. If you can provide a repro it would really help me fully solve it. Thanks!

[DaTrader]

I am going to. Currently I am trying to pinpoint yet another client-side bug that was introduced post 0.20.3. When I report that issue, I'll get on the repros for this one and the other one I reported a couple of days ago.

[DaTrader]

Hi, have you tried the latest main? There were some changes recently concerning the phx-no-feedback handling that are not released yet. If it happens there as well we'd need some minimal example to reproduce. https://github.com/wojtekmach/mix_install_examples/blob/main/phoenix_live_view.exs is a good starting point. Thank you!

@SteffenDE Do you have a working single file example with Tailwind?

[SteffenDE]

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.0"},
  {:phoenix_live_view, github: "phoenixframework/phoenix_live_view", branch: "main", override: true}
])

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

  defp phx_vsn, do: Application.spec(:phoenix, :vsn)
  defp lv_vsn, do: Application.spec(:phoenix_live_view, :vsn)

  def render("live.html", assigns) do
    ~H"""
    <script src={"https://cdn.jsdelivr.net/npm/phoenix@#{phx_vsn()}/priv/static/phoenix.min.js"}></script>
    <script src={"https://cdn.jsdelivr.net/gh/phoenixframework/phoenix_live_view@main/priv/static/phoenix_live_view.min.js"}></script>
    <script src="https://cdn.tailwindcss.com"></script>
    <script>
      let liveSocket = new window.LiveView.LiveSocket("/live", window.Phoenix.Socket)
      liveSocket.connect()
    </script>
    <style>
      * { font-size: 1.1em; }
    </style>
    <%= @inner_content %>
    """
  end

  def render(assigns) do
    ~H"""
    <%= @count %>
    <button phx-click="inc" class="bg-blue-500 text-white p-4">+</button>
    <button phx-click="dec" class="bg-blue-500 text-white p-4">-</button>
    """
  end

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

  def handle_event("dec", _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)

There you go

[DaTrader]

danke schön

[SteffenDE]

can you try

{:phoenix_live_view, github: "SteffenDE/phoenix_live_view", branch: "fix_skipped_feedback_assets", override: true}

I think that we found the root cause.

[DaTrader]

Nope. The ancestor element in question is still not receiving the phx-no-feedback.

[SteffenDE]

Why haven't you PR-ed just the extract_feedback_assets for that one did work properly?

Chris wants to wait before merging this because phx-feedback-for will be removed in one of the next releases.

[DaTrader]

Common guys, when is the end to the breaking changes?

[SteffenDE]

Common guys, when is the end to the breaking changes?

The answer is: when LiveView reaches 1.0. For this change there will also be a drop in solution for existing applications, no worries. And we will of course make sure that it works the way that it should.

[josevalim]

@DaTrader when 1.0 is out, by definition.

However, the goal is to also give developers a snippet that keeps phx-feedback-for functionality if they want to.

[DaTrader]

How can I reproduce this? The single file example does not show this as far as I can see? Using <script src={"https://cdn.jsdelivr.net/gh/SteffenDE/phoenix_live_view@apply_feedback_permanently_assets/priv/static/phoenix_live_view.js"}></script> in there seems to work fine?

@SteffenDE This JS works. Can you please make a commit that has this script within a dep so I don't need to update apps in two different places?

[DaTrader]

Chris wants to wait before merging this because phx-feedback-for will be removed in one of the next releases.

Also, this makes me wonder why we spent so much time on this bug in the first place (i.e. you could've told me that before)?

[josevalim]

Because these problems were the catalyst for looking for a new solution. If you believe you have put much time, please consider how much time we have put too, developing, maintaining, and fixing these bugs.

Also note that, whenever we introduce a new approach, we do our best to keep the old ways working for quite some time to ease migration. If you prefer, we can just introduce the new approach and yank the old permanently forever. It would definitely be less work on our side (and perhaps we could have launched 1.0 sooner if we really didn't care about backwards compatibility).

[SteffenDE]

@SteffenDE This JS works. Can you please make a commit that has this script within a dep so I don't need to update apps in two different places?

@DaTrader 0.20.5 will be released soon (probably today), please try this one first, it will include the exact code that is present in the script from apply_feedback_permanently_assets.

[DaTrader]

Because these problems were the catalyst for looking for a new solution.

Well, I didn't know that.

If you believe you have put much time, please consider how much time we have put too, developing, maintaining, and fixing these bugs.

I have never questioned that. The point is very simple here. If you know you're going to discard/completely change something, please advertise the decision in advance so we (the LV users) can make our decisions accordingly.

If you prefer, we can just introduce the new approach and yank the old permanently forever.

Is there a way to gain access to these observations and decisions as soon as they happen? I believe I asked this once before and was referred to "what was (or will be) said on a conference x". Do you have a chatroom or something where you discuss these things?

[DaTrader]

@josevalim Speaking of which, can you tell me now what the phx-feedback-for will be substituted with in a future release?

[josevalim]

If you know you're going to discard/completely change something, please advertise the decision in advance so we (the LV users) can make our decisions accordingly.

As said above, "whenever we introduce a new approach, we do our best to keep the old ways working for quite some time to ease migration". Plus you have been told in advance. The feature is in a pull request for everyone to see and it has not been merged yet.

Is there a way to gain access to these observations and decisions as soon as they happen?

Yes, the issues tracker. Following Phoenix and LiveView should be enough. In this case: phoenixframework/phoenix#5713

[SteffenDE]

@DaTrader 0.20.5 is out now :)

[DaTrader]

@SteffenDE Unfortunately, the bug has not been fixed but worsened as previously mentioned here: #3071 (comment).

This indicates that it wasn't extract_feedback_asset (that does work) that was applied but apply_feedback_permanently_assets (or both?)

PS. You can try it on the single file demo app and see for yourself.

[SteffenDE]

How can I reproduce this? The single file example does not show this as far as I can see? Using <script src={"https://cdn.jsdelivr.net/gh/SteffenDE/phoenix_live_view@apply_feedback_permanently_assets/priv/static/phoenix_live_view.js"}></script> in there seems to work fine?

I still cannot reproduce what you describe. I'm using your single file example and changed the dependency to 0.20.5 and the script tag to use LiveView main. If I type into the field and then on the invisible button the border stays white. If I click the invisible button without typing into the field, the border stays white. I cannot get the border to become red.

[DaTrader]

Yes, now I see it's working with this script, but when can we expect to have it merged into main?

[DaTrader]

PS. The v0.20.5 does have its "bump build"

[SteffenDE]

I'm sorry, but I'm not sure if I can follow. You're saying that it works with the script. The script is using LV 0.20.5. So if you are using LV 0.20.5 in your project you will have the same behavior as the single file script. If you are still seeing an issue with phx-feedback-for, I think we will need another reproduction.

[DaTrader]

Ok. Now I am not following any more.

I have the following dependency in the Mix.install call: {:phoenix_live_view, "0.20.5", override: true} and the following link script link (by default): <script src={"https://cdn.jsdelivr.net/gh/phoenixframework/phoenix_live_view@main/priv/static/phoenix_live_view.min.js"}></script>

The above does not work.

However, if when I replace the script with the one you just provided again, it does work. Shouldn't this script that you provided and that works be in the v0.20.5 by default i.e. without the need to link it additionally?

[SteffenDE]

Can you describe what "does not work" means? (LV 0.20.5 should be equal to apply_feedback_permanently_assets)

[DaTrader]

The single file app does not work (the border turns red after entering something in the white text field and clicking the invisible button) with the default script, but works with the one you just re-quoted: <script src={"https://cdn.jsdelivr.net/gh/SteffenDE/phoenix_live_view@apply_feedback_permanently_assets/priv/static/phoenix_live_view.js"}></script>

They are obviously not the same.

[DaTrader]

To summarize it in a single comment..

Does not work: <script src={"https://cdn.jsdelivr.net/gh/phoenixframework/phoenix_live_view@main/priv/static/phoenix_live_view.min.js"}></script>

Works: <script src={"https://cdn.jsdelivr.net/gh/SteffenDE/phoenix_live_view@apply_feedback_permanently_assets/priv/static/phoenix_live_view.js"}></script>

Meaning: they are not the same thing

[SteffenDE]

e3071.mov

Well, then something else must be going on, as I still cannot reproduce this. As you can see in the video I am using LV 0.20.5 and the script from main. The border does not get red. I tried this in Arc (Chromium) and Safari.
Maybe you still get a cached version of the old script from main? Can you try to download the file directly from GitHub and use it locally (e.g. place the content directly in the script tag)

[DaTrader]

You are correct about the demo app. It works. It was my browser's cache. However, in my real app it still does not work.

These are the moments I wish I could copy paste the screen shot. Here's the exact rendered element in my real app with just the name of the form changed so it does not reveal the use case particularities. The issue is still there in both Chrome and FF and after deleting both the build and deps and getting the v.0.20.5 deps all over and recompiling everything.

<div data-phx-id="ResourceLive-20" phx-feedback-for="form_composition[amount]" class="relative h-[54px] flex flex-col border-b input-border  [&amp;:not(.phx-no-feedback):not(:focus-within):not(:hover)]:border-system-error">
  <label for="form_composition_amount" class="textDetailsM text-dark50">Enter point amount</label>
  <input type="text" name="form_composition[amount]" id="form_composition_amount" class="absolute w-full left-0 top-[25px] textMedium p-0 bg-transparent border-0 outline-0 focus:ring-0" maxlength="13" phx-debounce="300">
  <div data-phx-id="ResourceLive-21" class="phx-no-feedback:hidden  absolute whitespace-nowrap left-0 top-[56px] textDetailsM text-system-error">
    <p>Please provide an amount</p>
  </div>
</div>

[SteffenDE]

If you can send a new reproduction I'll be happy to take another look. Currently I have no clue what could still be wrong, sorry :(

[DaTrader]

Yeah, I guess I'll have to.

[DaTrader]

@SteffenDE

Can you describe what "does not work" means? (LV 0.20.5 should be equal to apply_feedback_permanently_assets)

I found the difference between the two and why it works on v0.20.1 and not on v0.20.5+.

The reason is that on v0.20.1 (your { :phoenix_live_view, github: "SteffenDE/phoenix_live_view", branch: "0.20.1_fixes", override: true}) the phx-no-feedback was still automatically added to all elements with the phx-feedback-for attribute even if nothing's been entered in the form yet, while in the single app demo that only seems to work (because of the border color) it isn't so until something is entered and then only when LV detects first some input. In my real app, this is a problem because I set one of the form values programmatically (which then results in form errors for other fields which are supposed to be suppressed by the phx-no-feedback). This used works with your v0.20.1 fix because it unquestionably obeys the phx-feedback-for attribute while the v0.20.5+ does not.

Start the single file demo app and inspect the relevant data-its-this-one element and you'll see it does not have the phx-no-feedback class added by default (despite the phx-feedback-for attribute.

[SteffenDE]

@DaTrader you can try the fix with

    <script src={"https://cdn.jsdelivr.net/gh/SteffenDE/phoenix_live_view@feedback_node_added_assets/priv/static/phoenix_live_view.min.js"}></script>

or

{:phoenix_live_view, github: "SteffenDE/phoenix_live_view", branch: "feedback_node_added_assets", override: true}

[DaTrader]

@SteffenDE It works finally. Well done!