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!

#8: that implies (non-comment) HTML that is not an exact match. Been there, done that, with https://www.drupal.org/project/quickedit → . Endless whack-a-mole. Plus, as @lauriii described in #9, not everything that we can target with HTML comments can be targeted with attributes.

Ironically, @jessebaker describes in #11 that he's essentially converting HTML comments into attributes 🤪 Everything in #12 and #13 etc. is painfully predictable.

In #11, @jessebaker describes having started to write low-level infrastructure himself. Surely there is a thoroughly battle-tested HTML comment parsing library out there? For example, why can't we use https://www.npmjs.com/package/html-parse-stringify? The test coverage seems to indicate it can parse the structure for us?

The solution allow describing where an SDC prop appears

Using attributes is not a long-term solution, because it means we cannot achieve everything we need as described in 📌 Implement endpoint for realtime preview Active . Put simply: an SDC prop may appear either as a text node (which by definition does not have an attribute) or even an attribute node (same problem). Put differently: since an SDC prop's value may appear anywhere in the SDC's resulting markup, we must be able to annotate every HTML node type.
The attribute-based solution being argued for (with a lot of data to back it up in #18, impressively so, @pdureau! 😄) only supports element nodes. It must support everything https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType#value (well, then you could argue it must also support comment nodes — so let's rephrase that too: everything that results in visual impact: element, attribute and text nodes).

In this MR so far I'm focusing purely on removing the wrapping divs around each component and the divs wrapping each slot and replacing them with HTML comments. In doing this, most of the refactor is around a) making sure the Overlay UI still matches the size of the elements it is supposed to be overlaying and b) making sure sortableJs still works for dropping components into the iFrame.

Potentially, now that the MR for adding global region support has landed, support for global regions via comments instead of div will be added in this MR too but it might make sense to push that to a follow up issue. I'm still looking into it.

Adding comments around individual prop values is something we will need to address later and I believe the main technical challenge there is the work to add those comments on the server - potentially necessitating generating/leveraging and AST of the Twig files.

My solution in #20 (and the current state of the MR) is that some data attributes are added specifically to elements rendered by components that can be drag/dropped to enable SortableJs to function but they are added at run time (in JS) after parsing HTML comments and not added on the backend.

#21 requires that we are able to annotate element, attribute and text nodes in the HTML, and I don't disagree with this; we need to be able to reference any of these in XB. However, elements and text can be wrapped by comment nodes, but attributes cannot - comments are not allowed inside tags. Which I think means we need another solution for attributes anyway?

So given that I'm wondering if there is another solution here that doesn't require wrapping at all. Maybe trying to keep the XB metadata inside the document is not the right answer - should the metadata be out of band instead? Could we have a separate JSON structure that maps UUIDs/other data to some kind of querySelector/XPath style mechanism that can refer to a specific node in the DOM, no matter whether that's an element, attribute or text node?

I was thinking that annotating attributes could be achieved by something like the following with a comment immediate preceding an element describing which props map to which attributes.

<!-- xb-start-8db0a067-742c-4356-acdb-241cf3304e37 -->
<div>
  <p>
    <!-- xb-attrs cta_url:href|cta_target:target -->
    <a href="https://www.example.com" target="_blank">Link example</a>
  </p>
</div>
<!-- xb-end-8db0a067-742c-4356-acdb-241cf3304e37 -->

I can only think of use cases where we would want to highlight the element on which an attribute exists and this would have enough info to do that.

However, I am certainly interested in @longwave's suggestion of using some kind of separate map that comes along with the HTML. The only way I can see that falling down is similar to the issue described in #11 where the DOM is manipulated by JS library after the JSON map has been generated and thus the paths get out of sync.

#23

Which I think means we need another solution for attributes anyway?

😳 I'd swear we concluded months ago that this would work?! Sure enough, a simple test proves you right: <html><body><input type="<!-- test -->date" />.

#24: whew, Jesse to the rescue! 😄😅 I bet that's what @jessebaker suggested months ago and I just summarized it a bit too much in my head 🙈

The separate mapping described by @longwave is technically possible for sure, but even just for debugging ergonomics alone (and hence XB development velocity), I'd think that what @jessebaker described in his last comment is strongly preferable?

@jessebaker in #22:

I'm focusing purely on removing the wrapping divs around each component and the divs wrapping each slot and replacing them with HTML comments

I remember reading somewhere (perhaps in our private Slack?) that you mentioned that Block-sourced Component instances have a needed <div> gone missing. Which makes me think that the removing of the wrapping divs is going slightly too far: XB can only remove wrapping <div>s that are generated by XB, not any that are generated by (and hence owned by) a Component.

I bet it's worth pairing on this for a bit? That'd also make it easier for me to review/understand what exactly this MR is doing! 😇

The attribute-based solution being argued for (with a lot of data to back it up in #18, impressively so, @pdureau! 😄)

Ah ah indeed 😉

Seeing the complexity of the last days discussion and the current MR adding 500 new LOC to the already huge XB codebase, I am still believing something specific has been done wrong earlier in the project and we are now fighting against it.

I hope everything will land well 🤞

@pdureau's comment caused me to take another look at this and … it's green now! 😄🥳

99% of the server-side changes make sense — the few remarks I made yesterday still stand, but should be trivial to address 😊

Given both @bnjmnm and @jessebaker have worked on this … I think it'd be wise to ask either @hooroomoo or @balintbrews to review this MR?

#24 seems feasible for the solution to attributes although I still think injecting them in the right place on the server side is going to be tricky in some cases.

The sample in #25 doesn't work like you expect:

> document.querySelector('input').type
"text"

and it renders in the browser as a text input, not a date.

Created followup issue 📌 Implement HTML comment annotations for Regions, replace HTML wrappers Active to do the same conversion for wrappers around Global Regions

Thanks for creating the follow-up!

MR review (back-end only)

• Made one very significant change: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... — this removes part of 📌 Add HTML comments to twig output to attempt to allow in-place editing Active in favor of something much more explicit, and consistent across all XB component types (see: 🌱 [META] Support component types other than SDC Needs work ).
• Follow-ups opened: 📌 Only run XbTwigExtension inside XBPreviewRenderer Active .

Approved the back-end side of this MR!

because https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

@hooroomoo's performance observation/question at https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... could also be answered by just docs, by the way :)

I have added some docs to the MR and also created a follow up ✨ Refactor React Context (ComponentHtmlMapContext) to Redux to allow for better extensibility Active to perform a small refactor from Context to Redux that was not deemed important enough to hold up this MR any longer.

🔥