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

ElementInternals does not contain form if denoted by form attribute #89

Open
lennartbuit opened this issue Nov 16, 2022 · 8 comments
Open
Assignees
Labels
bug Something isn't working

Comments

@lennartbuit
Copy link

lennartbuit commented Nov 16, 2022

Buttons can associate to forms that they are a descendant of, or forms that have an id specified by the form attribute.

So can form associated custom buttons in Chrome (107), see for example this sandbox: https://codesandbox.io/s/loving-lena-p56owk?file=/index.html . Both the button inside the form, as well as the one specified by the form attribute can find the form it is associated to. (printed to the console on click).

However, in browsers that require this polyfill, such as Safari, only the form associated button that is a descendant of the form can properly find its form.

@calebdwilliams calebdwilliams added the bug Something isn't working label Nov 17, 2022
@calebdwilliams
Copy link
Owner

Good callout. Looks like this works generally for all APIs as well. I'll have to poke around at the best way to solve this unless you'd like to take a shot at it.

@lennartbuit
Copy link
Author

I added a workaround to our internal DS, if this._internals.form is unavailable, try document.getElementById(this.form). But I know too little of the specs to know whether that is really correct.

@calebdwilliams
Copy link
Owner

Yeah, my concern is basically wanting to confirm how this works if the form attribute changes midstream and how any values or behavior pivot.

@calebdwilliams
Copy link
Owner

Yeah, here's an example of what I was worried about. Although it might be as simple as forwarding the test attribute to the hidden input … but there are likely other side effects to worry about like event listeners, etc.

I'll play with it, but it won't be a fast fix.

@calebdwilliams calebdwilliams self-assigned this Nov 17, 2022
@lennartbuit
Copy link
Author

Thats alright; I think my naive workaround is enough for the foreseeable future. Let me know if theres anything I can do for you ^^.

@joelabrahamsson
Copy link

I just ran into this issue. I have a form which is submitted by a button which doesn't reside inside the form.
Maybe I'm naive but wouldn't a simple solution be to change the event listener in wireSubmitLogic to listen for the forms submit event instead of for clicks on buttons?

@calebdwilliams
Copy link
Owner

The problem is more how to tell when the form changes. It would be feasible for a developer to do something along the following lines:

html`
  <form action="one" id="one">
    <button type="submit">Continue</button>
  </form>

  <form action="two" id="two">
    <button type="submit">Continue</button>
  </form>

  <x-control form=${someVariable ? 'one' | 'two'}></x-control>
`;

When the value of someVariable changes we'd need to reinitialize all of the work. You might be right about the wireSubmitLogic code, I'll have to go back and see why/when that was changed because I remember it being a gap in my original thinking on this polyfill.

I am taking some time off from writing code over the holiday season as I am recharging before starting my next role in January but I might get bored and try to take this on later. If someone else would like to claim it, let me know.

@enjoythelive1
Copy link
Contributor

I was reading the spec for form associated elements and it seems to not be that complicated.

Basically, when the from attribute changes or the id of any element in the tree changes (I imagine document or shadow dom where the element is), then we have to recompute the form the element belongs too.

So, we could follow the spec. It would require extending what the mutation observer is observing, and it is not that simple, but it is doable.

If you guys agree, I could try making a PR to solve this issue, following the approach listed in the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants