Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions docs/components/capability-box/capability-box.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,27 @@ export default class CapabilityBox extends HTMLElement {
}

render() {
const heading = this.getAttribute('heading');
const { innerHTML } = this;

return (
<div class={styles.container}>
<span class={styles.heading}>{heading}</span>
{innerHTML}
</div>
);
const heading = this.getAttribute('heading') ?? '';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per your comment in the description

Updated wcc-capability-box to render heading using textContent instead of JSX interpolation.

Could you could expand on your solution here? I'm wondering if this is something that could / should be addressed within the JSX transformation itself. If you can provide some more details on the issue you observed, perhaps I can guide us to a solution that doesn't requiring refactoring the entire component.

Maybe it's a parse5 issue? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, doing a little playing around with AST explorer and browsing the parse5 GitHub, it does seem to be a parse5 "issue" - inikulin/parse5#191

In AST explorer, i can see &lt;template&gt; is getting parsed as <template> literally
Image

So I think the real work is not in the component itself, but rather in WCC itself, likely here
https://github.com/ProjectEvergreen/wcc/blob/0.19.0/src/wcc.js#L25

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming the parse5 behavior.

After stepping back from the component-level workaround, I agree this doesn’t seem like a rendering issue inside wcc-capability-box, but rather how WCC handles parsing and serialization after parse5 decodes entities like <template> into .

Looking at src/wcc.js, we parse via the DOM shim (which uses parse5) and later serialize the transformed tree with serialize(). Since parse5 decodes entities per spec, the attribute value becomes in the AST, and it appears to be getting interpreted as markup somewhere in that flow.

Before I make further changes, would you recommend focusing on how attribute values are handled during serialization (e.g. before serialize(finalTree)), or is it better to adjust behavior in the DOM shim layer?

I just want to make sure I’m addressing this at the correct abstraction level in WCC rather than introducing another workaround.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I make further changes, would you recommend focusing on how attribute values are handled during serialization (e.g. before serialize(finalTree)), or is it better to adjust behavior in the DOM shim layer?

Hmm... that's a good question. My hunch is that the bulk of the parse5 work is happening in the DOM shim, but I don't know if there would be any sort of pre-processing needed from WCC first to understand of there are entities that might get transformed? Since the behavior seems inherent in parse5, as the linked issue suggests, maybe some handling needs to happen on the way in and out? 🤔


Honestly, what I would do personally in a situation like this is create a unit test for the behavior in the WCC test suite (and with #246 we can do the same for the capability box component itself) and I would just explore what's possible to get the desired outcome with a lot of logging and a very narrow test case. Since I'm not sure to what degree working around parse5 would entail, and without a clear understanding of the exact solution, I would probably worry first about what it would take just to get things to work, and sort it out once I've made it that far.

Probably not a great answer, but that's pretty much how it goes sometimes. So maybe start a new branch / PR there? Happy to assist and answer questions as you make progress 🙌

const content = this.innerHTML;

this.innerHTML = '';

const container = document.createElement('div');
container.className = styles.container;

const span = document.createElement('span');
span.className = styles.heading;

span.textContent = heading;

container.appendChild(span);

const contentWrapper = document.createElement('div');
contentWrapper.innerHTML = content;

container.appendChild(contentWrapper);

this.appendChild(container);
}
}

Expand Down
6 changes: 3 additions & 3 deletions docs/pages/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ WCC aims to cover a reasonable set of DOM APIs to help facilitate server-renderi
<p>Declarative Shadow DOM</p>
</wcc-capability-box>

<wcc-capability-box heading="< template >">
<p>Template element support (createElement)</p>
</wcc-capability-box>
<wcc-capability-box heading="&lt;template&gt;">
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like formatting is off here. Prettier formatting should catch this, if you run it before committing

<p>Template element support (createElement)</p>
</wcc-capability-box>

<wcc-capability-box heading="CSSStyleSheet">
<p>Constructable Stylesheets shim</p>
Expand Down
Loading