Generate Source Mapping Error when a property value contains comma about closure-stylesheets HOT 19 CLOSED

Excellent bug report. I will take a look.

from closure-stylesheets.

This bug is causing problems for me too. For example:

@font-face {                                                                    
  font-family: 'Roboto';                                                        
  font-style: normal;                                                           
  font-weight: 400;                                                             
  src: local('Roboto'), local('Roboto-Regular'),                                
       url('/roboto.woff2') format('woff2'),                                    
       url('/roboto.woff') format('woff');                                      
}                                                                               

body {                                                                          
  font-family: 'Roboto', sans-serif;                                            
  color: #555;                                                                  
}

When run with the 1.3.0 compiler:

bazel-out/local-fastbuild/bin/external/io_bazel_rules_closure/third_party/java/csscomp/ClosureCommandLineCompiler --output-file bazel-out/local-fastbuild/bin/lathamlabs/ll_css_bin.css --output-source-map bazel-out/local-fastbuild/bin/lathamlabs/ll_css_bin.css.map --input-orientation LTR --output-orientation NOCHANGE --output-renaming-map bazel-out/local-fastbuild/bin/lathamlabs/ll_css_bin.css.js --output-renaming-map-format CLOSURE_COMPILED_SPLIT_HYPHENS --rename CLOSURE lathamlabs/ll.css

Results in the following compiler internal error:

Compiler internal error: null
java.lang.NullPointerException
        at com.google.debugging.sourcemap.SourceMapGeneratorV3$MappingTraversal.isOverlapped(SourceMapGeneratorV3.java:737)
        at com.google.debugging.sourcemap.SourceMapGeneratorV3$MappingTraversal.traverse(SourceMapGeneratorV3.java:693)
        at com.google.debugging.sourcemap.SourceMapGeneratorV3.prepMappings(SourceMapGeneratorV3.java:577)
        at com.google.debugging.sourcemap.SourceMapGeneratorV3.appendTo(SourceMapGeneratorV3.java:363)
        at com.google.common.css.compiler.passes.DefaultGssSourceMapGenerator.appendOutputTo(DefaultGssSourceMapGenerator.java:120)
        at com.google.common.css.compiler.commandline.DefaultCommandLineCompiler.execute(DefaultCommandLineCompiler.java:185)
        at com.google.common.css.compiler.commandline.ClosureCommandLineCompiler.executeJob(ClosureCommandLineCompiler.java:342)
        at com.google.common.css.compiler.commandline.ClosureCommandLineCompiler.main(ClosureCommandLineCompiler.java:408)

After removing the commas (eg: font-family: 'Roboto'; vs font-family: 'Roboto', sans-serif;) the compiler works correctly.

from closure-stylesheets.

For reference, here are links to source code

• SourceMapGeneratorV3.java:737
• SourceMapGeneratorV3.java:693
• SourceMapGeneratorV3.java:577
• SourceMapGeneratorV3.java:363
• DefaultGssSourceMapGenerator.java:120

And the prior one

• DefaultGssSourceMapGenerator.java:157

from closure-stylesheets.

@iflan Do you have any time to look more at this bug?

from closure-stylesheets.

I will look at this tomorrow. Sorry for the delay!

from closure-stylesheets.

Thanks.

from closure-stylesheets.

As expected, it looks like we're not generating the right end position sometimes:

I'm still looking at this. When I find a solution, I'll post a quick patch, but make a better patch, test, etc. and submit it internally which may take a few more days.

Thanks for your patience.

from closure-stylesheets.

Thanks Ian. Once the final patch is merged and pushed to the open source repo could a new release be cut?

from closure-stylesheets.

It seems to be taking the location of the first font family as the location of a bunch of font families.

from closure-stylesheets.

That's a good guess, but I don't think that's it. My debugging has lead me to a problem in the DefaultVisitController. For some reason, UniformVisitor is not always having the leave method called for each enter for CssCompositeValueNodes.

from closure-stylesheets.

OK, I'm pretty sure that the problem is that none of the methods in CompactPrinter that come from UniformVisitor actually call their super(). I'm surprised that nothing else broke, but that could explain a lot of our problems with sourcemaps.

That's a big change, so I'll tackle it tomorrow.

from closure-stylesheets.

After investigation, I have a patch (below) that avoids the NPE as originally described. It's not the right solution because further subclasses of CompactPrinter will end up having the same problem. They can't call their super methods because that would trigger whatever behavior they are trying to override.

The solution is (I think) to:

• split CompactPrinter's CssCompilerPass and CodePrinter logic into separate classes;
• remove CodePrinter from the *Printer class hierarchy;
• change CodePrinter to a pure visitor and call it something like SourceMapVisitor;
• create a delegating visitor that calls the enter* methods of several other visitors in order and the leave* methods in the opposite order;
• change the various *Printer classes to use the delegating visitor to generate sourcemaps.

I'm going to try to start on this change today, but in the meantime this patch should help:

0001-Fix-CompactPrinter-to-call-super-methods.patch.txt

Note that this patch won't be part of the next release. Instead, it will have the correct solution as outlined above. (And, yes, we will make the new release as soon as the fix is pushed out to GitHub.)

from closure-stylesheets.

Awesome. Thanks Ian.

from closure-stylesheets.

I've got a change internally that works. However, it's pretty big and complicated. My main reviewer is on vacation, so I've asked @mikesamuel to take a look. Unfortunately, this may take a few more days to get in and pushed out.

from closure-stylesheets.

Thanks. No worries about a delay. As long as a fix is slowly moving through the pipeline that meets my needs.

from closure-stylesheets.

Just an update on this: I'm currently getting this submitted internally and it should be done tonight. I'm not going to have time to push out a new release tonight, so I'm expecting to push it tomorrow morning. (The Maven release may be delayed until tomorrow night, though.)

from closure-stylesheets.

Awesome. Thanks for providing the timeline.

from closure-stylesheets.

A new release, v1.4.0, has been pushed. It's available on GitHub and Maven Central.

from closure-stylesheets.

Thanks Ian!

from closure-stylesheets.