Instead of assignable FrozenArray, use add / remove

[rniwa]

In cases where this API is used by custom elements, different subclasses may want to add a different stylesheet to adoptedStyleSheets. In that case, it's better to have explicit add and remove methods rather than to have author scripts manipulate FrozenArray.

[domenic]

We can do both! add/remove would be sugar, in addition to allowing access to the list.

[developit]

Just catching up on this - I'm definitely fond of explicit methods. Here's my pitch:

interface DocumentOrShadowRoot {
  void adoptStyleSheet(CSSStyleSheet sheet)
  void removeStyleSheet(CSSStyleSheet sheet)
}

[chrishtr]

The methods as described above might not make it obvious enough that there is an ordering to the style sheets, with later style sheets overriding earlier ones for cases when both apply to an element.

[emilio]

What about prependStyleSheet / appendStyleSheet / removeStyleSheet? That makes the ordering explicit, and if you want something else you just go ahead and manipulate the list yourself.

[emilio]

(Not that I'm particularly fond of it, just dumping thoughts :))

[developit]

Perhaps the FrozenArray used here could be extended to allow indirect modifications like CSSRuleList (document.adoptedStyleSheets.insertSheet(sheet, 0)) or NodeList?

I'll admit it seems odd to add a whole new API for working with an ordered set of like objects.

[surma]

May I ask what the rational was behind making it a frozen array in the first place? It seems a bit odd to make it a frozen array and then re-implement methods to basically allow arbitrary manipulation.

[domenic]

It's not possible to make an array that the browser "watches" for changes. So, if it were a normal array, you'd do document.adoptedStyleSheets.push(foo) and then nothing would happen. The browser can only react to setters/method calls. The frozen array's current API is to use the setter, but this thread is about requesting additionally adding a method call API.

[surma]

Fair. Thank you.

[v-python]

@developit 's comment on Jan 28 is key: because order is important to the list, you need to be able to insert/remove elements from specific positions, not just the front / end.

[rniwa]

Again, the fact people are introducing race conditions, etc... by writing a simple snippet of code in https://github.com/w3c/webcomponents/issues/759#issuecomment-521053064 is yet another evidence that having add/remove is better than having people assign a FrozenArray.

Now I consider this issue as an absolute show stopper. I don't think we want to ever implement this feature in WebKit unless this issue is resolved.

[Rich-Harris]

As an aside, it already feels odd that assigning an array to document.adoptedStyleSheets results in that array being frozen. Adding add and remove methods to it feels very surprising:

const sheets = [...document.adoptedStyleSheets, sheet];
document.adoptedStyleSheets = sheets;

sheets.add(otherSheet); // huh?

Is there precedent for that elsewhere in the DOM? Having a non-reassignable object with methods for adding and removing feels much more natural, notwithstanding the ergonomic issues around prepending (though I'd expect that to be a very small minority of cases, and maybe not the thing to optimise the API for).

[fvsch]

Very anecdotal, but as an author seeing this in examples:

document.adoptedStyleSheets = [ ...document.adoptedStyleSheets, sheet ];

my gut reaction was that it was a useless recreation of the array and I would quickly simplify it to:

document.adoptedStyleSheets.push(sheet);

Maybe the assignment style suggests that any kind of Array manipulation (push, sort, etc.) would work? (Or maybe there are several other web APIs where you set an Array and get a FrozenArray, with similar ergonomics and restrictions, and I'm just not used to them.)

[justinfagnani]

The potential race condition linked to in the CSS modules thread is a problem with any of the built-in data structures like arrays. It's not unique to adoptedStyleSheets at all. I think that any sufficient alternate to adoptedStyleSheets will have possible race conditions if you read from and write to the collection with an intervening await.

[calebdwilliams]

Is there a reason a FrozenArray was chosen as the type as opposed to a StyleSheetList?

Could that interface be modified to support for addition, removal and reordering of stylesheet objects (even as a subclass)?

[domenic]

StyleSheetList is like a frozen Array but with no useful reading methods (map(), filter(), etc.), and one useless method (item()). So that is why we did not choose StyleSheetList.

[calebdwilliams]

Right, but the question was if it could be made to be more useful …

[Rich-Harris]

It's not unique to adoptedStyleSheets at all

No-one is claiming that. But 'this pattern is bug-prone anyway' isn't a reason to embrace it. An API along these lines wouldn't suffer the same problem:

document.whatever.add(await import('./styles.css'));

[tabatkins]

The potential race condition linked to in the CSS modules thread is a problem with any of the built-in data structures like arrays. It's not unique to adoptedStyleSheets at all.

More specifically, it's a problem with using await in an argument to a function (which a literal array construction is, effectively). The JS engine evaluates all preceding arguments before it pauses the function for the await, and that can cause snapshotting bugs as seen in that code.

So that is why we did not choose StyleSheetList.

Yup, StyleSheetList is just one of those shitty legacy "whoops I guess we have to act like Java" interfaces that the old DOM is plagued with. We didn't want to spread it further, since it's kinda actively hostile to web authors.

We can do both! add/remove would be sugar, in addition to allowing access to the list.

With the additional point that we probably want to make prepending as easy as appending, yeah, I'm okay with some sugar methods for append/prepend/remove, and letting array manip handle all the rest, as Emilio suggests.

[matthew-dean]

Yup, StyleSheetList is just one of those shitty legacy "whoops I guess we have to act like Java" interfaces that the old DOM is plagued with.

I know this is a huge undertaking and tangential aside to this discussion, but I wish there was a dedicated effort to writing a new DOM, and standardizing interfaces and methods and deprecating old ones. Like, I'd love if someone attacked the problem of "if we built a DOM from scratch in 2019, what would it look like?"

[calebdwilliams]

Yeah, I guess none of that actually answered the question of if StyleSheetList could be made to solve our current problem. If it exposes "no useful reading methods," what would that look like? Could that class be outfitted with the add, remove, replace, order, etc. methods that would meet the needs of CSS modules?

[rniwa]

Yup, StyleSheetList is just one of those shitty legacy "whoops I guess we have to act like Java" interfaces that the old DOM is plagued with. We didn't want to spread it further, since it's kinda actively hostile to web authors.

I disagree with that proposition. Consistency is important.

Yeah, I guess none of that actually answered the question of if StyleSheetList could be made to solve our current problem.

Yes. In fact, I've suggested that we use StyleSheetList in numerous occasions.

[domenic]

StyleSheetList can't be made modifiable because then it would no longer serve well for document.styleSheets, which is a read-only view onto the list of link/style-element generated style sheets.

[Jamesernator]

It's not possible to make an array that the browser "watches" for changes. So, if it were a normal array,

Could it work to have an exotic subclass of Array that traps all operations on it to update the document on potential changes?

~e.g. If implemented in JS~:

Old example

class StyleSheetArray extends Array {
  #invalidateCurrentStyles() {
    /* Recompute styles, etc, whatever browsers do when new style sheets are added */
  }

  #maybeInvalidateStyles() {
    /* Test if stuff has changed and if so invalidate current styles */
  }

  constructor() {
    super();
    return new Proxy(this, {
      get(...args) {
        const value = Reflect.get(...args);
        if (typeof value === 'function') {
          const that = this;
          return function(...args) {
            const result = Reflect.apply(value, this, args);
            this.#maybeInvalidateStyles();
            return result;
          }
        }
        return value;
      },

      set(...args) {
        const result = Reflect.set(...args); 
        this.#invalidateCurrentStyles();
        return result;
      },

      /* etc for all the other traps */
    });
  }
}

EDIT: Actually array methods are generic things so just having exotic behavior on defineProperty/deleteProperty is probably enough to observe all the changes.

e.g.:

'use strict';

function isArrayIndex(string) {
    if (typeof string !== 'string') {
        return false;
    }
    const number = Number(string);
    return Number.isSafeInteger(number)
        && String(number) === string
        && number >= 0;
}

class StyleSheetArray extends Array {
    #updateStyles() {
         /* Do usual updates when style list changes */
    }

    constructor(...args) {
        super(...args);
        return new Proxy(this, {
            defineProperty: (target, prop, descriptor) => {
                try {
                    return Reflect.defineProperty(target, prop, descriptor);
                } finally {
                    if (prop === 'length' || isArrayIndex(prop)) {
                        this.#updateStyles();
                    }
                }
            },

            deleteProperty: (target, prop) => {
                try {
                    return Reflect.deleteProperty(target, prop);
                } finally {
                    if (prop === 'length' || isArrayIndex(prop)) {
                        this.#updateStyles();
                    }
                }
            },
        });
    }
}

[calebdwilliams]

Should this feature be a sort of LinkedList/LinkedSet? I realize there's no true analog for those concepts in JavaScript/DOM, but implementing that based on current JS should be fairly trivial (in userland at least, I obviously can't speak for browser implementation, but I'm pretty sure most other languages have this concept built in).

That sort of object should have the following methods:

• add (sheet: CSSStyleSheet): LinkedSet
• remove (sheet: CSSStyleSheet): LinkedSet
• insertAt (index: number, sheet: CSSStyleSheet): LinkedSet
• removeFrom (index: number): LinkedSet
• indexOf (sheet: CSSStyleSheet): number
• includes (sheet: CSSStyleSheet): boolean
• Symbol.iterator

Ideally I'd like to see these added to a subclass of StyleSheetList that adoptedStyleSheets could inherit from (for consistency sake). I understand that these can't be added to the base object for the reasons described in #4, but creating this sort of object should be easy enough and could then be made to respond to user actions.

Thoughts:

• Adding a CSSStyleSheet that is already included in the LinkedSet should throw
• Running LinkedSet.insertAt on a CSSStyleSheet that is currently in the list would reorder the list

Edit: Overly-simplistic demo

[emilio]

I think a more consistent naming and API with the existing CSSOM interfaces for inserting / removing rules would be good... CSSOM interfaces have:

• CSSStyleSheet.cssRules
• CSSStyleSheet.insertRule()
• CSSStyleSheet.deleteRule()

And same for CSSGroupingRule. So maybe:

• ShadowRoot.adoptedStyleSheets (that would allow getting, not setting a frozen array... Or should it be live like cssRules? dunno)
• ShadowRoot.removeSheet
• ShadowRoot.insertSheet

would be the bare minimum? I agree that maybe an appendSheet would be on point, though iteration and includes you'd get for free via the FrozenArray returned by adoptedStyleSheets.

[calebdwilliams]

@emilio, I like the thought, but that would require additional methods on document as well. Maybe DocumentOrShadowRoot.adoptedStyleSheets.insertSheet. Maybe to preserve current functionality, adoptedStyleSheets' setter could take any iterable of CSSStyleSheets?

[heycam]

There is some discussion about an IDL-level feature better than FrozenArray, aimed at solving use cases like adoptedStyleSheets, here: https://github.com/heycam/webidl/issues/796

[fergald]

Repeating a suggestion I made at TPAC...

If we give style elements the ability to adopt a style sheet then I think a lot of problems go away. They can be inserted and deleted just as DOM elements and their ordering vs parsed styles sheets is obvious (and it makes it possible to insert one before, after or even between parsed style elements rather then having to pick before/after in the spec).

Pro:

• no need for new mutable array object or insert/delete IDL
• give full control over ordering vs other sheets in the shadow root
• extends to scoped styles outside of shadow roots or anything else in the future
• no need for an adoptedStyleSheets attribute on Document and ShadowRoot

Con:

• wastes an empty style tag
• a bit more boilerplate
• ???

E.g.

# shadow root
<style>
  foo { ...}
</style>
<style adopted id=adopted></style>
<style>
  bar { ...}
</style>

adopted.adoptedStyleSheet = new CSSStyleSheet();
...

[Jamesernator]

If we give style elements the ability to adopt a style sheet then I think a lot of problems go away. They can be inserted and deleted just as DOM elements and their ordering vs parsed styles sheets is obvious (and it makes it possible to insert one before, after or even between parsed style elements rather then having to pick before/after in the spec).

I like this cause it means we could also integrate CSS modules to be loaded via both JS or HTML.

e.g.:

<style adopted src="./component.css"></style>

import styleSheet from './component.css';

const styleElement = shadowRoot.querySelector("style[adopted]");

styleElement.adoptedStyleSheet === styleSheet; // true