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

Sheet's rules after replace() fails to load @import rule #122

Closed
nordzilla opened this issue Feb 13, 2020 · 4 comments
Closed

Sheet's rules after replace() fails to load @import rule #122

nordzilla opened this issue Feb 13, 2020 · 4 comments

Comments

@nordzilla
Copy link
Contributor

In PR #112 I modified the spec description for replace() to only return a promise instead of throwing, as noted in the TAG API guidelines (here). I believe that I accidentally changed the semantics in the case of a failed @import rule.


Intended Behavior

I believe that the intended behavior should be that if an @import rule fails to load, the promise is rejected with a network error, but the sheet's rules are still cleared.

Actual Behavior

The new text that I wrote in #112 would suggest that if the @import rule fails to load, the sheet still contains the rules it had previously.


I want to double check that the intended behavior is correct. If so, I will create a PR to amend the mistake that I made.

Note that step 3 in the old text clears the sheets rules. I believe that I inadvertently removed this action in what is now step 4 of the new text.

Here are the rules before the change:

image

Here are the rules after the change:

image


cc @tabatkins @rakina

@rakina
Copy link
Member

rakina commented Feb 19, 2020

Yeah I think we should not replace the rules in that case. The behavior will be consistent with replaceSync, where there will be no change if at any point there's an error https://wicg.github.io/construct-stylesheets/#dom-cssstylesheet-replacesync

@nordzilla
Copy link
Contributor Author

Closing due to the decision to deprecate replace():
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/RKG8oxp22RY/fdFnG1rGCgAJ

@mfreed7
Copy link

mfreed7 commented Feb 27, 2020

We’re not deprecating replace() entirely, just the case where the replacement sheet contains an @import rule. Calls to replace() without @import will continue working as is.

@nordzilla
Copy link
Contributor Author

Thank you for this update. But I am going to leave this issue as closed, because it's more-or-less a subset of #123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants