Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Could we allow creating a no-imports stylesheet synchronously? #38

Closed
domenic opened this issue Sep 12, 2018 · 18 comments
Closed

Could we allow creating a no-imports stylesheet synchronously? #38

domenic opened this issue Sep 12, 2018 · 18 comments
Labels
needs resolution Needs consensus/resolution before shipping

Comments

@domenic
Copy link
Contributor

domenic commented Sep 12, 2018

It makes sense that createCSSStyleSheet has to be async to wait for @import rules.

Would it be possible to allow the creation of a no-import stylesheet synchronously, with a new factory method?

Otherwise I fear we are going to quickly see developers writing code like this:

function createCSSStyleSheetSync(text, options) {
  const ss = document.createEmptyCSSStyleSheet(options);
  for (const rule of text.split("}")) {
    ss.insertRule(rule + "}");
  }
  return ss;
}

which I'm sure is very buggy, but will mostly work.

@rakina
Copy link
Member

rakina commented Sep 12, 2018

@tabatkins mentioned in #25 (comment) that large parses might be a problem.

@domenic
Copy link
Contributor Author

domenic commented Sep 12, 2018

I just think there are so many hacks around it, that are easily encapsulated into libraries, that attempting to prevent authors from doing so will be futile, and just lead to them doing more buggy or expensive things instead. (E.g. my above, or inserting a <style> element and then copying over all the rules into a constructed stylesheet to get around the prohibition on using <style>-element-derived CSSStyleSheet instances in adoptedStyleSheets, or...)

@rakina
Copy link
Member

rakina commented Sep 13, 2018

Hmm, that's a valid concern, but if we do make a synchronous constructor, wouldn't that just take the same time as await document.createCSSStyleSheet(text)? Then it might be more natural for web devs to use that instead of the hacky thing. On this specific issue I defer to Tab who suggested the change.

On the <style> element copying thing, I am reopening #34. I think whether we allow non-explicitly constructed stylesheets to be used elsewhere or not will affect WICG/webcomponents#468. If we do not allow usage that means that proposal will be tied to constructable stylesheets too. I think it is possible to make this work.

@rakina rakina added the needs resolution Needs consensus/resolution before shipping label Sep 13, 2018
@domenic
Copy link
Contributor Author

domenic commented Sep 13, 2018

The issue with await is that it requires you then make all calling functions async. See http://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/ . So if it's not necessary then that'd be ideal.

In particular I think if we decide in #34 to allow use of <style> element CSSStyleSheets in either adoptedStyleSheets or in custom element default styles, then we should definitely also allow synchronous construction from a string. Otherwise a lot of authors will never bother using constructible style sheets, because it's just way more convenient to stuff their text in the textContent of a <style> element and use that CSSStyleSheet.

@calebdwilliams
Copy link

calebdwilliams commented Sep 13, 2018

I still think adding a synchronously-created stylesheet is a good idea, but mostly if there’s some way to efficiently edit that stylesheet after its initial creation. This is somewhat similar in developer ergonomics as something like document.createElement if you want to do anything interesting. That would definitely complicate this proposal, but there’s no need for it to be a version one thing.

@calebdwilliams
Copy link

As an outsider this feels like you’re trying to stuff a square peg into a round hole. I’m starting to wonder if a new type of InertStyleSheet That inherits from CSSStyleSheet would be called for. You could use a link or style tag to create this or do it with JS.

Just my two cents. Take it for whatever you will.

@tabatkins
Copy link
Contributor

Otherwise a lot of authors will never bother using constructible style sheets, because it's just way more convenient to stuff their text in the textContent of a style element and use that CSSStyleSheet.

That only works if the style element is connected (and thus applies to the document...), and then you have to go digging thru document.stylesheets to find it. It's risky and not convenient.

But creating an empty sheet and filling it rule-by-rule does indeed work, so that's annoying. :/

I'm not opposed to having a synchronous way to create a content-ful stylesheet. I am opposed to the sync way being easier and more natural to type than the async way. Perhaps a long-named method would work. ^_^ document.createEmptyCSSStyleSheet.syncBlockingParseStylesheet(text)

@rakina
Copy link
Member

rakina commented Sep 17, 2018

If we do go with the annoying-sync method, do we still not allow/ignore imports in that case?

(Previous suggestion to not allow @imports in this repo was opposed, though that's because there is no other way to actually make constructed ones with @import)

@rakina
Copy link
Member

rakina commented Sep 20, 2018

If we do go with the annoying-sync method, do we still not allow/ignore imports in that case?

I guess it's better to allow it, it should be the author's responsibility to not place imports where they don't want it to block for too long. But I wonder what should happen if any of the imports fail. For the async version, we will reject the promise. Do we return null in this case if an import fail?

@tabatkins
Copy link
Contributor

tabatkins commented Sep 20, 2018

So, thinking on this more, even the "rules can be parsed synchronously" doesn't really make things any better; as long as you're not using an @import, @charset, or similar, you can trivially just wrap the entire stylesheet text in an always-true @media rule and go to town.

So! I think I'll change strategy for the CSS Parser API. Relevant to this repository, we should stop worrying about synchronous parsing as something particularly bad. So my proposal:

  • document.createCSSStyleSheet() - async parse, returns a Promise<CSSStyleSheet>, as today
  • document.createCSSStyleSheetSync() - sync parse, returns a CSSStyleSheet

So the sync is slightly longer, which satisfies my "sync shouldn't be eaiser to type than async" requirement, but isn't too bad.


If we do go with the annoying-sync method, do we still not allow/ignore imports in that case?

So, we can't sync-process @import; that would be equivalent to a sync-XHR, which is a big no-no. So we're got two choices:

  1. @import is allowed, and does nothing in particular; it's just retained in the stylesheet. When you attach the stylesheet, the @import will activate.

  2. @import is not allowed and causes a syntax error.

The first one means we have to do something special for the CSSImportRule; it needs to null its .styleSheet property at first, but later asynchronously set it to something non-null. That's not great.

So, I think we have to go with the second - if you do a sync parse, you don't get to use @import.


If we add the sync version, we can eliminate createEmptyCSSStyleSheet(), as that's just a sync-create with an empty string.

@tabatkins
Copy link
Contributor

Alternately, we allow them, but auto-fail the import, so they act the same as if you'd pointed them to a 404.

@lilles
Copy link

lilles commented Oct 3, 2018

"@import is not allowed and causes a syntax error"

Syntax error here means throwing an exception, or just dropping the rule?

@tabatkins
Copy link
Contributor

Sorry, throwing a SyntaxError.

@rakina
Copy link
Member

rakina commented Oct 3, 2018

Sorry, throwing a SyntaxError.

In my CL for this, @lilles was saying that the CSS Syntax spec never generates a syntax error, so it might be a bit strange to introduce that. What do you think? (Rune, if I get this wrong, please correct me)

@lilles
Copy link

lilles commented Oct 4, 2018

I was talking about the "parse a stylesheet" entry point which currently never generates a syntax error.

I'm just saying there's a third option of dropping @import rules.

@tabatkins
Copy link
Contributor

Yeah, the grammar for CSS stylesheets is the regex /.*/; there's no such thing as an invalid sheet, so any byte sequence whatsoever will result in something (possibly just an empty stylesheet, but still). But that doesn't mean a specialized parsing API can't reject things that violate its assumptions.

For a similar example, using the string-OM, you can set a property to any value whatsoever; unknown/invalid values will just be silently ignored, just like they are when written into stylesheets. But Typed OM purposely breaks from this and throws a TypeError if it doesn't understand the value you're setting. This provides better feedback to the author, and JS code can respond to errors better than CSS can, so we can be more explicit with rejection.

Thus, I think we should match this sort of behavior with newer JS parsing APIs - if we're not allowing some construct, we should throw an error to let the author know immediately, rather than just producing a stylesheet that acts differently than they thought it would.

(I do not think we should fail the parsing just because we encounter something we don't recognize, like Typed OM does; the use-case here is parsing entire stylesheets, and stylesheets are intended to be written with forward-compatible error-recovery in mind. But @import is something we're explicitly disallowing, so it feels different to me.)

@lilles
Copy link

lilles commented Oct 5, 2018

Makes sense.

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 11, 2018
A new factory function, createCSSStyleSheetSync will allow synchronous
creation of CSSStyleSheet objects that have text but do not have
@import rules. If the sheet text turn out to have @import rules, an
exception will be thrown.

Draft spec at: https://wicg.github.io/construct-stylesheets/
Discussion: WICG/construct-stylesheets#38 (comment)

Bug: 807560
Change-Id: I9f8ef4151f616ecc343a12f3ddd0e5a31e1be363
Reviewed-on: https://chromium-review.googlesource.com/c/1237803
Commit-Queue: Rakina Zata Amni <[email protected]>
Reviewed-by: Rune Lillesveen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#598731}
@rakina
Copy link
Member

rakina commented Oct 17, 2018

Spec updated at #39.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs resolution Needs consensus/resolution before shipping
Projects
None yet
Development

No branches or pull requests

5 participants