Spec should define which global object to use for promises

nordzilla commented 4 years ago

Consider when CSSStyleSheet.replace() needs to create a promise to either resolve or reject. In most cases, we can create a promise with the global object that we retrieve from the sheet's associated document. For constructed style sheets, the associated document would be the sheet's constructor document.

There are some edge cases, however, where the sheet may not have an associated document from which to retrieve a global object:

let style = document.createElement("style");
document.head.appendChild(style);
let sheet = style.sheet;
style.remove(); // sheet's associated document becomes null.
sheet.replace('');

Ideally, this code should still return a rejected promise with a NotAllowedError since this is not a constructed sheet. But it should be well-defined in the spec which global object should be used to create the promise in the event that there is no associated document.

It appears that Blink uses the caller's global object in this case.

tabatkins commented 4 years ago

Okay, so reviewing the relevant section of HTML, I think the right answer is:

If the sheet has an associated document, the "relevant Realm" of the document.
If not, the "relevant Realm" of the sheet.

I think you're not allowed to move sheets between documents anymore, so it's actually probably possible to skip (1) and just do (2).

("Caller's global object" implies it's one of the other Realms defined by the spec, which HTML recommends against using.)

nordzilla commented 4 years ago

@tabatkins thank you for your response, and for linking to the relevant section of the spec. I'm implementing this in Firefox right now, and I want to make sure that we do the correct thing.

The entry for Relevant global says:

Every platform object has a relevant Realm, which is roughly the JavaScript realm in which it was created.

In my current implementation, I am ensuring that I hang on to the global object from the associated document (before the associated document is nulled out), so that I can still refer to that same global to create the promise. I would say that this is the global from "the JavaScript realm in which it [the sheet object] was created."

In the following test case: https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=7879

<!DOCTYPE html>
<input type="button" value="start" onclick="test()"/>
<script>
function test() {
    var f = document.createElement("iframe");
    document.body.appendChild(f);

    var style = f.contentDocument.createElement("style");
    f.contentDocument.body.appendChild(style);
    var sheet = style.sheet;
    style.remove();
    let p = sheet.replace('').then(()=> { w("resolve"); }, () => { w("reject"); });
    f.remove();

    Promise.resolve().then(p).then(() => w("test1!"));
    Promise.resolve().then(() => p).then(() => w("test2!"));
    Promise.all([Promise.resolve(), p]).then(() => w("test All!"));
}
</script>

when I explicitly use the global that I got from the associated document to create the replace() promise, I see only:

log: reject
log: test1!

But when I run this same test in Chrome, I see:

log: reject
log: test1!
log: test2!
log: test All!

Where as, for example, if I force replace() to use the Incumbent global, I also see all of the logs.

This is the context with which I said that it seems to me that Blink is using the global from the caller.

tabatkins commented 4 years ago

In my current implementation, I am ensuring that I hang on to the global object from the associated document (before the associated document is nulled out), so that I can still refer to that same global to create the promise. I would say that this is the global from "the JavaScript realm in which it [the sheet object] was created."

Yes, that should be correct, afaik.

Note, tho, that your tests are not testing what you think they are.

/* 1 */ let p = sheet.replace('').then(()=> { w("resolve"); }, () => { w("reject"); });
f.remove();

/* 2 */ Promise.resolve().then(p).then(() => w("test1!"));
/* 3 */ Promise.resolve().then(() => p).then(() => w("test2!"));
/* 4 */ Promise.all([Promise.resolve(), p]).then(() => w("test All!"));

In (1), sheet.replace('') produces a rejected promise, which you can see by both impls outputting "reject". But the .then() returns a fulfilled promise - the rejection handler returns undefined, which is a normal value rather than an exception, so it just returns a Promise fulfilled with undefined, and assigns that to p.

In (2), calling .then(p) is a no-op; .then() takes callbacks, but you've passed it a promise; since that's not a function, it's just ignored and resolves to the same thing as the Promise.resolve() that it's chained off of, another Promise fulfilled with undefined. So "test1!" gets output by the resolve handler.

In (3) the promise successfully integrates p, but p is fulfilled with undefined, so this line also outputs "test2!" from the resolve handler.

And (4) is the same - p is a fulfilled promise, so P.all resolves.

I'm not sure what's going on that would make Firefox act differently based on your handling of constructable stylesheets.

WICG / construct-stylesheets

Spec should define which global object to use for promises #128