Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

browser warning about "declarative Shadow DOM has not been enabled by includeShadowRoots" #130

Closed
thescientist13 opened this issue Jan 3, 2024 · 2 comments · Fixed by #129
Assignees
Labels
0.11.0 question Further information is requested
Milestone

Comments

@thescientist13
Copy link
Member

thescientist13 commented Jan 3, 2024

Summary

Have been noticing this warning lately in Chrome , like if you go here and click the Load Products button at the bottom of the page
https://greenwood-demo-adapter-vercel.vercel.app/

Found declarative shadowrootmode attribute on a template, but declarative Shadow DOM has not been enabled by includeShadowRoots.

Screenshot 2024-01-03 at 11 16 56 AM
Screenshot 2024-01-03 at 11 18 08 AM

It doesn't actually seem to be an issue with WCC per se, but rather just how we are loading it on the page? 🤔
https://github.com/ProjectEvergreen/greenwood-demo-adapter-vercel/blob/main/src/pages/index.html#L18C1-L32C14

<!-- Fragment API Setup -->
<script>
  globalThis.addEventListener('DOMContentLoaded', () => {
    const limit = 10;
    const page = limit;
    let offset = 0 - page;

    globalThis.document.getElementById('load-products').addEventListener('click', async () => {
      offset = offset += page;
      const html = await fetch(`/api/fragment?offset=${offset}&limit=10`).then(resp => resp.text());

      document.getElementById('load-products-output').insertAdjacentHTML('beforeend', html);
    });
  });
</script>

Interestingly, the same implementation using HTMX does not complain about it.
https://greenwood-htmx.vercel.app/

Screenshot 2024-01-03 at 11 20 25 AM

Details

Per some reading around, it does seem that this is related to the use of insertAdjacentHTML, as per these docs, it suggests that it would be required to use DOMParser instead

To avoid some important security considerations, Declarative Shadow Roots also can't be created using fragment parsing APIs like innerHTML or nsertAdjacentHTML(). The only way to parse HTML with Declarative Shadow Roots applied is to pass a new includeShadowRoots option to DOMParser:

<script>
  const html = `
    <div>
      <template shadowrootmode="open"></template>
    </div>
  `;
  const div = document.createElement('div');
  div.innerHTML = html; // No shadow root here
  const fragment = new DOMParser().parseFromString(html, 'text/html', {
    includeShadowRoots: true
  }); // Shadow root here
</script>

I can see for example that HTMX is using DOMParser, but just not with the includeShadowRoots option.

function parseHTML(resp, depth) {
  var parser = new DOMParser();
  var responseDoc = parser.parseFromString(resp, "text/html");

   // ...
}

(more context here - whatwg/dom#912 (comment) )


I was seeing some of this behavior even with HTMX as part of the solution for #128 ( see #129) so want to conduct some more tests to arrive at a proper conclusion and if this is just a userland issue, or anything WCC itself actually needs to worry about.

So going to play around with some of our implementations and ascribe any relevant documentation if needed and report back.

@thescientist13 thescientist13 added the question Further information is requested label Jan 3, 2024
@thescientist13 thescientist13 self-assigned this Jan 3, 2024
@thescientist13 thescientist13 changed the title browser warning about _declarative Shadow DOM has not been enabled by includeShadowRoots_ browser warning about "declarative Shadow DOM has not been enabled by includeShadowRoots" Jan 3, 2024
@thescientist13 thescientist13 added this to the 1.0 milestone Jan 3, 2024
@thescientist13
Copy link
Member Author

thescientist13 commented Jan 3, 2024

Can confirm converting to use DOMParser makes the issue go away as seen in ProjectEvergreen/greenwood-demo-adapter-vercel#25, though technically includeShadowRoots isn't needed since we are constructing our shadowRoot through a <template> tag?

export default class Greeting extends HTMLElement {

  connectedCallback() {
    if (!this.shadowRoot) {
      const template = document.createElement('template');

      template.innerHTML = `
        <div>
          <h1>Hello World!</h1>
        </div>
      `;
      this.attachShadow({ mode: 'open' });
      this.shadowRoot.appendChild(template.content.cloneNode(true));
    }
  }
}

customElements.define('app-greeting', Greeting);

If we do something like this though, then we will get the error and we will have to use includeShadowRoots it seems

export default class Greeting extends HTMLElement {
  connectedCallback() {
    if (!this.shadowRoot) {
      this.attachShadow({ mode: 'open' });
      this.shadowRoot.innerHTML = `
        <div>
          <h1>Hello World!</h1>
        </div>
      `
    }
  }
}

customElements.define('app-greeting', Greeting);

hmm, looks like it happens even when manually setting innerHTML, even in the component definition itself?
Screenshot 2024-01-03 at 2 15 58 PM

So not sure how much this does / doesn't muck with any assumptions we are making about how to construct "dynamic" templates I guess, or at least we should emphasize the work arounds needed if you don't use <template>.

@thescientist13
Copy link
Member Author

thescientist13 commented Jan 4, 2024

So yeah, for #128 , we can't use innerHTML even in the component definition, so best option would be to use <template> elements, otherwise the browser will complain about that too
Screenshot 2024-01-03 at 6 53 46 PM

Will see if there are some docs to add around this, but not sure there's much more for us to do here at this point. I did open up an issue to the MDN docs about documenting includeShadowRoots - mdn/content#31501

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.11.0 question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant