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 WICG/webcomponents#759 (comment) 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.

[domenic]

Since you can't observe an existing array, that would only be possible to spec in cases where you assigned another observable array of the correct type, e.g. document.adoptedStyleSheets = someShadowRoot.adoptedStyleSheets. Making that behave differently than other assignments would add spec complexity, and probably having non-uniform behavior would be confusing.

[mfreed7]

This was discussed today at the Web Components F2F meeting, notes here: https://docs.google.com/document/d/160HetuBh6sLArif1y-heWpe749b3KPnX54AS-aB8UQ4/edit#

We discussed the state of this issue, and the primary open issue, which is assignability. (There seemed to be general consensus on the rest, i.e. that we should move adoptedStyleSheets to use ObservableArray.) We talked for 15-20 minutes about the pros and cons for assignability:

• Main pro: ergonomics. Being able to assign seems to be a big convenience, and conversely, lack of assignability can be confusing.
• Main con: confusion around assigning from a JS array, then mutating that array. In that case, the adoptedStyleSheets would not reflect that mutation.

The tentative resolution from the meeting was that the pros outweigh the cons, and we should move ahead with an assignable ObservableArray. Nobody on the call made the case for removing assignability.

[rniwa]

I must once again object to that resolution.

[saithis]

how about a reset method on ObservableArray?
reset() clears it.
reset(somearray) clears it and fills it with the items of somearray.
then it wouldn't need to be assignable, still be ergonomic and it would be more clear that somearray modifications wouldn't propagate to the ObservableArray.

[annevk]

@rniwa could you follow-up to #45 (comment) please?

[rniwa]

@rniwa could you follow-up to #45 (comment) please?

There is a long history of discussion but I guess the key take away is WICG/webcomponents#759 (comment). The key difference of other properties that allow assignment and this API is that the constructible stylesheet API was made so that parsing happens asynchronously. Even if the parsing itself was made sync, then there is an issue of fetching the stylesheet content itself, which may also involve an asynchronous work.

There were a few people who wrote incorrect code just during the discussion of adoptedStyleSheets discussion. Given people who are engaging in this discussion are more of expert and even they're making this kind of a mistake, I don't believe supporting assignment to this property is going to result in ergonomic API for an average developer.

[rniwa]

By the way, it's highly problematic that this feature doesn't work with declarative shadow DOM.

It's actively harmful for the web platform as a whole to introduce a bunch of new APIs that behave differently to or don't work well with another existing API. Such inconsistencies force more cognitive strains on new developers who are learning to use the web platform, and even experienced developers would have to spend more time navigating through dozens of different documentations and keep reminding themselves of which API uses which programming paradigm or idiom.

[annevk]

I'm not sure how that comment is related. It seems to be from two years ago, which is prior to the introduction of observable arrays. Now that document.adoptedStyleSheets will have a push() method, people will use that as appropriate. And presumably the asynchronous parsing would be enabled by the usage of CSS module scripts, no?

Firefox has no plans to implement declarative shadow DOM, but I think it would be good to get this unblocked. Are the issues with declarative shadow DOM written down in an issue somewhere?

[rniwa]

I'm not sure how that comment is related. It seems to be from two years ago, which is prior to the introduction of observable arrays. Now that document.adoptedStyleSheets will have a push() method, people will use that as appropriate. And presumably the asynchronous parsing would be enabled by the usage of CSS module scripts, no?

Well, the point isn't so much that we need asynchronous parsing, it's that asynchronous parsing combined with assignment will result in a racy code.

I'd also add that we remain skeptical of this whole feature as it introduces yet another way of inserting a stylesheet into a document, and the claim that this will help with the performance when the same stylesheet is used in multiple shadow roots since modern browser engines like WebKit and Gecko have a mechanism to share the underlying data structures across different shadow trees.

[annevk]

@rniwa well, once the import of a CSS module script completes the style sheet is parsed, right? Otherwise you wouldn't have a usable CSSStyleSheet object. What kind of race are you thinking about? (I think @emilio had similar reservations with regards to performance. Not sure if they were alleviated somehow.)

[justinfagnani]

@rniwa as someone is that previous discussion, I think racy code is possible to write with any mutable collection and multiple actors, even with sync resources, and whether or not that mutability comes from replacement (assignability) or mutation. And I don't think that adoptedStyleSheets will typically have multiple actors involved.

In any case, the Lit team's highest priority is to see some feature like this ship, assignable or not. Chrome also has a mechanism to share underlying stylesheet data-structures from <style> tags and still measured a perf increase from using adoptedStyleSheets. Stylesheet references are not just about performance, but an ergonomics / style primitive feature too. Once CSS Modules ship, we don't want to have to get the CSS text from an already parsed stylesheet (which there isn't a great API for) and create a <style> tag and insert that into a ShadowRoot who's contents might be managed by some other template system. It's much easier for developers to import the stylesheet reference and push it on to the stylesheets array.

To me, this pattern of importing CSS modules and adding them to adoptedStyleSheets is the optimal way to load scoped CSS taking advantage of parallel loading, off-main-thread parsing, and sharing the parsed stylesheet result across many components instances.

Regarding integration with declarative shadow DOM, I'm confident we can make this work - it's similar to other cross-scope element reference issues that are being worked on - with a requirement for serializability. I'm not sure if we have a central issue for this yet, but if not, let's open a discuss there - I agree it's critical to get that solved, but don't see any blocker or drastically different alternative that would make it easier.

[EisenbergEffect]

The FAST team at Microsoft is completely aligned with the Lit team on this issue. We have very similar scenarios and concerns. This is an extremely high priority for us.

[emilio]

off-thread main-thread parsing? That's news to me, how would such a thing work? I guess it was just a typo? :)

Performance shouldn't be different from <link rel="stylesheet"> tbh. Compared to a lot of <style> elements you can save some memory overhead (for the text contents of the <style> on each shadow tree), which might be significant, but otherwise it should also be pretty equivalent.

<link rel="stylesheet"> has an even better advantage, which is that it can be shared across navigations in the same origin, which avoid both the fetch() and reparsing the stylesheet. With constructable stylesheets you could avoid the reparsing I guess, but that is a lot more error prone and somewhat annoying (because of base URIs, plus having to hash the whole string like we do for inline styles, instead of just the URI like we do for <link>).

Anyhow, Firefox has an implementation of adopted stylesheets behind a flag and I'd be curious if someone can build a test-case that shows any measurable performance benefit compared to <link rel=stylesheet> :)

But anyhow I think concerns about performance are valid, but kind of off-topic for this particular issue, we can file a separate one.

[justinfagnani]

@emilio

off-thread main-thread parsing?

Sorry, I meant off-main-thread

[justinfagnani]

@emilio

Performance shouldn't be different from <link rel="stylesheet"> tbh

<link rel="stylesheet"> can cause FOUC. We really, really want to be able load all critical resources for a component before potentially partially rendering a component or app.

The dev ergonomics of <link rel="stylesheet"> are also pretty bad compared to just importing a CSS module and applying. There's a reason why bundlers have made importing CSS so popular even though it hasn't been a feature yet browsers.

<link rel="stylesheet"> is also very bad for bundlers because it's not nearly as analyzable as an import. The mostly likely place for a <link rel="stylesheet"> is in a template, so bundlers would have to understand various template syntaxes. Otherwise if the <link> is created imperatively by a helper library from CSS text, the bundler will have to understand where that text is fetched from, and have some way of transforming the stylesheet URL to be a data URL. I'm not even sure how all that could work. It would probably require very library-specific bundler plugins, rather than a system that just works with standard semantics.

[travisleithead]

Given that assignment has this common pattern today (because of a lack of mutation APIs) associated with the adoptedStyleSheets name:

shadow.adoptedStyleSheets = [ ...shadow.adoptedStyleSheets, componentStylesheet ];

Which is not a great pattern to encourage when we do add mutation methods, it seems to me that we will want a new name associated with the new type to force a code-change. It also makes sense to me that we might not want to support assignment in that new API to avoid web developers just replacing the adoptedStyleSheets name with the new name and continuing the less-optimal pattern. (The new name also has the advantage of a new documentation page, news cycle, etc. to help get the word out if we can all agree.)

I suppose there is a use case for clearing any previously assigned stylesheets and adding a whole new set, but based on the discussion above I don't think that's typical. (Clear/reset can be done by setting length = 0 on the ObservableArray correct?)

I could also get behind dropping support for assignment on ObservableArray generally (throw on assignment instead?). The ObservableArray is destined to become the recommended type to be used as an IDL attribute (sequence is not allowed today and FrozenArray as an attribute has existing concerns) and I think we shouldn't dismiss the potential author confusion of the current copy-on-assignment behavior vs the typical JS assignment behavior authors are used to for IDL attributes. (And if we throw on assignment initially, we can safely introduce assignment behavior later if desired.)

I don't find the race condition argument for dropping assignment support particularly compelling. As others have pointed out, a potential race condition will still be present even without supporting assignment. Even with ObservableArray, the push method is variadic, and there are plenty of opportunities for async interruptions in map, filter, etc.

document.newAdoptedStyleSheets.push( localSheet1, localSheet2, await import('./remoteSheet3.css') );

[emilio]

@justinfagnani sure, I was not debating the ergonomics of the API, just the perf argument :)

[justinfagnani]

Let me restate that from my team's perspective, it's much, much higher priority that this feature ship in all browsers than that the property remain assignable and keep the same name.

[emilio]

Sure, I agree! We discussed this on the last F2F and I'm totally fine with the conclusion we reached there. I was just pointing out that @rniwa's initial objection and the performance concerns are two totally different things... I mentioned this in the F2F, but if ObservableArray is compatible with assignability, I personally don't think changing the name just to remove assignability is worth the churn (for both authors and implementors alike).

[trusktr]

I've been using the adoptedStyleSheets Proxy Array in Chrome, and its quite nice. It's great that we get standard methods for working with lists, unlike the other array-like APIs.

Here's one way to remove a sheet, for example:

root.adoptedStyleSheets = root.adoptedStyleSheets.filter(s => someSheet !== s)

With this API, writing racy code would be my own fault, not adoptedStyleSheet's. Async/await has nothing to do with synchronous arrays.

If people really want to "solve racy code" then a possibility is supporting Reponse, Promise<Response>, and Promise<CSSStyleSheet> objects in the array, and internally swapping them in place later when they resolve (cached if they are from the same URL).

root.adoptedStyleSheets = [fetch('foo.css'), getStyleSheetAsync(), styleSheet]
root.adoptedStyleSheets.push(fetch('another.css'))

But I'd argue this is unnecessary because the developer will also need to know when things are loaded if they wish to avoid FOUC, which means they need to handle async code themselves anyway.

Handling async code is a basic part of the web developer job description. Trying to prevent people from having to know how to handle async code is probably futile.

[mfreed7]

I think this issue can be closed, since the spec now uses ObservableArray, and multiple browsers have shipped this behavior.