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

Change registries to be per document and never shared between documents #369

Closed
domenic opened this issue Jan 26, 2016 · 28 comments
Closed
Assignees

Comments

@domenic
Copy link
Collaborator

domenic commented Jan 26, 2016

In particular, createDocument() and similar don't share registries with the parent.

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2016

So in IRC @rniwa and I discovered this really doesn't work. You want basically one registry per window, not one registry per document, now that we have the constructor setup.

The reason is basically that, when doing new CustomElement(), you don't have any of the information necessary to figure out where that element was registered. You only have the global object of the function. That's, in fact, what we use to figure out the registry to do tagname lookup in.

We could fix this in theory by mandating a document argument to all custom element constructors (and defaulting it to the current realm's document). But that's pretty frustrating to use for people who are trying to use new createDocument() documents. And we couldn't agree on whether the argument would be a dictionary or not. So instead we think we should just go back to disallowing multiple registries per window.

That means either sharing the registry with the parent (per the current spec) or just saying that they have no registry (and disallowing defineElement in them too).

Side note: we need to check how document.open() affects this since it creates a 1-document to many-windows mapping.

@rniwa
Copy link
Collaborator

rniwa commented Mar 3, 2016

Similarly, we can't make template.content.ownerDocument.defineElement work.

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2016

Upon sleeping on this I am amenable to a new alternative, which is a mandatory document parameter to all custom element constructors. So:

  • HTMLElement takes a mandatory first argument which is the document in which it is created.
  • super() calls inside custom element constructors must pass that first argument. This is probably easiest accomplished by doing constructor(...args) { super(...args); ... }, or maybe constructor(document, optionalSpecialStuff) { super(document); ... }
  • The parser and createElement pass the relevant document as the first parameter to the custom element constructor (new CustomElement(relevantDocument))
  • As a corollary, in general new CustomElement() will not work.
    • This should make the new CustomElement(document) <-> document.createElement("custom-element") connection more obvious. They are simply alternative syntaxes for each other.
    • new CustomElement() could be made to work by authors if they do something like constructor() { super(window.document); }.
    • But doing this is opting in to a behavior which prevents your element from being used correctly when registered on other documents.
  • This is forward-compatible with future expansions like passing an optional tag name (which I would still prefer to be in a dictionary).

I am not OK with it defaulting to some specific document because it is ambiguous which document. We either:

  1. Should make this safe for authors by ensuring a 1:1 window:custom element registration setup (e.g. by sharing or disallowing registries in non-windowed documents), so that new CustomElement() is unambiguous
  2. Or should make this an explicit mandatory part of the API that authors need to be aware of.

WDYT? (1) or (2)?

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

Note that we already have document defaulting in DOM, for Comment, Document, DocumentFragment, Text...

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

And also for Image, Audio, others?

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2016

Yeah, but you can't register custom text nodes in a variety of documents. It's always unambiguous when you do new someWindow.Text() what document that will correspond to. If you do someWindow.defineElement("x-tag", XTag) and then new otherWindow.XTag() we have a IMO fatal ambiguity. It needs to be mandatory to clear up that ambiguity (2), or we need to eliminate the ambiguity (1).

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2016

(Maybe a good way to implement (1) is to check that the passed constructor's realm's global object is equal to the context object's associated window. So otherWindow.defineElement("x-tag", XTagDefinedInDifferentGlobal) just fails. It's not 100% but it seems like a good sanity check. Probably in addition to the proto-walking @rniwa proposed a few times.)

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

I think I prefer 1. And we later figure out how you get custom elements to instantiate in documents not associated with a window object (if ever, obviously that's not a good idea for XMLHttpRequest's document, unless you can only do this action after you get a hold of the document).

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2016

In IRC @jakearchibald mentioned a third alternative which has been floating around in my brain for a while:

  1. Maintain a per-event loop (really per unit of related similar-origin browsing contexts, since workers aren't relevant) mapping table of custom element constructors to documents. This could ensure that new CustomElement() always knows what document to work in.
  • This is similar to (1) but means there are separate registries for createDocument()-documents, instead of sharing them with the parent or prohibiting registration there.
  • This breaks the connection document.createElement("x-tag") <-> new XTag().
  • But on the other hand if you do someOtherDoc.defineElement("x-tag", XTag) and then new XTag() you probably know what you're doing and will understand that the equivalent is someOtherDoc.createElement("x-tag").

@csuwildcat
Copy link

I've yet to hit the multi-doc issue in practice because I've personally stayed in the proto definition/registration lane, but always wondered how one might deal with it. Glad to see it fleshed out in such thoughtful detail.

Oh, and thank you for continually using "X-Tag" in your examples ;)

@rniwa
Copy link
Collaborator

rniwa commented Mar 3, 2016

I definitely don't want option 3. Magical implicit relationships like that are hard to understand and will bite authors in edge cases; e.g. imagine script calling into a library which creates a window-less document and calls defineElement on it, and then proceeds to construct an element in another document. Something like that WILL happen and it's not obvious at all as to why that didn't work as intended.

I'm fine with either options 1 or 2. I'll add option 4:

  1. Add an optional document argument to custom elements constructor and HTMLElement constructor. UA will always specify this argument when creating an element inside DOM APIs or in the parser, and verifies that the element returned by the constructor is indeed owned by the specified document. HTMLElement constructor will fallback to use the document associated with the global object if document is not specified in the first argument.

This will allow new MyCustomElement (without arguments) but effectively forces authors to pass in the arguments in the custom elements constructor. If they forget, custom elements don't work in window-less documents but those authors probably won't call defineElement on those documents in the first place.

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2016

Yeah, I was saying in #369 (comment) that I was not ok with (4). It should either be unambiguous or required.

It sounds like we should go with (1). The remaining questions to me are:

  • What should otherDoc.defineElement do? Always throw, or operate on a shared registry with its "creator document"?
  • What should otherDoc.createElement("custom-element") and the parser in that other doc do? HTMLUnknownElement, or use a shared registry?

The current spec shares the registry. If we want to continue with that, maybe we should move the registry to window instead of document... document.defineElement is looking like a stranger and stranger API, as opposed to window.defineElement.

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

Yeah, I like making the registry per-realm, but we need to have something to enable its usage on a per-document level.

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2016

but we need to have something to enable its usage on a per-document level.

Hmm, what do you mean by that?

@annevk
Copy link
Collaborator

annevk commented Mar 3, 2016

So <template> and XMLHttpRequest don't end up using it. Maybe there can be an opt-in for that at some point, but probably not right away. It's a little weird that the opt-in is only for custom elements, but I don't see a way around that.

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2016

So I guess putting on Window doesn't work actually if we want to share since createDocument() documents don't have any associated window. So let's keep it on document and create patches for createDocument() and createHTMLDocument() that hand out the registry to those.

The current spec gives template a new empty registry but it seems like that's a bug per recent discussions and instead it should just have no registry so that defineElement() always fails there. (Same with XHR.)

I will try to work on a PR for this today. It sounds like we have a rough plan which is best critiqued in concrete form. I might tackle #413 first since it's getting in the way.

@rniwa
Copy link
Collaborator

rniwa commented Mar 3, 2016

So it turns out that we can't share the registry in option 1 because ownerDocument would need to be window.document initially and this is observable in custom elements constructor. So I think we need to disallow custom elements in window-less documents.

kisg pushed a commit to paul99/webkit-mips that referenced this issue Mar 4, 2016
Disallow custom elements inside a window-less documents
https://bugs.webkit.org/show_bug.cgi?id=154944
<rdar://problem/24944875>

Reviewed by Antti Koivisto.

Disallow custom elements inside a window-less documents such as the shared inert document of template elements
and the ones created by DOMImplementation.createDocument and DOMImplementation.createHTMLDocument.

Throw NotSupportedError in defineCustomElement when it's called in such a document as discussed in:
WICG/webcomponents#369

Tests: fast/custom-elements/parser/parser-constructs-custom-element-in-document-write.html
       fast/custom-elements/parser/parser-uses-registry-of-owner-document.html

* bindings/js/JSDOMBinding.cpp:
(WebCore::throwNotSupportedError): Added.
* bindings/js/JSDOMBinding.h:
* bindings/js/JSDocumentCustom.cpp:
(WebCore::JSDocument::defineCustomElement): Throw NotSupportedError when the context object's document doesn't
have a browsing context (i.e. window-less).
* html/parser/HTMLDocumentParser.cpp:
(WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder): Replaced a FIXME with an assertion now that we
disallow instantiation of custom elements inside a template element.

LayoutTests:
Disallow custom elements inside template elements and share the registry for windowless documents
https://bugs.webkit.org/show_bug.cgi?id=154944
<rdar://problem/24944875>

Reviewed by Antti Koivisto.

Added various tests to ensure the custom elements registry is not shared between documents with
distinct browsing context (e.g. iframes) but shared among the ones that share a single browsing context
(e.g. documents created by DOMImplementation).

Also added a test case for defineCustomElement to ensure it throws NotSupportedError when it's called on
a template element's inert owner document as well as a basic test case for document.write.

* fast/custom-elements/Document-defineCustomElement-expected.txt:
* fast/custom-elements/Document-defineCustomElement.html: Added a new test case.
* fast/custom-elements/parser/parser-constructs-custom-element-in-document-write-expected.txt: Added.
* fast/custom-elements/parser/parser-constructs-custom-element-in-document-write.html: Added.
* fast/custom-elements/parser/parser-uses-registry-of-owner-document-expected.txt: Added.
* fast/custom-elements/parser/parser-uses-registry-of-owner-document.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@197528 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@annevk
Copy link
Collaborator

annevk commented Mar 4, 2016

All documents (all platform objects, really) have an associated global object per IDL. Most don't have a browsing context, however, but nobody is proposing to put the registry there.

@rniwa can you explain what the problem is with the initial node document being the document associated with the global object?

@rniwa
Copy link
Collaborator

rniwa commented Mar 4, 2016

@annevk : It would mean that custom element's constructor would see a wrong ownerDocument until it gets adopted into that document.

It's not a huge deal per se but it's yet another edge case author has to know / be aware of. We think it's better to disallow it altogether in the first place so that the behavior is consistent and easy to understand.

@annevk
Copy link
Collaborator

annevk commented Mar 4, 2016

@rniwa your custom element will have to be resilient against that anyway as I can easily imagine folks will construct it in A and insert it in B all the same. (It also matches what we do for new Text() and friends.)

@rniwa
Copy link
Collaborator

rniwa commented Mar 4, 2016

@annevk what are use cases for custom elements in window-less document anyway? What do people use that for?

Unless there are use cases for these documents, we would rather not support it.

@annevk
Copy link
Collaborator

annevk commented Mar 4, 2016

  1. How do you prevent them from being used there?
  2. Who says they are window-less? You could be inserting them into an <iframe>.

But maybe we are saying the same, since I think that by default, custom elements should only instantiate automatically in documents with a browsing context. And for v1 we should not go beyond that. The other thing I'm saying is that I don't see a reason not to associate the registry with a global and put the API there (although maybe it should be stored on that global's document due to window.open() and such).

@domenic
Copy link
Collaborator Author

domenic commented Mar 7, 2016

Please take a look at 58a2f7b which implements our consensus here, I believe. Output at the bottom of https://w3c.github.io/webcomponents/spec/custom/#concepts.

@annevk
Copy link
Collaborator

annevk commented Mar 8, 2016

I think I still favor a slightly different model:

  1. Registry is per realm
  2. Documents by default don't use the registry
  3. Documents with a browsing context use the registry

That would also mean we keep the registry when you navigate an initial about:blank which I think is what we want. The document.open() semantics would also fall out more naturally.

I guess I can live with the current model, but this seems more logical.

@annevk annevk reopened this Mar 8, 2016
@rniwa
Copy link
Collaborator

rniwa commented Mar 8, 2016

Sure, I was using window-less and "without a browsing context" synonymously ignoring those details. Your definition makes sense to me.

@domenic
Copy link
Collaborator Author

domenic commented Mar 8, 2016

@annevk one thing I prefer about the current model is that it's easy to define in DOM without having DOM define properties of the current realm or window, or knowing about browsing contexts. Instead DOM just says that a document has a registry, potentially null, and lets HTML set up a new registry when appropriate. Anything else seems to involve too much intertwingling.

That would also mean we keep the registry when you navigate an initial about:blank which I think is what we want.

I don't really agree. I think if you've added elements to the initial about:blank, for whatever reason, they should probably go away when you navigate.

@annevk
Copy link
Collaborator

annevk commented Mar 9, 2016

I don't really agree. I think if you've added elements to the initial about:blank, for whatever reason, they should probably go away when you navigate.

So you get to "define" all kinds of JavaScript objects there, but not elements?

Note that the layering between DOM and HTML is already muddy. E.g., "get the parent" on documents returns the WindowProxy object. Mutation observers depend the "unit of related similar-origin browsing contexts", etc.

It's hard to make concrete, but I think in the long run we'll find that we want the registry to be per realm since there's no way to scope these constructors to documents (other than if we made it a mandatory argument, which seems rather sad). I'm not a 100% on this though, it's a hunch.

@domenic domenic assigned domenic and unassigned annevk Mar 11, 2016
domenic added a commit that referenced this issue Mar 12, 2016
- Figured out where custom elements will go in HTML, and started writing as if they were there
- Fleshed out and moved around examples and motivation to form an "introduction" section
- Separated out and rearranged concepts a bit better (fixes #437)
- Started the process of moving the registry to the browsing context instead of document (#369)
- Moved to `window.customElements`, an instance of `CustomElementsRegistry`, with one `define` method currently (#431)

Still to-do:

- fix all broken links detected by respec; a few terms have changed
- Look up registry on the browsing context instead of the document
- Move custom element semantics into the "HTML+: Custom elements" section
- Define a HTML+ patch that adds a `customElements` property to `Window`.
@domenic
Copy link
Collaborator Author

domenic commented Mar 15, 2016

ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
Disallow custom elements inside a window-less documents
https://bugs.webkit.org/show_bug.cgi?id=154944
<rdar://problem/24944875>

Reviewed by Antti Koivisto.

Disallow custom elements inside a window-less documents such as the shared inert document of template elements
and the ones created by DOMImplementation.createDocument and DOMImplementation.createHTMLDocument.

Throw NotSupportedError in defineCustomElement when it's called in such a document as discussed in:
WICG/webcomponents#369

Tests: fast/custom-elements/parser/parser-constructs-custom-element-in-document-write.html
       fast/custom-elements/parser/parser-uses-registry-of-owner-document.html

* bindings/js/JSDOMBinding.cpp:
(WebCore::throwNotSupportedError): Added.
* bindings/js/JSDOMBinding.h:
* bindings/js/JSDocumentCustom.cpp:
(WebCore::JSDocument::defineCustomElement): Throw NotSupportedError when the context object's document doesn't
have a browsing context (i.e. window-less).
* html/parser/HTMLDocumentParser.cpp:
(WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder): Replaced a FIXME with an assertion now that we
disallow instantiation of custom elements inside a template element.

LayoutTests:
Disallow custom elements inside template elements and share the registry for windowless documents
https://bugs.webkit.org/show_bug.cgi?id=154944
<rdar://problem/24944875>

Reviewed by Antti Koivisto.

Added various tests to ensure the custom elements registry is not shared between documents with
distinct browsing context (e.g. iframes) but shared among the ones that share a single browsing context
(e.g. documents created by DOMImplementation).

Also added a test case for defineCustomElement to ensure it throws NotSupportedError when it's called on
a template element's inert owner document as well as a basic test case for document.write.

* fast/custom-elements/Document-defineCustomElement-expected.txt:
* fast/custom-elements/Document-defineCustomElement.html: Added a new test case.
* fast/custom-elements/parser/parser-constructs-custom-element-in-document-write-expected.txt: Added.
* fast/custom-elements/parser/parser-constructs-custom-element-in-document-write.html: Added.
* fast/custom-elements/parser/parser-uses-registry-of-owner-document-expected.txt: Added.
* fast/custom-elements/parser/parser-uses-registry-of-owner-document.html: Added.


Canonical link: https://commits.webkit.org/173073@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@197528 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants