Consider disallowing duplicate stylesheets about construct-stylesheets

[tabatkins]

Oh, hm, is that just an implementation detail of how .reverse() is defined? That's very surprising.

This sort of accidental and hidden problem leans me more toward "this is making an Array act like a Set; we shouldn't do it". There's indeed very little, if any, reason for an author to intentionally insert the same stylesheet twice, but I also don't think it's a common problem crying for protection; I think it's a slightly-nice-to-have as long as there's nothing else pushing against it, but here is a good reason to push against it.

[tabatkins]

This might be reasonable. You can still end up generating identical stylesheets from the same strings that would still work, and I'm glad that would still work, to avoid accidental blocking-at-a-distance from different libraries managing to insert the same stylesheet somehow.

However, this does make it act less like an Array and more like a Set, so I'm not sure if it's well-justified. I could go either way.

[nordzilla]

To be clear, I think that identical stylesheets (i.e. same content, different address) should reasonably be allowed.

Adding the same stylesheet (i.e. same content, same address) seems like it would only ever be done by mistake.

This restriction would make adoptedStyleSheets a bit more set-like; but ultimately the order still matters, unlike a set.

[nordzilla]

I want to point out that one of my primary concerns about this is if we eventually move away from the current FrozenArray semantics for adoptedStyleSheets.

Right now, since mutating adoptedStyleSheets requires re-assignment, having the same sheet represented multiple times is not an issue, because everything will simply be cleared and re-assigned.

In the future, if we implement the ability to add/remove sheets in some sort of observably array, such as the one proposed (here), having the same sheet represented more than once could present some strange edge cases. And I still don't think that including the exact same sheet multiple times would be particularly useful.

[heycam]

If we end up moving to using that ObservableArray proposal, and we use a spec hook to disallow duplicates, then that could have surprising effects such as preventing .reverse() from working when called on the array, since shortly into the reversal, you'll have duplicate StyleSheet objects in the array, which would cause an exception to be thrown, even though the end state of the reversal is fine. Maybe that's an acceptable trade-off for wanting to disallow duplicates and moving to a better array-like API?

[css-meeting-bot]

The CSS Working Group just discussed Consider disallowing duplicate stylesheets, and agreed to the following:

• RESOLVED: Close no change to normative text but add a note about needing impl experience

The full IRC log of that discussion

<dael> Topic: Consider disallowing duplicate stylesheets
<dael> github: https://github.com//issues/120
<dael> TabAtkins: I'm happy to talk on this b/c we want to port it over
<dael> TabAtkins: Don't know who poster is but they said there's no good reason to put the same object in the list twice. Would be useful to have a check against that to not allow insertion if same thing shows twice
<dael> TabAtkins: Generally agree there isn't a use case to put same object in twice. heycam pointed out webIDL to deal with array do temp cause same object to show twice like reverse. It would be confusing and bad if putting this is means we can't reverse the array.
<dael> TabAtkins: Given a decent arguement against the requirement and it was a nice to have I think we should reject and keep it that this uses normal array semantics.
<dael> emilio: Nice to not have the same stylesheet twice but I don't obj to argument
<dael> TabAtkins: Does it apply to same text not twice?
<dael> emilio: As of right now our system assumes same sheet doesn't appear twice. It uses stylesheet object at rest. constructable is only thing that can break this
<dael> TabAtkins: Got a balance of could not puttin in restriction have its . own impl work. Still concerned that this would result in confusing errors for authors. Do you think impl work to handle same stylehseet multi is large enough to add the restriction?
<dael> emilio: It would be fair amount. I don't know bar to justify adding this restriction. I would like other engines to weight in. I know blink and wk store stylesheet locals in stylesheet. That means same rule can allow 2 order in list of declarations. Order is not specific to stylesheet. So I suspect edge case in other engines.
<dael> emilio: I don't know enough to be able to justify
<dael> TabAtkins: I propose reject for now but have a note in spec that due to this violating existing assumptions about a stylesheet existing once there may be impl concerns and we don't know severity so may have to change in the future
<dael> emilio: Okay with that
<dael> astearns: Having somebody using constructable stylesheet api to put dup stylesheets in is edge case. Then person doing reverse and finding error is smaller so I think edge error case
<dael> TabAtkins: We're not. algo with reverse puts same object in two spots
<dael> astearns: Ah, so not a combo but doing a reverse on any stylesheet
<dael> TabAtkins: Yes.
<dael> astearns: Okay, I'm fine allowing with that note that we need impl experience
<dael> astearns: Other opinions?
<tantek> I'd like to q+ insert the meta issue of https://github.com/w3c/csswg-drafts/issues/3433 just after the discussion of this issue if we could, hoping we get a group consensus decision recorded to merge from WICG into the CSSWG CSSOM spec. Seems like there is agreement in the issue just need to get a decision recorded AFAIK
<dael> astearns: Prop: Close no change to normative text but add a note about needing impl experience
<TabAtkins> tantek, we already have that resolution iirc
<dael> astearns: Objections?
<dael> RESOLVED: Close no change to normative text but add a note about needing impl experience
<dael> astearns: tantek the decision is recorded and I believe there's an open PR
<dael> tantek: Sorry, wasn't obvious there's a consensus agreement from WG. I guess minor request to get it recorded in GH
<dael> astearns: I'll take an action to find the agreement

[nolanlawson]

Rather than disallowing duplicate styles, it would be nice to have some way to add styles without duplicating. A pretty common developer pattern would probably be:

function addStyleSheet(sheet) {
  if (!document.adoptedStyleSheets.includes(sheet)) {
    document.adoptedStyleSheets = [...document.adoptedStyleSheets, sheet]
  }
}

The problem is that, in Chrome's implementation at least, this can be pretty unperformant if called frequently, since it's effectively checking the entire array and copying it every time you add a sheet.

I have a benchmark of adding 1,000 duplicate sheets plus 1,000 unique sheets, and I get about 230ms on my machine.

So a developer may want to avoid the includes check, at the cost of including duplicate styles in the adoptedStyleSheets:

function addStyleSheet(sheet) {
  document.adoptedStyleSheets = [...document.adoptedStyleSheets, sheet]
}

Unfortunately, this technique takes even longer in this benchmark (~590ms) because as slow as includes() is, it's actually slower to set the adoptedStyleSheets array when there are duplicates that can be avoided.

So the ideal method (AFAICT) ends up being buffering:

const bufferedSheets = []
function addStyleSheet(sheet) {
  bufferedSheets.push(sheet)
  if (bufferedSheets.length === 1) {
    queueMicrotask(() => {
      const existing = [...document.adoptedStyleSheets]
      const existingSet = new Set(existing)
      const sheetsToAdd = bufferedSheets.filter(stylesheet => {
        if (existingSet.has(stylesheet)) {
          return false;
        }
        existingSet.add(stylesheet);
        return true;
      });
      document.adoptedStyleSheets = [...existing, ...sheetsToAdd]
      bufferedSheets.length = 0
    })
  }
}

In the benchmark, this cuts the time down to <1ms, but it's a lot of extra heavy lifting for the web author. Plus it has side effects, which is that between now and the microtask, it would be observable that the stylesheets haven't been added yet.

Something like adoptedStyleSheets.addIfNotExists(sheet) would maybe be a cleaner way to solve the duplicate problem, and perhaps would be more optimizable on the UA side of things?