Leverage HTML comment annotations, remove wrapping div elements

Updating issue metadata to reflect we can't do this until 📌 Add HTML comments to twig output to attempt to allow in-place editing Active lands.

📌 Add HTML comments to twig output to attempt to allow in-place editing Active is in!

jessebaker → changed the visibility of the branch 3491021-leverage-html-comment to hidden.

Exciting! 🤩

Instead of using HTML comments ( 📌 Add HTML comments to twig output to attempt to allow in-place editing Active ) which are not supposed to have anything processable in them, why not leveraging the Attribute object called attributes injected in all component template, to add the HTML attributes XB would need?

which are not supposed to have anything processable in them

Is there a concrete problem that you're anticipating?

We're using HTML comments because we don't want to rely on a specific attribute for this. The goal for Experience Builder is to provide an easy way for developers to expose their existing SDC. Requiring a specific attribute would make the integration harder because components would need to always have this attribute which isn't the case today based on what I've seen in design systems.

Attributes also have another challenge. We need to be able to identify all of the slots as well. This means that not only would we need to have the attributes in the wrapper, but also on wrappers for slots. This would add additional requirements for components (i.e. consistent display of wrappers + attributes for slots) which is not acceptable from DX perspective.

Thanks for your clear answer Lauriii.

Is there a concrete problem that you're anticipating?

I am expecting HTML comments to not be reliable place to put processable data, according to some HTML parsing behaviours I have witnessed a few times, long time ago, but it may be an outdated opinion.

If using the attributes variable doesn't fit your needs, I understand you are looking for other ways.

My initial approach to this problem was to build out an api/library similar to JS's native DOM querying methods (e.g. getElementById) that would allow me to do things such as getComponentElementByIdInHTMLComment or getSlotByHTMLComment. This would mean that it was possible to replace document.querySelector etc with one of the above and hopefully lead to a fairly easy to implement solution.

However:

1. building out that library is proving tricky in itself (but I do have some parts working)
2. after having built several parts of it, there is such a level of complexity already in the code for e.g. building the overlay components, that adding this additional layer of abstraction makes the code feel very unwieldy to work with.

The advantage of my initial approach is that it means that we don't have to touch the user's mark up at all and thus it should (in time) be robust and able to handle any and all situations. The downside is the addition of significant complexity in lots of places.

I have started to look into a different approach however. This second approach is far simpler. As the HTML for the preview is retrieved from the server we can modify it (parse it to a DOM document and then use JS to modify the DOM) before we hand it over to the preview renderer.

The advantatge of doing it this way is that I can take each HTML comment and easily convert it to data-xb- attributes on the corresponding HTML elements. This means things like document.querySelector(['data-xb-slot']) become available, simplifying the implementation everywhere.

The downside of this new approach is that there a small number of scenarios that we won't be able to support. An example of that is:

Let's say someone has a JS library to render a video. The server outputs an XB component that renders a <video> tag but also attaches a JS library. The JS library, on load, will find that video tag and replace it with an

So we are now generating comments server side, and parsing those comments and turning them into data attributes into the client? In the interests of keeping things simple, and with the fact that we control the server side output, why can't we just add those data attributes up front on the server side and save translating anything in the client?

I think adding the attributes on the server would mean that SDCs are required to output the $attributes variable in their Twig file, and to do that on the component's root element. This might be fine if we think SDC creators already know to output $attributes for other reasons, but #9 indicates that this isn't currently being consistently done in the wild. Also, per #9, it would mean SDCs would also need to output a similar variable on the slots -- is that true or could we automate that one transparently?

It's possible that the implications above are acceptable and that it makes sense to do that. However, if we stick with the HTML comments approach, I agree with #11's suggestion to do the simpler client-side option and punt on the edge case of component JS code that fully swaps out the component's root element.

I think we could add the attributes afterwards, either naively with regex if it's always to a single top level element otherwise we could just do the same DOM parsing and manipulation that Jesse is suggesting, but on the server.

This might be fine if we think SDC creators already know to output $attributes for other reasons, but #9 indicates that this isn't currently being consistently done in the wild.

We made the decision not to use $attributes in our twig, as it'd tightly couple things to Drupal. We may be the outlier, but I'm living proof of the concern :-)

we could just do the same DOM parsing and manipulation that Jesse is suggesting, but on the server

I like this idea but for this issue, let's optimize for speed of getting this initially done, because as I understand it, this issue is blocking further work on implementing the UI for editing content in global regions, and it would be great to unblock that and make more progress on global regions before people go on holiday breaks. If it's faster to get this done client-side for now, and then have a follow-up for moving it to server-side, I think the benefit of unblocking the other regions work is worth the cost of the extra refactor.

So we are now generating comments server side, and parsing those comments and turning them into data attributes into the client?

😅 - that's basically true, yeah!

The benefit of handling on the client side is that the browser has everything we need to manipulate the DOM easily. I'm sure the same things are possible on the back end but, long term, I do think that leveraging the HTML comments "in real time" as and when we query for components/slots is the preferred solution. Handling this on the front end for now means that the later refactor will be more easily handled without having to make both PHP and JS changes.

effulgentsia:

This might be fine if we think SDC creators already know to output $attributes for other reasons, but #9 indicates that this isn't currently being consistently done in the wild.

luke.leber:

We made the decision not to use $attributes in our lowest level* twig, as it'd tightly couple things to Drupal.

I don't know what are "our lower level twig" here, but Experience Builder will be used with components from contrib or custom themes, so let's have a look on what themers are doing:

It seems that #18 is something we could consider if we run into challenges with the current comments approach. The data looks concerning because only one of the non-UI Suite themes has 100% coverage. It also seems that the concerns around using HTML comments are currently hypothetical.

Wrapping up for the weekend - my work is not complete here but here is where I'm at.

I pivoted slightly from my two different approaches described in #11.

Now, each time a new document is rendered in the iFrame, I run two walker functions that iterate over the document and 'parse' the HTML comments into two maps, one for components and one for slots. The mapping allows me to very easily find the HTMLElement in the iFrame for any given component UUID or slot ID.

They look like this:

interface SlotInfo {
  element: HTMLElement;
  componentUuid: string;
  slotName: string;
}

interface ComponentInfo {
  elements: HTMLElement[];
  componentUuid: string;
}

export type SlotsMap = Record<string, SlotInfo>;
export type ComponentsMap = Record<string, ComponentInfo>;

Where the string/key in each Record is the ID or UUID of the slot or component.

In practice this allows me to call componentsMap[uuid].elements and retrieve an array of the DOM elements that make up that component (and do similar for slots).

This doesn't immediately solve the issue described where the DOM may be manipulated after the map is built to remove the original element but it works as well as the current 'wrapping divs' solution and will be easy enough to extend to make it capable of running a fallback to catch those kind of situations.

In less positive news, what I have done in the refactor so far seems to have broken the ability to drag and drop so there is still some way to go before this is complete. Hopefully I can polish it off before holidays!