Add getHTML() and the serializable concept

[mfreed7]

trafficstars

Add getHTML() and the serializable concept for shadow roots. See https://github.com/whatwg/html/issues/8867#issuecomment-1856696628 for the rough consensus on how this should work, plus the few comments below that with modifications.

• [X] At least two implementers are interested (and none opposed):
  • Chromium
  • WebKit
• [X] Tests are written and can be reviewed and commented upon at:
  • https://wpt.fyi/results/shadow-dom/declarative/gethtml.tentative.html
• [ ] Implementation bugs are filed:
  • Chromium: https://crbug.com/41490936
  • Gecko: …
  • WebKit: …
• [ ] MDN issue is filed: …
• [X] The top of this comment includes a clear commit message to use.

See the corresponding DOM spec PR here: https://github.com/whatwg/dom/pull/1256

---

/indices.html ( diff ) /infrastructure.html ( diff ) /parsing.html ( diff ) /scripting.html ( diff )

[mfreed7]

Parsing MDN data... Parsing... Parse Error: (130197,76) unexpected end tag

I'm not sure what this is - I do not get any errors when I build locally, using the build.sh script. Clues appreciated.

[domenic]

I'm not sure what this is - I do not get any errors when I build locally, using the build.sh script. Clues appreciated.

This is due to https://github.com/whatwg/html-build/issues/290. You have some invalid HTML around that line, but the Rust portion of the build step is fixing it up for you. PR preview doesn't run the Rust portion of the build step, so it fails.

You can try applying https://github.com/whatwg/html-build/pull/291 locally. Or you can stare really hard at (130197,76) to find the extra end tag.

[mfreed7]

This is due to whatwg/html-build#290. You have some invalid HTML around that line, but the Rust portion of the build step is fixing it up for you. PR preview doesn't run the Rust portion of the build step, so it fails.

Ahh, thanks. Is there a way to run the local build without the Rust, so it matches what the preview does? Generally I try to make sure things build before I put up a PR, so I was disappointed to see this failure.

You can try applying whatwg/html-build#291 locally. Or you can stare really hard at (130197,76) to find the extra end tag.

Thanks. 130197 is a blank line, and I had been staring at 130198 instead. Turns out the problem was on 130196.

[mfreed7]

Parse Error: (142841,9) unexpected body end tag

Looks like I'll be uploading a bunch of patches to try to find this one now...

[mfreed7]

Parse Error: (142841,9) unexpected body end tag

Looks like I'll be uploading a bunch of patches to try to find this one now...

Side note, as I manually scan for mis-nested tags, it's nice that WHATWG fought for keeping HTML and not switching to XHTML.

[annevk]

Also:

1. We need a WPT PR that renames away from .tentative and corrects the naming of the shadowrootserializable attribute.
2. There should be an MDN issue.

[mfreed7]

Also:

1. We need a WPT PR that renames away from .tentative and corrects the naming of the shadowrootserializable attribute.

This is in progress, here and here.

2. There should be an MDN issue.

Done: https://github.com/mdn/content/issues/32731

[lukewarlow]

I can't find a way to unresolve the comment and it's not relevant to this PR anymore since it's been removed, but if Chrome is using HTMLString as a return type it's likely doing nothing more than being a DOMString, or worse case is slower.

For context HTMLString comes from Trusted Types and it's an alias for [StringContext=TrustedHTML] DOMString which does some enforcement of TT when relevant CSP is set.

[mfreed7]

I can't find a way to unresolve the comment and it's not relevant to this PR anymore since it's been removed, but if Chrome is using HTMLString as a return type it's likely doing nothing more than being a DOMString, or worse case is slower.

For context HTMLString comes from Trusted Types and it's an alias for [StringContext=TrustedHTML] DOMString which does some enforcement of TT when relevant CSP is set.

So this PR has been changed to do HTMLString -> DOMString. But I think perhaps you're talking about the Chromium implementation which does use HTMLString? If so, can you clarify what you think should be changed? Existing serializers like innerHTML and outerHTML are supposed to return HTMLString also, right? I see that in Chromium they don't currently, but I thought I should try to return HTMLString for new things. LMK if that's wrong.

[annevk]

I think HTMLString is only relevant for input which is why innerHTML uses it (for its setter).

It seems this PR needs rebasing. I think we should also resolve https://github.com/whatwg/html/issues/10211.

And where you wrote "element or shadow root" you didn't update the variable name. There might be more errors, I didn't read through it all again.

[mfreed7]

I think HTMLString is only relevant for input which is why innerHTML uses it (for its setter).

Ahh, makes sense, thanks. I "forgot" innerHTML is a settable attribute - my brain was in serialization mode. I'll change the Chromium impl to return DOMString.

It seems this PR needs rebasing.

Ok, rebased. And now I see why @lukewarlow was commenting on trusted types. 😄

I think we should also resolve #10211.

I really hope we don't gate this PR on that issue. My strong feeling on that issue is that we shouldn't change course again. But even if we do, the attribute being added in this PR is the least of the concerns - if we go forward with #10211, we'll be ripping out several other attributes also, which have been there longer. Let's please get this one landed so we can finally be done with all of the renames, deprecations, etc.?

And where you wrote "element or shadow root" you didn't update the variable name. There might be more errors, I didn't read through it all again.

Done - I renamed the var to elementOrShadow. Let me know if another name is more consistent with the rest of the spec.

[mfreed7]

WebKit only shipped template.shadowRootMode so that seems straightforward to undo. And I think we're also all agreed they're useless so I guess I at least want all of us to seriously consider it.

Yep, it's a lot easier to unship something that wasn't shipped. As a result of all of the changes after the initial shipment, Chrome has had six intents to ship so far. I've been trying to be a good sport about it, so we get to an interoperable state with this feature. And I'm really happy with where we're ending up. But changing back from reflections to no-reflections would be a reversal of course on a part of the API that has no developer use case, so it feels like a bit too much.

@domenic or @rniwa any opinions? I'm completely ambivalent about reflections vs. no-reflections, since there's no real developer use case. But I have a strong preference to avoid changing directions again, if we can.

I also realized that this PR enshrines a serialization order for the shadow root attributes. Attributes are normally sorted in insertion order so there's no clear correct answer here, but just something to keep in mind I guess.

Yeah, this is an interesting case, since these were never attributes in the first place. The fixed/predictable order does make testing easier. Sounds like you're ok leaving this as-is?

[domenic]

I've commented on whether we should keep the reflected IDL attributes or not at https://github.com/whatwg/html/issues/10211#issuecomment-2016364509.

[lukewarlow]

I think HTMLString is only relevant for input which is why innerHTML uses it (for its setter).

Ahh, makes sense, thanks. I "forgot" innerHTML is a settable attribute - my brain was in serialization mode. I'll change the Chromium impl to return DOMString.

Sorry for the delay, yeah this is correct, so TLDR if it's a setter or a method param it's a candidate for HTMLString (or ScriptString, and ScriptURLString), otherwise just use the raw type.

[annevk]

No need to revert, but https://github.com/web-platform-tests/wpt/commit/2969dd3b1af9126ed769915ff852cb29eb615d52 before this PR merges is somewhat annoying. They should be merged around the same time, ideally by the same person. This PR doesn't even have r+.

[mfreed7]

Sorry for the delay, yeah this is correct, so TLDR if it's a setter or a method param it's a candidate for HTMLString (or ScriptString, and ScriptURLString), otherwise just use the raw type.

I landed the change in Chromium, and it turns out the bindings are smart enough to treat them the same for a return value, just FYI. But now the IDL is at least correct.

I've commented on whether we should keep the reflected IDL attributes or not at #10211 (comment).

Thanks! Your rationale sounds totally reasonable to me. Sounds like @annevk agrees.

No need to revert, but web-platform-tests/wpt@2969dd3 before this PR merges is somewhat annoying. They should be merged around the same time, ideally by the same person. This PR doesn't even have r+.

Seems like we're very close to landing this? All comments are resolved.

[annevk]

https://github.com/whatwg/html/pull/10139#pullrequestreview-1884248638 is still open.

[mfreed7]

#10139 (review) is still open.

Hopefully addressed now?

[domenic]

@annevk want to do a final check? Also I think you should merge https://github.com/whatwg/dom/pull/1256 first?

[annevk]

I'm sad that my comment there was used to delay landing this.

Your opinion on this apparently differs, but I don't think delaying this by a couple days is a big deal, given that it might make blame a lot more useful. I also didn't think @mfreed7 would act on the comment given what you had written there. I wanted to wait with landing until I had some conversation with you about it. I think the changes you made make sense, though Simon just spotted a bigger problem.

[mfreed7]

Thanks everyone!