named form fields result in element duplication

[FND]

reduced test case: https://burningchrome.com/~fnd/_/19c4516b-8d17-4d8b-b732-7cfdf4ca49e8.html (→ view source, <script> at the bottom)

<input name="id"> results in the surrounding form being duplicated rather than replaced.

As far as I can tell, this is because if such a field exists within a form, form.id returns that DOM node rather than the form's id attribute value (because the 90s were a magical time): https://github.com/patrick-steele-idem/morphdom/blob/e4a34d934b24bb5aad1fcbc6222ed21bcdb5e56d/src/morphdom.js#L12-L14 I suppose it would suffice to change this to node.getAttribute("id") - but I'm insufficiently familiar with morphdom's innards to be sure.

[snewcomer]

@FND Great reproduction! Since in a form, the name field is what determines what is sent to the server and b/c these are form elements, the browser thinks you want an element in this form scope with "name" id. Form controls shouldn't be given a name that are the same as other form properties. Lastly, there is some sort of "shorthand access" for forms, so that is what I expect is going on as well.

This is a "hot path" function as well, so adding extra conditional checks does impose a small problem.

Closing for now but feel free to let me know if you have other thoughts.

[FND]

Thanks, @snewcomer. I'm afraid I might have been unclear before though, so please allow me to clarify:

 function defaultGetNodeKey(node) { 
-    return node.id;
+    return node.getAttribute("id"); 
}

That would circumvent the legacy shorthand access you've identified, without any conditionals. In fact, I'd even expect that to be faster because the browser just needs to directly retrieve the string value rather than cycling through alternative interpretations (like form fields and other cleverness). My assumption is that the attribute value is always what morphdom wants here; it doesn't care about the heavily overloaded property.

I agree that <input name="id"> is an awkward choice at best, so I too would also have preferred a different name there. However, I had little control over the HTML being provided, which is probably not an unusual situation for library/component authors. My understanding is that, as a generic library, morphdom aims not to care about specific details of whatever HTML it's processing - yet current behavior constitutes a limitation which is not just unintuitive and very tricky to debug for users, but also difficult to document for library authors. If my assumptions above are correct, that limitation is unnecessary and should be easy to remedy.

Either way, thank you all for your efforts; morhpdom is a pleasant addition to the ecosystem.

[snewcomer]

@FND So the extra conditional logic would come in for nodes that aren't a nodeType of 1. We process 3 (text nodes) && 8 (comment nodes), and those don't have getAttribute 😞

I'm not familiar with the different library permutations, but generally since this is the behaviour of the browser when using name="id", I would imagine the library authors would have other issues as well.

[FND]

Ah, I hadn't considered different node types, silly me.

However, for the reasons stated above, I still think it's worth doing:

    return node.getAttribute ? node.getAttribute("id") : node.id;

I would expect that, too, to be faster because this simple conditional should be less expensive than the id property's internal, much more elaborate checks (see above).

(Tests are passing with that change, but npm run benchmark doesn't seem to be doing anything for hours, so I can't compare.)

Sorry for being annoyingly persistent, but this kind of subtle issue makes it harder to convince folks to use morphdom-based hijaxing over full-blown SPAs.

[snewcomer]

jsperf tested this and got the below results with x and y being DOM elements. What are your thoughts?

[FND]

Well, I guess my naive assumptions were pretty far off then. 🙂 Thanks for checking.

That of course thwarts my attempt to appease your (perfectly reasonable) performance concerns. I do wonder whether that's a significant difference, as it still allows for a lot of operations per second - but I can't reasonably assess the real-world impact, so happy to rely on your expertise.

I'm afraid I can't think of a good way to document this potential pitfall ("use reasonable names for form fields and make sure they don't conflict with DOM APIs" seems more confusing than helpful), so it might just an implicit wart we'll have to accept. (Update: #151 ¯\_(ツ)_/¯ )

I'll still happily keep using morphdom. Thank you for humoring me.