Phoenix_live_view: Form errors from a `phx-focus` event are not displayed due to presence of `phx-no-feedback` class

[justincjohnson]

Environment

• Elixir version (elixir -v):

Erlang/OTP 23 [erts-11.0.3] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe]

Elixir 1.10.4 (compiled with Erlang/OTP 21)

• Phoenix version (mix deps): 1.5.5
• Phoenix LiveView version (mix deps): 0.14.7
• NodeJS version (node -v): v12.16.1
• NPM version (npm -v): 6.14.8
• Operating system: macOS Catalina 10.15.6
• Browsers you attempted to reproduce this bug on (the more the merrier): Chrome, Brave, Safari
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

Actual behavior

When an input field with phx-focus is focused and the corresponding event assigns a changeset with validation errors to the socket, the corresponding errors are not displayed due to the continued presence of the phx-no-feedback class on the errors.

Expected behavior

I expect the phx-no-feedback class to be removed from the input field I had focused.

Details

• I have an example repo at https://github.com/justincjohnson/sandbox.
• Run mix setup followed by mix phx.server and visit http://localhost:4000/.
• You will see a simple form with 3 input fields, first name (required), last name (required), and email (optional), backed by an embedded Ecto schema.
• The form as a whole has a phx-change binding and the input fields have a phx-debounce value of 1000 to cause form validation to run after 1 second.
• This works nicely but I also want the validations to run 1 second after any required fields are focused, or after they are blurred. So I added phx-focus bindings to first name and last name.
• The corresponding phx-focus validation events fire correctly and I can see the errors being injected into the DOM, but the phx-no-feedback class is not removed from the errors, so they don't appear.
• I'm not familiar with LiveView's JS code, but I set up a local LiveView dev env and pointed to that version of LiveView so I could debug a little further. I gather that discardError runs and based on the conditional at https://github.com/phoenixframework/phoenix_live_view/blob/713a3d4f25ee03ef0f930d132b6cc6f0b23c3217/assets/js/phoenix_live_view.js#L1197 might add the phx-no-feedback class.
• Should this.private(input, PHX_HAS_FOCUSED) be true at this point in the code? It is not for this use case and therefore the class is added, making the errors invisible. If I set it to true just before the conditional, the error shows up as expected since the phx-no-feedback class is not added.
• Per the docs at https://github.com/phoenixframework/phoenix_live_view/blame/713a3d4f25ee03ef0f930d132b6cc6f0b23c3217/guides/client/form-bindings.md#L89-L91 it seems that the phx-no-feedback class should only be added in cases where the form fields has yet to receive user input/focus. Is it possible that the focus part of that isn't being handled properly in the code?

Thank you for your time.

[justincjohnson]

The changes I made at https://github.com/justincjohnson/phoenix_live_view/commit/615666be030089338116b866173fd0ed30030c5b seem to fix the problem for me, but again, note that I'm not familiar with this code. 😬 Also this wasn't an attempt to make beautiful code.

[justincjohnson]

As an update, I still have this problem in 0.14.8.

[achedeuzot]

As mentioned on #1135, this bug still occurs on 0.15.0.

I'm pretty sure, I've pinponted the issue to the PHX_HAS_SUBMITTED that remains undefined even after a submit. It requires a touch to one of the form fields for the value to be correctly set to true.

[achedeuzot]

Hi @justincjohnson, as mentionned in #1135, I fixed the issue by setting the action to :insert instead of :validate.

In your sample project, I see you're using Ecto.Changeset.apply_action(changeset, :validate) when the form is submitted. Try replacing the action to apply by :insert, it also might fix your issue 🤞

[justincjohnson]

@achedeuzot nope, that doesn't change anything for me. Thanks for the suggestion though. 🤝

[justincjohnson]

Would it be possible for someone from the core team for this project to comment on this? I don't ask that in an entitled manner. 🙏🏻 I'd just like to know if this is a legit issue and if I'm heading in the right direction. If so, I'll gladly attempt to dive in further and fix the bug, but since I'm new here it would be nice to get some confirmation I'm not missing something obvious. Thank you. 🙇🏻

[chrismccord]

We haven't had a chance yet to dive into this one. In general, if it's on the issue tracker and open, it's on our list. Hang tight! :)

[mveytsman]

I'm pretty sure that this is fixed by https://github.com/phoenixframework/phoenix_live_view/commit/95d5c7ccd0ac66e04b15c7b6128d44b60767e682 !

@justincjohnson can you confirm that the issue no longer exists when you use LiveView from master?

[josevalim]

Looking at the description it should be indeed, thanks @mveytsman! :green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: I will reopen if it isn't!

[justincjohnson]

This is still broken for me. 😭

I pushed changes to my sandbox repo above to use a local clone of phoenix_live_view sitting at the same level as my sandbox clone. I started completely clean and confirmed with npm and mix that I am running the local version and not some other cached version. The latest revision in my clone of phoenix_live_view is the fix you mention @mveytsman. When I run my sandbox repo though, if I click in the first name field or the last name field, I can see in the DOM that the validation error shows up but still with the phx-no-feedback class.

Am I doing something wrong?

Thanks for the comment on the ticket regardless of what the outcome is here.

[josevalim]

@justincjohnson can you please try again on latest master? I forgot to update priv/static/phoenix_live_view.js before but I just did that now. Sorry about that.

[justincjohnson]

I'm still getting the error @josevalim. Is this the right way to reference a local JS dependency for phoenix_live_view? https://github.com/justincjohnson/sandbox/blob/master/assets/package.json#L12

[josevalim]

@justincjohnson i believe it should be ../deps/phoenix_live_view.js in this case and you use phoenix_live_view master as a git dep.

[justincjohnson]

From a look at the JS it seems I was doing the right thing, as I saw the definition of the new showErrors function and the calling of the function. Never the less, I tried again from github and still get the same results. I've pushed my changes to my repo, so reproducing should be easy there now.

[justincjohnson]

Is it possible that focus needs to be added to https://github.com/phoenixframework/phoenix_live_view/blob/master/assets/js/phoenix_live_view.js#L1227?

According to https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/input_event

The input event fires when the value of an <input>, <select>, or <textarea> element has been changed.

And according to https://developer.mozilla.org/en-US/docs/Web/API/Element/focus_event

The focus event fires when an element has received focus. The main difference between this event and focusin is that focusin bubbles while focus does not.

[justincjohnson]

My last comment was too simplistic. I was drawn there though because it is the only place where we set PHX_HAS_FOCUSED to true, and if I'm reading it right, it will only occur if there is an actual change, which matches the behavior I'm seeing.

[dvic]

I was drawn there though because it is the only place where we set PHX_HAS_FOCUSED to true, and if I'm reading it right, it will only occur if there is an actual change, which matches the behavior I'm seeing.

This is something that also confused me when reading the source code, maybe we can rename the PHX_HAS_FOCUSED constant to PHX_HAS_CHANGED (or something along those lines)?

[mveytsman]

@justincjohnson I think you're correct here! I think we should set PHX_HAS_FOCUSED when the element has focused as that seems to be the intent -- as opposed to changing the name.

[mveytsman]

@justincjohnson As I said I do think there's an issue with not marking PHX_HAS_FOCUSED correctly, but I am having some trouble understanding the intentions of your code re-read your initial bug report. Do you mind walking me through again what your expected behavior is?

Do I understand correctly that we want

• individual elements to be validated when focuses, debounced by 1 seconds
• the form itself to be validated when an input is changed, debounced by 1 second

[justincjohnson]

@mveytsman I'll jump back into my example to remind myself of what works and doesn't. I think what I probably really want long term is to have individual fields validated on change or blur and the entire form validated on submit. There may be cases though where I want individual fields validated on focus with debounce, which I believe is where my example ended up after trying out random things to figure out what worked and didn't.

Does that answer your question?

Thanks.

[justincjohnson]

I assume you know all of what I say here, but for clarity and to make sure we're on the same page with why my sandbox repo is set up the way it is...

If I ignore the problem this issue is about (with the phx-no-feedback class), I see that I am able to add either phx_focus or phx_blur or both to an input field and the corresponding validation function is called on focus and blur of that field as expected.

A phx_focus event with a debounce will fire either on focus after debounce expires or on blur if the blur happens before debounce expires. This matches the behavior I would desire.

Adding phx_change to input elements does nothing, as the change event only occurs on the form. So this is why error_tag in error_helpers.exs uses phx_feedback_for to control which fields have this phx-no-feedback class removed, as documented at https://hexdocs.pm/phoenix_live_view/form-bindings.html#phx-feedback-for.

My assumption is that this last bit about phx_change only working for an entire form is the only use case that was in mind when the phx-no-feedback related code was written, which is why it only addresses change events.

Since only the change event was leading to any removal of the phx-no-feedback class for me, that is why I had phx_change on the entire form even though I was validating individual fields. At least this gave me the desired validation when I changed something.

Note that I pushed changes to my sandbox repo to remove the use of phx-change on the form though and to make it more consistent, since in my testing I wasn't paying much attention to the email field previously.