Skip to content
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

[LiveComponent] Allow form elements to live outside the form tag by using "form" attribute #2400

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from

Conversation

dsoriano
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
Issues
License MIT

Actually there is an issue if we want to use form elements that live outside the form tag and refer it by using the "form" attribute, i.e:

<input type="text" name="myinput" form="custom-form">
<form data-model="*" id="custom-form"></form>

this should work, especially to be able to handle nested forms (managed by others live components).

This PR fix this issue.

@carsonbot carsonbot added Bug Bug Fix LiveComponent Status: Needs Review Needs to be reviewed labels Nov 20, 2024
Copy link

📊 Packages dist files size difference

Thanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
Please review the changes and make sure they are expected.

FileBefore (Size / Gzip)After (Size / Gzip)
LiveComponent
live_controller.js 120.36 kB / 23.38 kB 120.45 kB0% / 23.4 kB0%

Comment on lines +164 to +165
const formId = element.getAttribute('form');
const formElement = formId ? document.getElementById(formId) : element.closest('form');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check getElementById(formId) returns a form element

@smnandre
Copy link
Member

I think i see where you want to go from here, but i'd like to see it (working) and not just merge this MR that has not shown its full impact yet.

One thing i'm not sure to understand for instance: in your test you call "parseDirective"... but I'm not sure how/when it would be call in real situation?

LiveComponent only what what's inside it, Stimulus controllers too, and the multiple belongsToComponent methods (or similar) looks to me to always scope things.

Do you think you could describe a precise case and then we'll see if and how it would work ?

@smnandre smnandre added Status: Waiting Feedback Needs feedback from the author and removed Bug Bug Fix Status: Needs Review Needs to be reviewed labels Nov 21, 2024
@smnandre
Copy link
Member

(i removed the bug label because it never was a documented feature in the first place :) )

@dsoriano
Copy link
Contributor Author

Hello,

the use case is relatively simple, the ability to use nested forms. In HTML this is forbidden:

<form>
    <input type="text" name="username">

    <form>
        <input type="number" placeholder="Add credits">
        <button type="submit">Add credits</button>
    </form>

    <button type="submit">Save</button>
</form>

To do that HTML standard allow to create form field outside the form tag by using the form attribute:

<input type="text" name="username" form="user-form">

<form>
    <input type="number" placeholder="Add credits">
    <button type="submit">Add credits</button>
</form>

<button type="submit" form="user-form">Save</button>
<form id="user-form"></form>

In LiveComponent, this is not possible, because updating a field outside the form tag doesn't enable the data-binding on the form because of const formElement = element.closest('form'); that constraint the element to be a child of the form.

<div {{ attributes }}>
    <input type="text" name="username" form="user-form">

    {# A nested component that contain another form tag #}
    {{ component('user_credits', {user: user}) }}

    <button type="submit" form="user-form">Save</button>
    <form id="user-form" data-model="*"></form>
</div>

There is a lot of cases where this is useful and this is the clean way to do that (I encounter this problem in my two last projects, especially in complex forms in admins for example).

My example is a good one: in the user edition, I want to see the user credits history, and want to be able to add an entry. Why not to use LiveCollection ? For two main reasons:

  • In this case, the history must not be editable. We can consult it but not update. We just want to add one entry.
  • In some cases LiveCollection can be usefull, but in anothers not: LiveCollection offers a poor UI experience by rendering all the entries editable with form fields. This is better to deal with that as a CRUD logic. But create a CRUD embed into a form tag is not possible.

Now the problem with my update you pointed:
I would love to do a component.getElementById(formId) to ensure the element refer a form tag embed into the component and keep the scope, but I don't have access to it in this function, that's why I used document

I hope my explanation was clear, not easy to describe 😂

I hesitated for the "bug" tag, but as LiveComponent does not take into account the entire specification of HTML in this case, I considered it was probably a bug.

I am available for further explanations if needed.

@smnandre
Copy link
Member

Will answer more if i can find time this week-end.

I get the use case, and it would be great to make this work, but.

<input type="text" name="username" form="user-form">

<form>
    <input type="number" placeholder="Add credits">
    <button type="submit">Add credits</button>
</form>

<button type="submit" form="user-form">Save</button>
<form id="user-form"></form>

This is not a good example to me, as this could 100% be written

<form>
    <input type="number" placeholder="Add credits">
    <button type="submit">Add credits</button>
</form>

<form id="user-form">
    <input type="text" name="username" form="user-form">
    <button type="submit" form="user-form">Save</button>
</form>

Now, there is one constraint here to acknowledge: form fields inputs must be descendant of the the live component root tag (and must not descendant of any other nested live component root tag)

So in this situation...

<div data-controller="live" id="A">

  <form id="form-a">
      <input type="text" form="form-a" name="input-a" />
  </form>
  
  <input type="text" form="form-a" name="foo-a />
  <input type="text" form="form-b" name="foo-b />
  
  <div data-controller="live" id="B">
      <input type="text" form="form-a" name="live-a" />
       <input type="text" form="form-b" name="live-b" />
      
       <form id="form-b">
          <input type="text" form="form-a" name="input-c" />
      </form>
  </div>

</div>

I call "After" a future where we implement the suggested changes.

Input Form LiveComp Today After
input-a inside same
foo-a outside same
foo-a outside different
live-a outside different
live-b outside same
input-c outside different

This would need some extensive testing.. and clarity in documentation to prevent frustrations or missunderstandings. But if you think "foo-a" and "live-b" are some real improvements, why not.

Again, as long we find time to test it 100% and ensure this change nothing for existing users, nor for nested components 👍

@smnandre smnandre added the Feature New Feature label Nov 22, 2024
@dsoriano
Copy link
Contributor Author

Yeah I understand, indeed the goal is to ensure that the form fields are in the same component that the form tag, I agree. Actually I'm not sure this can be done easily with the actual available functions. I will try to investigate more deeply about that.

I'm sure this is a goog improvement (and necessary in some cases). That's why I defend it 😉

I will try to improve this PR as soon as possible

@smnandre smnandre added Status: Needs Work Additional work is needed and removed Status: Waiting Feedback Needs feedback from the author labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature LiveComponent Status: Needs Work Additional work is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants