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

CSS @import corrupts scoped CSS #3254

Closed
p3k opened this Issue Jul 3, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@p3k

p3k commented Jul 3, 2018

Description:

Assume the following example:

const css = `
  body { background-color: red; }
  .foo { color: green; }
`;

const template = `
  <div class='foo'>Hello, World!</div>
`;

const App = Ractive.extend({
  css,
  template
});

Ractive({
  components: { App },
  target: '#app',
  template: '<App/>'
});

This renders a green text on a default background and correctly scopes the styles, no issue here:

/* Ractive.js component styles */

/* {6dc0437c-13f6-1d65-b647-481773bded6a} */

  body[data-ractive-css~="{6dc0437c-13f6-1d65-b647-481773bded6a}"], [data-ractive-css~="{6dc0437c-13f6-1d65-b647-481773bded6a}"] body { background-color: red; }
  .foo[data-ractive-css~="{6dc0437c-13f6-1d65-b647-481773bded6a}"], [data-ractive-css~="{6dc0437c-13f6-1d65-b647-481773bded6a}"] .foo { color: green; }

However, when I add a CSS @import directive, e.g. for loading a font, the rendered CSS for body suddenly becomes unscoped and thus, the background appears red:

const css = `
  @import url('https://fonts.googleapis.com/css?family=Shrikhand');
  body { background-color: red; }
  .foo { color: green; }
`;

//

In fact, the rendered CSS does not have a scoped body, anymore:

/* Ractive.js component styles */

/* {c7d35506-0525-63f6-f618-e16cafaa8ccb} */

  @import url('https://fonts.googleapis.com/css?family=Shrikhand');
  body { background-color: red; }
  .foo[data-ractive-css~="{c7d35506-0525-63f6-f618-e16cafaa8ccb}"], [data-ractive-css~="{c7d35506-0525-63f6-f618-e16cafaa8ccb}"] .foo { color: green; } 

Versions affected:

  • 0.9.14 (my environment)
  • 1.0.0-edge (playground)

Platforms affected:

  • Chromium 66.0.3359.181 on Ubuntu 16.04.4 LTS

Reproduction:

Playground

@MartinKolarik MartinKolarik self-assigned this Jul 3, 2018

@p3k

This comment has been minimized.

Show comment
Hide comment
@p3k

p3k Jul 3, 2018

Btw. just noticed that something similar happens when there is a syntax error instead of @import, e.g.

const css = `
  // CSS comments need to be enclosed in /* */
  .foo { color: green; }
`;

(In fact, I am currently trying to compile with LESS the CSS rendered by Ractive, and LESS allows simple line comments.)

Here is the rendered CSS output which looks pretty, um, interesting to say the least:

/* Ractive.js component styles */

/* {e25f538e-07bc-cd9b-7825-014cb8d6d82f} */

  //  CSS  comments  need  to  be  enclosed  in 
   .foo[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"], //  CSS  comments  need  to  be  enclosed  in 
   [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] .foo, //  CSS  comments  need  to  be  enclosed  in[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] 
   .foo, //  CSS  comments  need  to  be  enclosed  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] in 
   .foo, //  CSS  comments  need  to  be  enclosed[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  in 
   .foo, //  CSS  comments  need  to  be  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] enclosed  in 
   .foo, //  CSS  comments  need  to  be[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  enclosed  in 
   .foo, //  CSS  comments  need  to  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] be  enclosed  in 
   .foo, //  CSS  comments  need  to[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  be  enclosed  in 
   .foo, //  CSS  comments  need  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] to  be  enclosed  in 
   .foo, //  CSS  comments  need[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  to  be  enclosed  in 
   .foo, //  CSS  comments  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] need  to  be  enclosed  in 
   .foo, //  CSS  comments[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  need  to  be  enclosed  in 
   .foo, //  CSS  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] comments  need  to  be  enclosed  in 
   .foo, //  CSS[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  comments  need  to  be  enclosed  in 
   .foo, //  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] CSS  comments  need  to  be  enclosed  in 
   .foo, //[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  CSS  comments  need  to  be  enclosed  in 
   .foo, [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] //  CSS  comments  need  to  be  enclosed  in 
   .foo { color: green; }

p3k commented Jul 3, 2018

Btw. just noticed that something similar happens when there is a syntax error instead of @import, e.g.

const css = `
  // CSS comments need to be enclosed in /* */
  .foo { color: green; }
`;

(In fact, I am currently trying to compile with LESS the CSS rendered by Ractive, and LESS allows simple line comments.)

Here is the rendered CSS output which looks pretty, um, interesting to say the least:

/* Ractive.js component styles */

/* {e25f538e-07bc-cd9b-7825-014cb8d6d82f} */

  //  CSS  comments  need  to  be  enclosed  in 
   .foo[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"], //  CSS  comments  need  to  be  enclosed  in 
   [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] .foo, //  CSS  comments  need  to  be  enclosed  in[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] 
   .foo, //  CSS  comments  need  to  be  enclosed  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] in 
   .foo, //  CSS  comments  need  to  be  enclosed[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  in 
   .foo, //  CSS  comments  need  to  be  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] enclosed  in 
   .foo, //  CSS  comments  need  to  be[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  enclosed  in 
   .foo, //  CSS  comments  need  to  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] be  enclosed  in 
   .foo, //  CSS  comments  need  to[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  be  enclosed  in 
   .foo, //  CSS  comments  need  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] to  be  enclosed  in 
   .foo, //  CSS  comments  need[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  to  be  enclosed  in 
   .foo, //  CSS  comments  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] need  to  be  enclosed  in 
   .foo, //  CSS  comments[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  need  to  be  enclosed  in 
   .foo, //  CSS  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] comments  need  to  be  enclosed  in 
   .foo, //  CSS[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  comments  need  to  be  enclosed  in 
   .foo, //  [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] CSS  comments  need  to  be  enclosed  in 
   .foo, //[data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"]  CSS  comments  need  to  be  enclosed  in 
   .foo, [data-ractive-css~="{e25f538e-07bc-cd9b-7825-014cb8d6d82f}"] //  CSS  comments  need  to  be  enclosed  in 
   .foo { color: green; }
@MartinKolarik

This comment has been minimized.

Show comment
Hide comment
@MartinKolarik

MartinKolarik Jul 3, 2018

Member

The css "parser" is just a bunch of regular expressions so this is not surprising. I think we should aim to make it reliable with valid CSS but that's it, handling syntax errors is a bit too much. If you want to use less, you should definitely compile before applying scoping.

Regarding @import, it's a problem with this regex and I'll fix it later today or tomorrow unless anyone does it sooner.

Member

MartinKolarik commented Jul 3, 2018

The css "parser" is just a bunch of regular expressions so this is not surprising. I think we should aim to make it reliable with valid CSS but that's it, handling syntax errors is a bit too much. If you want to use less, you should definitely compile before applying scoping.

Regarding @import, it's a problem with this regex and I'll fix it later today or tomorrow unless anyone does it sooner.

@p3k

This comment has been minimized.

Show comment
Hide comment
@p3k

p3k Jul 3, 2018

Currently, I don’t mind if the CSS would simply be copied as is – just because I am trying postprocessing with LESS and am fully aware of the syntax breaches that could arise.

I agree compiling before scoping would be the better approach. However, my first attempts were futile because I could not solve asynchronous issues with the LESS renderer (see less/less.js#3238).

You would not have some good idea or further suggestion for me I still could try by any chance?

p3k commented Jul 3, 2018

Currently, I don’t mind if the CSS would simply be copied as is – just because I am trying postprocessing with LESS and am fully aware of the syntax breaches that could arise.

I agree compiling before scoping would be the better approach. However, my first attempts were futile because I could not solve asynchronous issues with the LESS renderer (see less/less.js#3238).

You would not have some good idea or further suggestion for me I still could try by any chance?

@MartinKolarik

This comment has been minimized.

Show comment
Hide comment
@MartinKolarik

MartinKolarik Jul 3, 2018

Member

You would not have some good idea or further suggestion for me I still could try by any chance?

I tried to do this some time back but don't remember if I was successful. Eventually I just moved to separate less files because the components files were getting too long and hard to navigate. Now I simply wrap all component styles with .c-component-name which is super easy in less and has worked fine so far.

Member

MartinKolarik commented Jul 3, 2018

You would not have some good idea or further suggestion for me I still could try by any chance?

I tried to do this some time back but don't remember if I was successful. Eventually I just moved to separate less files because the components files were getting too long and hard to navigate. Now I simply wrap all component styles with .c-component-name which is super easy in less and has worked fine so far.

@p3k

This comment has been minimized.

Show comment
Hide comment
@p3k

p3k Jul 3, 2018

Thanks for sharing the idea with the component class names, sounds good!

p3k commented Jul 3, 2018

Thanks for sharing the idea with the component class names, sounds good!

MartinKolarik added a commit that referenced this issue Jul 5, 2018

Merge pull request #3255 from ractivejs/gh-3254
Fix @import css transformation (fixes #3254)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment