DOM patching: Fall back to PHX_MAGIC_ID if node ID was touched by client hook

[ukutaht]

Context for proposal

I'm working on a UI component library with the idea to create an API similar to HeadlessUI / BaseUI. One of the features I'd like to implement is auto-generated IDs for children of the root component. In the following example I've added the desired IDs as comments:

~H"""
<.dropdown id="account-dropdown">
  <.dropdown_trigger>Account</.dropdown_trigger> <!-- #account-dropdown-trigger -->
  <.dropdown_menu> <!-- #account-dropdown-menu -->
    <.dropdown_item>Settings<.dropdown_item> <!-- #account-dropdown-menuitem-1 -->
    <.dropdown_item>Log out</.dropdown_item> <!-- #account-dropdown-menuitem-2 -->
  </.dropdown_menu>
</.dropdown>
"""

Auto-generating these based on the root component's ID is more convenient for the user and avoids copy-paste errors. In my opinion it's one of these cases where implicitness is really good because the IDs will always be consistent, avoids human error, and keeps the code less noisy. The generated IDs are completely predictable based on the root element.

Generating the IDs in the client hook is straightforward so I went with that. The hook also binds aria-labels so it it makes sense for it to deal with the IDs it binds as well.

The Problem

It worked fine until I ran into a case where a component is animated while receiving a liveview patch. I don't fully understand what parts of the setup are critical for this, but what happens is that the phx-skip optimization gets applied and it completely breaks rendering. The problem seems to be that during DOM patching, Liveview assumes that the DOM ID was set by the backend and it expects the update patch for that DOM node to have the same ID. Since the ID was in fact set by the client, morphdom fails to match nodes and rendering breaks in various interesting ways.

This logic is on line 152 in dom_patch.js

phoenix_live_view/assets/js/phoenix_live_view/dom_patch.js

Lines 142 to 154 in 0332007

	getNodeKey: (node) => {
	if (DOM.isPhxDestroyed(node)) {
	return null;
	}
	// If we have a join patch, then by definition there was no PHX_MAGIC_ID.
	// This is important to reduce the amount of elements morphdom discards.
	if (isJoinPatch) {
	return node.id;
	}
	return (
	node.id || (node.getAttribute && node.getAttribute(PHX_MAGIC_ID))
	);
	},

The Solution

My proposal is that:

1. When a DOM ID has been touched via this.js().setAttribute()
2. Then we ignore that DOM ID in morphdom node matching logic

So in pseudo-code the line:

return node.id || node.getAttribute(PHX_MAGIC_ID)

becomes:

return (!hasClientSetID(node) && node.id) || node.getAttribute(PHX_MAGIC_ID)

Where we fall back to PHX_MAGIC_ID when the ID has been touched by the client hook.

I've confirmed that it works as expected for the use-case that motivated this investigation in the first place.

Questions for maintainers

First of all - do you agree it's a problem worth fixing?

If so, we can talk about the specific solution. Currently it adds an array scan in presumably very performance-sensitive part of the DOM patching code which seems suboptimal. I don't have a strong intuition about the performance characteristics here so I'm looking to get feedback on it. If checking that the ID has been touched by the client is indeed too expensive, I can look into optimizing that at the cost of more complexity. The current solution is nice in that it's very straight-forward.

The solution is also currently incomplete in that it doesn't account for the client-side overriding a backend ID. It only works when the backend assigns no ID at all so it can fall back to the PHX_MAGIC_ID attribute. A more completely solution would be to also remember the backend-assigned ID when it is overridden by the client, and restore it for morphing purposes. I do not have a use-case for that and I can see why one would discourage this kind of pattern. And since it requires more complexity, my intuition is to stay away from it unless there's evidence it would be a useful addition.

[SteffenDE]

We indeed should not do expensive lookups in getNodeKey. Is there a reason to use separate components instead of slots for that case? With slots, you could generate the ID server side, so there'd be no mismatches.

When you say rendering breaks, do you see phx-skip lingering in the DOM afterwards or does it just break animations because nodes are replaced when they should not? The former would be a bug, the latter would be expected. Now in general its not ideal to have mismatching IDs between client and server, but I do see the requirement of generating IDs for aria attributes.

What we could do is require using js().setAttribute and special case the id attribute by also marking the node with DOM.putPrivate(node, "clientsideIdAttribute", true), then we can check it cheaply with DOM.private(node, "clientsideIdAttribute"). We can document that manipulating DOM ids with node.id = ... or replacing server IDs with a different client ID is not supported.

[ukutaht]

Thanks for the thoughtful response @SteffenDE!

Is there a reason to use separate components instead of slots for that case? With slots, you could generate the ID server side, so there'd be no mismatches.

I did consider slots but what made me avoid them is that they invert the control over the markup structure. In my view the greatest innovation of HeadlessUI/BaseUI is how they are completely agnostic about the markup structure, leaving it entirely up to the library user how they want to organize the markup.

A concrete example for dropdowns would be something like this:

~H"""
<.dropdown id="account-dropdown">
  <.dropdown_trigger>Account</.dropdown_trigger>
  <.dropdown_menu>
    <.dropdown_section>
      <.dropdown_heading>My account</.dropdown_heading>
       <span>{@user.email}</span>
    </.dropdown_section>
    <.dropdown_divider /> <!-- optional -->
    <.dropdown_section>
      <.dropdown_heading>Actions</.dropdown_heading>
      <.dropdown_item>Settings</.dropdown_item>
      <.dropdown_item>Log out</.dropdown_item>
    </.dropdown_section>
  </.dropdown_menu>
</.dropdown>
"""

This isn't a contrived or synthetic example - this is more or less the structure of the account dropdown in Plausible. You can have a dropdown with straight items, or have them nested in sections. The first section contains an informational <span> element, and you could put any other stuff in there. You could put an icon before the heading if you want. Dividers can be added or not.

I couldn't figure out how one would allow this kind of flexibility in terms of markup structure via slots.

Another example are modals - I want the header to be a <.modal_header> component so that we can auto-generate the ID and wire it up to the modal container as aria-labelledby for accessibility. But you should be able to place the header anywhere in the markup with no opinions from the library whether something can be above it or not.

Does that make sense? Using slots forces the library to start having opinions about how things should be structured and ultimately ends up limiting how components can be built. Sticking to composable function components allows maximum flexibility while keeping the library focused on solving specific problems - accessibility and dynamic interactions.

When you say rendering breaks, do you see phx-skip lingering in the DOM afterwards or does it just break animations because nodes are replaced when they should not?

It's the former. I managed to throw together a relatively minimal reproduction here. On localhost:4000/repro page when you pin and unpin elements you'll see how the markup is left in a broken state with phx-skip and empty innerHTML. It doesn't even require animations to break. With the patch from this PR applied it works as expected.

What we could do is require using js().setAttribute and special case the id attribute by also marking the node with DOM.putPrivate(node, "clientsideIdAttribute", true), then we can check it cheaply with DOM.private(node, "clientsideIdAttribute"). We can document that manipulating DOM ids with node.id = ... or replacing server IDs with a different client ID is not supported.

Yeah that's what I was thinking as well, glad to hear we're on the same page. I'll work on getting something like this going. Do you want me to close this PR in the meanwhile while I work on a proper solution?

[SteffenDE]

Thank you for the reproduction. Yeah, after thinking more about it it's actually expected, since the skip optimization basically assumes that there's another node with the same key without skip set, but because of the ID that assumption is wrong and the placeholder node is left.

You can push in the same branch if you want to!

[ukutaht]

I've pushed the performance improvement: ad81a09

Your suggestion was excellent - the code feels better this way since the attribute lookup is done in its natural context and the patching check is cheap.

I've also documented the issues around setting IDs on the client-side. I'm not sure if the bottom of the js-interop page is the best place for this. Let me know if you want me to move it around somewhere.

From my end it feels ready to go pending any comments from you.