Referential integrity is lost on replace()

[DesignByOnyx]

Describe the bug
The object returned by replace is not the same as the original instance. I'm not sure if this is a bug, but it differs from Chrome's implementation.

To Reproduce

const sheet = new CSSStyleSheet();
const css = '/* all the css */';

sheet.replace(css).then(theSheet => {
  console.log(theSheet === sheet); // true in Chrome, false in Firefox
});

Expected behavior
Being that Chrome has native support for the CSSStyleSheet constructor, I would expect the polyfill should stay in line with their implementation until a more formal spec is in place.

[DesignByOnyx]

If you want to see why this is a problem, this is what we are doing:

// App.js
const sheet = new CSSStyleSheet();
const css = '/* all the css */';

sheet.replace(css).then(theSheet => {
  window.OUR_COMPANY.sharedStyles = theSheet;
});

and then later on:

// SomeComponent.js
this.el.shadowRoot.adoptedStyleSheets = [window.OUR_COMPANY.sharedStyles];

The above works natively in Chrome but fails with the polyfill (in Firefox). The problem is that theSheet (returned from replace) is not the same sheet which is registered in the sheetMetadataRegistry. As such, the lookup fails here: https://github.com/calebdwilliams/construct-style-sheets/blob/master/src/adopt.js#L16

[calebdwilliams]

Thanks for raising this issue. It's been awhile since I've been in this code base, but I'm not 100 percent sure this is doable with how the polyfill has to work. I am happy to dive in to this or accept a contribution if you want to take a crack at it.

[DesignByOnyx]

Yeah, I was looking at the code and am not entirely sure it's possible either... not without some major rework. The code essentially reduces down to this:

const styleEl = document.createElement('style');
const origSheet = styleEl.sheet; // This is used as the key in the sheetMetadataRegistry

styleEl.innerHTML = 'p { position: static }'; // this happens inside of replace()
origSheet === styleEl.sheet; // false :(

I might take a stab at fixing this. It would definitely be a major breaking change. Do you have any opinions, advice, or reservations about my doing this? Essentially, what needs to happen is that an instance of ConstructStyleSheet should be "the thing". Right now, the class does this (pseudo code for brevity) - I've added comments to very briefly highlight the changes:

class ConstructStyleSheet {
    constructor() {
        const styleEl = document.createElement('style');
        document.head.appendChild(styleEl);
        // Object.assign(this, styleEl.sheet);
        return styleEl.sheet; // should return "this" instead
    }
    replace(css) {
        styleEl.innerHTML = css;
        // Object.assign(this, styleEl.sheet);
        resolve(styleEl.sheet);  // should resolve to "this" instead
    }
}

... of course the code would be a little more involved once implemented. I just want to see what you think before I move forward.

[calebdwilliams]

Yeah, this isn't going to be strictly possible because of how style elements work. Try this (even in Chrome)

let style = document.createElement('style');
document.body.append(style);
let sheet = style.sheet;
style.innerHTML = '* { color: tomato; }';
console.assert(sheet === style.sheet, 'The sheet property apparently gets replaced instead of modified directly in this instance'); // false

@DesignByOnyx, @Lodin any ideas here?

[Lodin]

Due to the specifics of the polyfill implementation (we are trying to work with the immutable object as the mutable one), I'm not sure we can fix this issue.

However, there is one possible workaround. You could use the sheet instance itself in the closure instead of received theSheet. These two variables should be used in the following case:

• theSheet should be used for retrieving the current CSS rules or any other possible state of the sheet.
• sheet should be used for applying the new state: adding/removing CSS rules using the insertRule, deleteRule, etc., or for putting it to adoptedStyleSheet array.

So, the code should look like the following:

// App.js
const sheet = new CSSStyleSheet();
const css = '/* all the css */';

sheet.replace(css).then(() => {
  window.OUR_COMPANY.sharedStyles = sheet;
});

Would it work for your case?

[calebdwilliams]

Yeah, I've played with this a bit and I'm not sure we're going to get it to work. I would advise using @Lodin's workaround. If you'd like to attempt a contribution to make this work, feel free. I'll keep this issue open for a bit and if there's no activity I'll close it.

[DesignByOnyx]

@Lodin - that is what we are doing on our end, and it works - so I am personally not blocked by this. I just was hoping to save somebody else the same headache. It took two of us a couple hours to figure out the root cause, and we both had bald spots from scratching our heads.

@calebdwilliams - your example echos my previous comment ;). I think I have a solution, however I feel like the essence of what I am suggesting kind of got lost. Let me open a PR and write some tests to highlight what I'm talking about.

[Lodin]

@DesignByOnyx, I think we can mention this case in the documentation to avoid such time loss for other people. Though, I really would like to see the solution you came up with.

[DesignByOnyx]

Give a look at that PR. I didn't want to spend too much time on it yet, but it fixes the issue I brought up in this ticket, and it does it without overwriting methods on the default browser CSSStyleSheet prototype.

I feel like there's a use case that I'm not considering, but I tested this in Firefox and it works as expected. Let me know if you see any situations that I neglected to consider.

[calebdwilliams]

I’ll check that out this weekend. Right off the bat, can you make sure non-constructed stylesheet a throw with the right error message? You are right about the methods being in those sheets in Chrome though that might be a bug. I’ll double check the spec threads.