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

Add information to DOMParser.parseFromString page about the includeShadowRoots option #31501

Closed
thescientist13 opened this issue Jan 4, 2024 · 6 comments
Labels
Content:WebAPI Web API docs needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened.

Comments

@thescientist13
Copy link

thescientist13 commented Jan 4, 2024

MDN URL

https://developer.mozilla.org/en-US/docs/Web/API/DOMParser/parseFromString

What specific section or headline is this issue about?

Parameters

What information was incorrect, unhelpful, or incomplete?

Per the Chrome Developer docs, there is reference to a third parameter to parseFromString that handles parsing Declarative Shadow DOM

<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>

What did you expect to see?

I would like to see the content expanded to include the third param and some details about the option and why it would be used. For example, the Chrome docs frame it as such:

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

Do you have any supporting links, references, or citations?

Here is a thread from WHATWG/dom GitHub discussion regarding its participation in the Declarative Shadow DOM spec
whatwg/dom#912 (comment)

Do you have anything more you want to share?

It seems like this might make a good companion to #29600 ?


Happy to help contribute to drafting this content if you would like. 👍

@thescientist13 thescientist13 added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Jan 4, 2024
@github-actions github-actions bot added the Content:WebAPI Web API docs label Jan 4, 2024
@pepelsbey
Copy link
Member

pepelsbey commented Jan 19, 2024

Hey! Great idea :) We’re currently documenting shadowrootmode and clonable for the upcoming Firefox 122 release (behind the flag for now, but it will be enabled by default in 123). Feel free to go ahead and send a PR with the includeShadowRoots option as well.

Though I’m curious what’s the browser support for includeShadowRoots. I couldn’t find it in BCD, apart from the parseFromString method itself.

@thescientist13
Copy link
Author

Thanks, I'll get started on that PR then!

Though I’m curious what’s the browser support for includeShadowRoots. I couldn’t find it in BCD, apart from the parseFromString method itself.

I can try and take a look into that. Is it just a matter of getting an official response from each vendor a confirmation of what version that feature was added in? (Apologies, haven't worked with BCD myself).

I found this blog post from Webkit about Declarative Shadow DOM which calls out DOMParser but not specifically includeShadowRoots. Not sure if that is helpful...

@thescientist13
Copy link
Author

thescientist13 commented Jan 21, 2024

With some help from the WCCG, I was pointed to the WPT which shows that includeShadowRoots is under test
https://wpt.fyi/results/shadow-dom/declarative/declarative-shadow-dom-opt-in.html?label=experimental&label=master&aligned

And that is being shown in reports
Screenshot 2024-01-21 at 12 52 55 PM

It's called out as "(includingShadowRoots is historical)" and It's not mentioned in the spec.

Additionally this comment implies that the explanation for these then is that it's not going to be standardized.


So I wonder then if the goal here with this particular content around includeShadowRoots should be focused on capturing from a historical perspective and that it is not a standard, and should encourage readers to use / look to the alternative options instead?

There is a tracking item for this content as well - mdn/mdn#459

@pepelsbey
Copy link
Member

pepelsbey commented Jan 29, 2024

It’s not mentioned in the spec
It’s not going to be standardized

I’m afraid this changes everything. Thank you so much for bringing this to our attention, filing the issue, and opening a draft PR. We really appreciate it! However, since the options param is not in the spec and hence why the WPT tests fail. It is also not a part of BCD. I’ll have to close it since we’re not documenting features that aren’t in the spec or the browsers, even for historical reasons. I’m sorry for confusing you earlier.

@pepelsbey pepelsbey closed this as not planned Won't fix, can't repro, duplicate, stale Jan 29, 2024
@thescientist13
Copy link
Author

Alrighty.

Do you think there's any value on starting on mdn/mdn#459 at all, or is it too soon for that one? It's in WPT and the spec PR was merged but not sure around the sanitization efforts / parameters called out in that description.

Either way, happy to support on this now or in the future when MDN is ready for it. 👍

@pepelsbey
Copy link
Member

@thescientist13 thank you! It seems like the setHTMLUnsafe and parseHTMLUnsafe methods were implemented behind the flag in Firefox 122 and will be shipped by default in 123. It means we have a stable spec and at least one implementation with other browsers ready to follow. That’s a perfect case for documenting this on MDN :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants