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

CSS builds incorrectly #151

Closed
silvenon opened this issue Dec 18, 2014 · 13 comments
Closed

CSS builds incorrectly #151

silvenon opened this issue Dec 18, 2014 · 13 comments

Comments

@silvenon
Copy link
Contributor

I'm authoring my styles in Sass, then compiling to CSS before handing it over to assetgraph-builder. Then during the build I get a warning:

No input files specified, defaulting to file:///Users/matija/myproject/.tmp/*.html
 ✔ 0.001 secs: logEvents
 ✔ 0.001 secs: registerRequireJsConfig
 ✔ 0.004 secs: registerLabelsAsCustomProtocols
 ✔ 0.438 secs: loadAssets
 ✔ 0.705 secs: populate
 ✔ 0.437 secs: assumeRequireJsConfigHasBeenFound
 ⚠ WARN: .tmp/styles/main.css: Parse error in .tmp/styles/main.css(line 6107, column 48):
         Unexpected } (line 6107, char 48)
         Falling back to using the 1460 parsed CSS rules
 ✔ 1.939 secs: populate
 ✔ 0.003 secs: fixBaseAssetsOfUnresolvedOutgoingRelationsFromHtmlFragments
 ✔ 0.000 secs: assumeThatAllHtmlFragmentAssetsWithoutIncomingRelationsAreNotTemplates
 ✔ 0.062 secs: populate
 ✔ 0.002 secs: removeRelations
 ✔ 0.023 secs: addDataVersionAttributeToHtmlElement
 ✔ 0.001 secs: replaceDartWithJavaScript
 ✔ 0.000 secs: populate
 ✔ 0.000 secs: compileLessToCss
 ✔ 0.002 secs: removeRelations
 ✔ 0.001 secs: compileScssToCss
 ✔ 0.236 secs: populate
 ✔ 0.237 secs: compileJsxToJs

I let it run all the way, then when I look at the site a lot of CSS seems to be missing or broken, basically only the half of my home page renders correctly. I checked if cssmin screwed it up, but it didn't.

Could you help me debug this? I don't know at which step this warning occurred.

@gustavnikolaj
Copy link
Member

If you can provide a reduced test case where this happens we can take a look at it :-) See how much you can remove before it doesn't fail anymore.

The reason it only renders partially correct is, as the log says, that it falls back to only use that CSS which it could parse.

Arguably, it would be more correct to make it an error instead of a warning and thus stop the build instead of allowing it to complete.

@Munter
Copy link
Member

Munter commented Dec 19, 2014

My guess is that the error exists somewhere around line 6107 of the compiled CSS output. Give or take a few lines :)

I wonder if we can improve on that error message and actually show a few lines around what the parser couldn't parse, just to give a hint and maybe make it easier to figure out of the problem is in the parser. Just thinking that there are still quite a few known non-implementations in CSSOM

@silvenon
Copy link
Contributor Author

Yeah, I was looking at the compiled CSS at that location, but I couldn't find anything wrong (syntax highlighting was fine too).

I will debug further by taking out bits of CSS until I have more useful info 😃

@Munter
Copy link
Member

Munter commented Dec 19, 2014

The full main.css should be helpful in itself for us, unless there are secrets in there

@silvenon
Copy link
Contributor Author

Oh, I didn't know you would be kind enough to take a look :) https://gist.github.com/silvenon/ed2feed9c0e587842e14

Line 6108, column 48. Sorry about the nested output, libsass doesn't support the expanded output yet.

@Munter
Copy link
Member

Munter commented Dec 20, 2014

This construction seems to be the problem:

@media (min-width: 768px) {
  .section-home-technology {
    background-image: url("../images/technology.png");
    background-position: 65% 0%;
  }

  @media only screen and (-webkit-min-device-pixel-ratio: 1.3), only screen and (min--moz-device-pixel-ratio: 1.3), only screen and (-o-min-device-pixel-ratio: 1.3/1), only screen and (min-resolution: 125dpi), only screen and (min-resolution: 1.3dppx) {
    .section-home-technology {
      background-image: url("../images/technology@2x.png");
      background-size: 2553px 893px;
    }
  }
}

My editor CSS plugin also complains about expecting the first media query block to end before the next one begins.

@Munter
Copy link
Member

Munter commented Dec 20, 2014

There is a similar block later in the file that also has nested media queries, which throws the same error: https://gist.github.com/silvenon/ed2feed9c0e587842e14#file-thinkapps-css-L6195-L6205

@Munter
Copy link
Member

Munter commented Dec 20, 2014

This is the corresponding issue in CSSOM: NV/CSSOM#1

@silvenon
Copy link
Contributor Author

Thanks for identifying the problem! I didn't even know that nested media queries are valid CSS… Ruby Sass combines them instead of nesting them. Here's the corresponding libsass issue.

Though I remember trying it out with Ruby Sass too, I think I also got a parsing error. I will try later when I get access to my computer.

@silvenon
Copy link
Contributor Author

I tried with Ruby Sass and everything compiled successfully and correctly, so no problem there. Can't wait for the libsass issue to get resolved.

@Munter
Copy link
Member

Munter commented Dec 21, 2014

Sounds like nested media queries are allowed though, so I would still consider this a bug. Need to figure out how to patch CSSOM. The problem is we're running a non-compatible fork of it atm :(

@Munter
Copy link
Member

Munter commented Oct 23, 2015

We're talking about switching from CSSOM to Postcss in assetgraph/assetgraph#195

This might fix this issue

@papandreou
Copy link
Member

The switch to postcss happened a while ago. Please reopen if this still happens.

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

No branches or pull requests

4 participants