-
Notifications
You must be signed in to change notification settings - Fork 893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some containers do not receive the phx-no-feedback
class even though they have the phx-feedback-for
properly defined
#3071
Comments
Hi, have you tried the latest main? There were some changes recently concerning the phx-no-feedback handling that are not released yet. |
@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). |
@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. :) |
Just an idea, but maybe it's an escaping problem of the feedback names; can you try the following?
|
Unfortunately, it's not. |
I'm going to try, although it's always the same elements that don't receive the class. |
Ok, I found what it takes to replicate the bug. It's the presence of the new The form element in question (the one that won't receive
This overrides the default So, in my case the following is the problematic child of the element that should have received the
rendering as:
which does not work. However, if I replace the component with the rendered code but without the
The thing is I prefer to keep on leveraging the |
When you trigger the bug, what does the DOM look like for the input element? |
As I wrote above:
|
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! |
Ok, will do it tomorrow (now it's getting late on the side of the world :) |
I haven't reproduced, but I have a guess on what might be the cause. Can you try the |
YES! It works. Thanks, Chris! |
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 |
Ok, the first difference I can see is that in this other case the 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 |
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. |
Yeah 0.20.4 only has the partial fix. If you can provide a repro it would really help me fully solve it. Thanks! |
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. |
@SteffenDE Do you have a working single file example with Tailwind? |
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 |
danke schön |
can you try
I think that we found the root cause. |
Nope. The ancestor element in question is still not receiving the |
Chris wants to wait before merging this because phx-feedback-for will be removed in one of the next releases. |
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. |
@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. |
@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? |
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)? |
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). |
@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. |
Well, I didn't know that.
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.
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? |
@josevalim Speaking of which, can you tell me now what the |
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.
Yes, the issues tracker. Following Phoenix and LiveView should be enough. In this case: phoenixframework/phoenix#5713 |
@DaTrader 0.20.5 is out now :) |
@SteffenDE Unfortunately, the bug has not been fixed but worsened as previously mentioned here: #3071 (comment). This indicates that it wasn't PS. You can try it on the single file demo app and see for yourself. |
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. |
Yes, now I see it's working with this script, but when can we expect to have it merged into |
PS. The v0.20.5 does have its "bump build" |
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. |
Ok. Now I am not following any more. I have the following dependency in the 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? |
Can you describe what "does not work" means? (LV 0.20.5 should be equal to apply_feedback_permanently_assets) |
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: They are obviously not the same. |
To summarize it in a single comment.. Does not work: Works: Meaning: they are not the same thing |
e3071.movWell, 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. |
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 <div data-phx-id="ResourceLive-20" phx-feedback-for="form_composition[amount]" class="relative h-[54px] flex flex-col border-b input-border [&: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> |
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 :( |
Yeah, I guess I'll have to. |
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 Start the single file demo app and inspect the relevant |
@DaTrader you can try the fix with
or
|
@SteffenDE It works finally. Well done! |
Environment
b2c8999aaccf07509bf2f119f7a23d14269bfa26
vsbed2ff05bb024b2927fb9c1523e42ca55627715f
"Speed up patching 5-10x (Speed up patching 5-10x #2845)"Actual behavior
Some elements (containers in
phx-feedback-for
parlance) no longer receive thephx-no-feedback
class when they should i.e. when thephx-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 :
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" in5a3368da026ea011c2f6790bdd7045ce216402ec
but when going in reverse it will be there inbed2ff05bb024b2927fb9c1523e42ca55627715f
where the relevant changes tophoenix_live_view.js
were first made.Expected behavior
All elements with the
phx-feedback-for
attribute properly set should receive thephx-no-feedback
class.The text was updated successfully, but these errors were encountered: