Form with id field

[kobutri]

Here is a kind of fun bug.
line 477:

/ If oldElt has an `id` with possible state and it doesn't match newElt.id then avoid morphing.
// We'll still match an anonymous node with an IDed newElt, though, because if it got this far,
// its not persistent, and new nodes can't have any hidden state.
(!oldElt.id || oldElt.id === newElt.id)

When oldElt or newElt is a form element with an input field that has name="id", then elt.id will return the input field.
This should probably be !oldElt.getAttribute("id") || oldElt.t.getAttribute("id") === newEltt.getAttribute("id")

Edit: Clarified that it's elt.id not elt.input.

[botandrose]

@kobutri Hello, thanks for bringing this to our attention! It is definitely interesting, but I'm not clear on how this can cause a problem in practice. We're not calling elt.input anywhere, for example. Is this a bug you ran into while using Idiomorph? If so, can you share a Codepen or etc demonstrating the circumstances that trigger it?

[botandrose]

@kobutri Oh, I think I see. You're saying something like:

<form id="post_form">
  <input name="id">
</form>

document.getElementById("post_form").id // returns the <input> Element, rather than "post_form"

Is that right? Let me try to verify. If so, that would be very surprising and yes could be a source of bugs.

[botandrose]

Confirmed! https://jsbin.com/qijidezuxi/edit?html,js,output

JavaScript never ceases to yield new dumb surprises :) Thanks for letting us know about this. I'll get a fix out the door soon.

[MichaelWest22]

https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input#name check the red warning box note at the bottom of name section.

All inputs names become properties of the form and override other properties of the form in javascript. Pretty stupid feature! This means that if you plan on using any javascript or libraries then you should never name your inputs the same as a form elements properties. It us not practical to resolve this in a javascript library for every property name we have to use and trying to work around it for one value will just cause issues for users when it breaks the next library they add.

[botandrose]

@MichaelWest22 I hear you on this being a footgun, and I agree that in an ideal world, people simply shouldn't set themselves up for weirdness by using reserved property names as form input names.

However, the list of property names on Element is quite lengthy, especially when including those inherited from its ancestors. So its not as obvious as it may seem whether you're using one or not. Also, I'm more of a realist than an idealist, so I want to accommodate the world that exists, not the one that I wish existed.

Looking into fixing this, I don't think it really costs us much to swap our our usage of .id for .getAttribute("id"), if there are no perf implications, or subtle semantic differences. It is a bit wordier, but it is also slightly more intention-revealing.

It also makes me wonder if this could technically happen with any other properties of element.

[botandrose]

Thinking about this some more, this bug seems relatively likely to affect a fair porportion of Rails users. Rails uses Turbo by default, which uses Idiomorph under the hood. In Rails, the default resource routes end with a parameter named "id". So if you ever want to use this parameter in a form, you'll hit this bug!

I'll have to expose it in a test case, but I expect that the main fallout from this is that the <form> won't be considered persistent, and that may have more serious consequences.

More exploration needed.

[kobutri]

Thanks for taking up this issue. I can't speak about rails, but I hit on it in htmx.
I have a dashboard that refreshes every couple of seconds an noticed that my input fields were loosing focus and their values with every refresh.
I think it would be very advisable to fix this in the library and not just defer to the spec.
The bug can surface in a very subtle manner and is pretty hard to spot. Even after I found the issue I thought for quite some time that htmx or something was messing up my form element rather than this just being a web API quirk.

[botandrose]

Okay, I've got a WIP PR up with a failing test exposing the issue, and a proof-of-concept commit that fixes it, but unfortunately also has a roughly ~5% hit to perf. I'm going to see if I can't figure out a way to fix this without perf implications. Discussion will continue there: #136