whatwg / xhr

XMLHttpRequest Standard
https://xhr.spec.whatwg.org/
Other
314 stars 131 forks source link

Add link to declarative shadow dom opt-in #300

Open mfreed7 opened 3 years ago

mfreed7 commented 3 years ago

This PR goes along with the two in HTML and DOM to add declarative Shadow DOM to the spec. This PR goes along with the discussion at #912, to opt-in to the fragment parser for declarative Shadow DOM.


Preview | Diff

annevk commented 3 years ago

As I mentioned there, I don't think we should add this feature to this API. XMLHttpRequest is essentially frozen and new functionality should go into fetch() if applicable, but I don't think this is. I didn't see pushback there from you so maybe you missed that feedback?

mfreed7 commented 3 years ago

I agree, and I didn’t miss that feedback. This change explicitly sets the “include shadow roots” flag to “deny”, so that XHR cannot be used to parse DSD content. Without this change to XHR, it will allow DSD parsing. So effectively, this is “no change”.

annevk commented 3 years ago

I missed that, is there a reason "deny" is not the default, given that it's "unsafe"?

mfreed7 commented 3 years ago

I missed that, is there a reason "deny" is not the default, given that it's "unsafe"?

I'm open to suggestions on how I implemented this in the HTML spec (and in code), but as it stands, the "include shadow roots" flag is tri-state. It can be unset, or explicitly "allow" or "deny". The reason is that for fragment parsing, unset means "don't allow" DSD content. But for non-fragment parsing, unset means "allow" DSD content. The explicit cases allow that default behavior to be overridden, e.g. here for XHR, where the XHR document is parsed with a non-fragment parser, but we still don't want to allow DSD content. Or the opposite case for DOMParser with the includeShadowRoots flag set to true, we want to explicitly allow DSD content even for the fragment parser.

annevk commented 3 years ago

Interesting, I guess I need to look at the specific PRs, but it seems that it could be a boolean everywhere that defaults to false, and when we invoke the main parser for documents we set it to true.

mfreed7 commented 3 years ago

Interesting, I guess I need to look at the specific PRs, but it seems that it could be a boolean everywhere that defaults to false, and when we invoke the main parser for documents we set it to true.

Ok. I'll be interested to hear your suggestions for implementing it that way. I found in the implementation that there are multiple places we invoke the parser, so this was the least overhead approach to avoid touching most of them. But always open to improvements here!

annevk commented 1 year ago

This is obsolete now, right @mfreed7?

mfreed7 commented 1 year ago

This is obsolete now, right @mfreed7?

No, I think we still need this, to ensure XHR doesn't parse DSD content. It does need an update, since "include shadow roots state" has been renamed.

annevk commented 1 year ago

But isn't the default of this state deny? Oh, I guess it also has unset, but I still don't really understand that third state.

mfreed7 commented 1 year ago

But isn't the default of this state deny? Oh, I guess it also has unset, but I still don't really understand that third state.

The default is "unset" which is interpreted as "allow" for document parsing and "deny" for synchronous parsing.

annevk commented 1 year ago

Can we make it deny and have page loading overwrite it? Usually when we have tri-states there are actually three different behaviors.