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

Clarify "settings object" things when calling a custom element constructor #2381

Closed
EdgarChen opened this issue Feb 22, 2017 · 20 comments
Closed
Assignees
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)

Comments

@EdgarChen
Copy link
Member

EdgarChen commented Feb 22, 2017

There are two cases will call custom element constructor:

Both of them seems not handle settings object. Do they need to handle settings object? Or it is already handled in some place.

@domenic @annevk

@annevk annevk added the topic: custom elements Relates to custom elements (as defined in DOM and HTML) label Feb 22, 2017
@bzbarsky
Copy link
Contributor

Specifically, what needs handling is entry/incumbent stuff around the Construct() invocations.

@domenic
Copy link
Member

domenic commented Feb 22, 2017

I think most of the time these are invoked synchronously from code that has already set up things, but I'll do an audit.

@bzbarsky
Copy link
Contributor

Seems like we'd at least want to set the incumbent to whatever set up the custom element definition, etc...

@EdgarChen
Copy link
Member Author

Consider following examples:

<!-- a.html -->
<!DOCTYPE html>
<iframe src="b.html"></iframe>
<script>
  class UnresolvedElement extends frames[0].HTMLElement {};
  frames[0].customElements.define('unresolved-element', UnresolvedElement);
</script>

<!-- b.html -->
<!DOCTYPE html>
<unresolved-element></unresolved-element>
  1. When the algorithm for define [1] runs, the entry setting object is the settings objects of a.html and the incumbent is that of a.html, too.
  2. Since define is annotated with the [CEReactions] extended attribute in idl, custom element reactions will be invoked [2] after define steps.
  3. Then constructor of UnresolvedElement will be invoked during upgrading steps [3].

[1] https://html.spec.whatwg.org/multipage/scripting.html#element-definition
[2] https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-reactions
[3] https://html.spec.whatwg.org/multipage/scripting.html#concept-upgrade-an-element

and

<!-- a.html -->
<!DOCTYPE html>
<iframe src="b.html"></iframe>
<script>
  class UnresolvedElement extends frames[0].HTMLElement {};
  frames[0].defineCustomElement(UnresolvedElement);
</script>

<!-- b.html -->
<!DOCTYPE html>
<unresolved-element></unresolved-element>
<script>
  window.defineCustomElement = (definition) => {
    customElements.define('unresolved-element', definition);
  };
</script>
  1. When the algorithm for define in this example runs, the entry setting object is still that of a.html but the incumbent is that of b.html.
  2. and 3. are the same steps in previous example.

Above two examples have different incumbent when custom element constructor is invoked (If my understanding is correct). Are the incumbent setting objects expected in above two examples?

@domenic
Copy link
Member

domenic commented Mar 7, 2017

I suppose we should probably make the incumbents match. I'm a bit loathe to put more work into the incumbent mess, but it's better to be consistent with the rest of the spec I guess.

Do you have a similar example where #concept-create-element might be affected, or just upgrades so far?

@annevk
Copy link
Member

annevk commented Mar 8, 2017

High-level question: are we any closer to figuring out what incumbent means, if anything, in non-Gecko implementations? Last time we discussed this nobody really knew and they might not even have such a concept, so I'm wondering if we're just keeping Gecko technical debt alive because we don't really know what else to do?

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 8, 2017

Non-Gecko implementations certainly have a concept of "incumbent" (e.g. they do it for postMessage if you call it in the simple way). It's just really ad-hoc and kinda broken in some of them.

I'm wondering if we're just keeping Gecko technical debt alive

Do you mean the concept of incumbent at all, or its implementation? The concept is required for Window.postMessage to work at all. The implementation is not too complicated if you've already made the hard case (which is the normal case!) work.

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 8, 2017

And maybe the point here is that there are no constructors in the web platform which examine the incumbent, in which case it doesn't so much matter what we do with it around the Construct() call. Using a bound postMessage will throw if you try to Construct() it, of course.

@annevk
Copy link
Member

annevk commented Mar 8, 2017

Apologies, I didn't realize there is one observable point where we are in fact interoperable. Do we have bugs filed against Chrome/Edge/WebKit to implement incumbent properly then?

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 8, 2017

It's possible that the incumbent stuff in Location is interoperable too, in the simple cases. Would need to test...

I can't speak to bugs existing or not.

@EdgarChen
Copy link
Member Author

similar examples for #concept-create-element,

<!-- a.html -->
<!DOCTYPE html>
<iframe src="b.html"></iframe>
<script>
  class MyCustomElement extends frames[0].HTMLElement {};
  frames[0].customElements.define('my-custom-element', MyCustomElement);
  frames[0].document.createElement('my-custom-element');
</script>

<!-- b.html -->
<!-- simply be an empty document -->
  1. When the algorithm for createElement [1] runs, the entry setting object is the settings objects of a.html and the incumbent is that of a.html, too.
  2. And the constructor of MyCustomElement will be invoked in step 6.2 of [2].

[1] https://dom.spec.whatwg.org/#dom-document-createelement
[2] https://dom.spec.whatwg.org/#concept-create-element

and

<!-- a.html -->
<!DOCTYPE html>
<iframe src="b.html"></iframe>
<script>
  class MyCustomElement extends frames[0].HTMLElement {};
  frames[0].customElements.define('my-custom-element', MyCustomElement);
  frames[0].createCustomElement('my-custom-element');
</script>

<!-- b.html -->
<!DOCTYPE html>
<script>
  window.createCustomElement = (name) => {
    document.createElement(name);
  };
</script>
  1. When the algorithm for createElement in this example runs, the entry setting object is still that of a.html but the incumbent is that of b.html.
  2. (the same step in previous example)

@domenic
Copy link
Member

domenic commented Mar 8, 2017

Thanks @EdgarChen, I'll work on fixing.

And maybe the point here is that there are no constructors in the web platform which examine the incumbent, in which case it doesn't so much matter what we do with it around the Construct() call. Using a bound postMessage will throw if you try to Construct() it, of course.

Well, it matters because it runs the MyCustomElement constructor, which could e.g. call window.postMessage, right?

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 8, 2017

Yes, but at that point the incumbent will be whatever the global of the Realm of the MyCustomElement constructor is, assuming that constructor is not a built-in function.

The incumbent setup around calling into script is only needed when directly invoking built-in functions, basically. Any time you've made it out into an actual scripted function, your incumbent is now the global of that function.

@annevk
Copy link
Member

annevk commented Mar 8, 2017

I can't speak to bugs existing or not.

@domenic did you file any when you write web-platform-tests? If not, if you can point me to a web-platform-tests test that cover this, I'm happy to do it. I'd really like Chrome and WebKit engineers to get involved somehow in this ongoing issue.

@domenic
Copy link
Member

domenic commented Mar 8, 2017

Yes, but at that point the incumbent will be whatever the global of the Realm of the MyCustomElement constructor is, assuming that constructor is not a built-in function.

The incumbent setup around calling into script is only needed when directly invoking built-in functions, basically. Any time you've made it out into an actual scripted function, your incumbent is now the global of that function.

Right, OK... Hmm. So this issue doesn't need any fix? There are certainly no constructors on the web platform that inspect the incumbent and will also pass the requirements of customElements.define(). (Namely, being a subclass of HTMLElement, but not corresponding to a built-in tag name.)

Or do you think we should still fix it, because relying on this kind of tenuous connection between parts of the system to uphold our invariants is fragile? If we fix it in that case though, it sounds like it's an un-testable change, right?

I hope I'm not completely lost here :)

did you file any when you write web-platform-tests? If not, if you can point me to a web-platform-tests test that cover this, I'm happy to do it. I'd really like Chrome and WebKit engineers to get involved somehow in this ongoing issue.

I don't think we're quite there yet. We need to work through the rest of #1430 first, writing tests to discover which of those cases actually use incumbent (my guess is only ~3 of them will end up doing so, across 2+ browsers). Once we have an actual list of places on the platform that definitely use incumbent everywhere, we'll have a good test suite. And then we can work on fleshing out that test suite to cover edge cases that only Gecko gets right.

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 8, 2017

There are certainly no constructors on the web platform that inspect the incumbent

Apart from https://html.spec.whatwg.org/multipage/comms.html#dom-broadcastchannel but I guess that can never be a subclass of HTMLElement? Or can it, if people mess with the proto chains?

@domenic
Copy link
Member

domenic commented Mar 8, 2017

but I guess that can never be a subclass of HTMLElement? Or can it, if people mess with the proto chains?

Ugh... upon further inspection, it looks like registering BroadcastChannel as an element constructor will pass through define(). Any attempts to create an element with the registered tag name will then call Construct(BroadcastChannel), and inspect the returned value, notice it doesn't implement the HTMLElement brand check, and throw. But BroadcastChannel will still be called...

I guess in this case we get lucky since passing no arguments to BroadcastChannel will immediately throw, before it inspects the incumbent?

So again, very fragile... probably we should fix this, but doing so will be un-testable. Right?

(Of course this all assumes BroadcastChannel actually uses the incumbent settings object in implementations, which remains to be proven.)

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 8, 2017

So again, very fragile... probably we should fix this, but doing so will be un-testable. Right?

Yes.

I can't figure out a way to do the equivalent of bind() with ctors in ES; I think there is nothing right now. But that doesn't mean something won't get added. :(

I agree having it not be testable is quite unfortunate.

this all assumes BroadcastChannel actually uses the incumbent settings object in implementations

It sure seems to in Gecko, fwiw. I can't speak for others.

@domenic
Copy link
Member

domenic commented Mar 23, 2017

I think there is an observable consequence here besides just settings object stuff. Namely, we currently don't {prepare to, clean up after} {running script, calling a callback}. In particular this means that a microtask checkpoint does not happen at the proper time. That seems bad; it should be happening after the constructor.

This observable consequence was noticed at https://bugs.chromium.org/p/chromium/issues/detail?id=703802. Blink seems to already trigger a microtask checkpoint here. We should fix this and write web platform tests.

domenic added a commit to whatwg/webidl that referenced this issue Mar 23, 2017
This helps move some of the heavy lifting into IDL, to avoid issues like whatwg/html#2381.
domenic added a commit that referenced this issue Mar 23, 2017
This fixes part of #2381 (the other part is in DOM's "create an
element").
domenic added a commit to whatwg/dom that referenced this issue Mar 23, 2017
domenic added a commit to whatwg/dom that referenced this issue Mar 23, 2017
domenic added a commit that referenced this issue Mar 23, 2017
This fixes part of #2381 (the other part is in DOM's "create an
element").
domenic added a commit to whatwg/webidl that referenced this issue Mar 27, 2017
This helps move some of the heavy lifting into IDL, to avoid issues like whatwg/html#2381.
domenic added a commit to whatwg/dom that referenced this issue Mar 27, 2017
domenic added a commit that referenced this issue Mar 27, 2017
This fixes part of #2381 (the other part is in DOM's "create an
element"). Note this has two consequences, as discussed there:

* Properly managing the entry and incumbent concepts. This is
  technically unobservable in the current spec landscape.
* Ensuring microtasks are run, if the construct is initiated with an
  empty stack (such as during the parser).
domenic added a commit to whatwg/dom that referenced this issue Mar 28, 2017
domenic added a commit that referenced this issue Mar 28, 2017
This fixes part of #2381 (the other part is in DOM's "create an
element"). Note this has two consequences, as discussed there:

* Properly managing the entry and incumbent concepts. This is
  technically unobservable in the current spec landscape.
* Ensuring microtasks are run, if the construct is initiated with an
  empty stack (such as during the parser).

Tests: web-platform-tests/wpt#5208
@domenic
Copy link
Member

domenic commented Mar 28, 2017

Fixed by whatwg/dom#430 and #2457 with tests at web-platform-tests/wpt#5208 :).

@domenic domenic closed this as completed Mar 28, 2017
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
This fixes part of whatwg#2381 (the other part is in DOM's "create an
element"). Note this has two consequences, as discussed there:

* Properly managing the entry and incumbent concepts. This is
  technically unobservable in the current spec landscape.
* Ensuring microtasks are run, if the construct is initiated with an
  empty stack (such as during the parser).

Tests: web-platform-tests/wpt#5208
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)
Development

No branches or pull requests

4 participants