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

combine_css should scan for syntax errors to avoid combining broken files #253

Closed
GoogleCodeExporter opened this issue Apr 6, 2015 · 9 comments

Comments

@GoogleCodeExporter
Copy link

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

What steps will reproduce the problem?
1.  Surf to that site
2.  Look scroll down below the main photo
3.  Look at the 55x55 photos

What is the expected output? What do you see instead?

Expected: grid of photos
Found: 3 diagonals of photos

What version of the product are you using (please check X-Mod-Pagespeed
header)?

    0.9.16.9-576

Please provide any additional information below, especially a URL or an
HTML file that exhibits the problem.

http://www.polywood-furniture.com/polywood-adirondack-chair-1.html

Original issue reported on code.google.com by jmara...@google.com on 28 Mar 2011 at 4:36

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

Quite likely due to the missing } in 
http://www.polywood-furniture.com/skin/frontend/default/blank_seo/css/SpryTabbed
Panels.css which gets combined with 
http://www.polywood-furniture.com/skin/frontend/default/blank_seo/css/highslide.
css (the latter of which is responsible for hiding these labels)

Original comment by morlov...@google.com on 28 Mar 2011 at 4:46

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

Nice catch, Maksim.  We should consider a syntax check and avoid combining CSS 
files that have syntax errors.

Original comment by jmara...@google.com on 28 Mar 2011 at 4:52

  • Changed state: Accepted
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

There was an earlier issue with Spry menubars: 
http://code.google.com/p/modpagespeed/issues/detail?id=22 which we thought we 
had probably fixed.  But maybe this is really a manifestation of the same issue.

Original comment by jmara...@google.com on 28 Mar 2011 at 4:55

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

I'm very impressed guys!! That did the trick. Seems to be working okay now. 
Going to do some more testing, but so far everything looks good.

Original comment by polywood...@gmail.com on 28 Mar 2011 at 5:28

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

Summary was: combine_css breaks a Magento site

The problem was that if you combine files A & B, and there is a syntax error in 
A, it may break functionality in file B.  When they are separate 'link' tags 
this is not an issue.  This is a user-error but it could be avoided if 
mod_pagespeed scanned files it was combining and disqualified files with syntax 
errors.

Original comment by jmara...@google.com on 6 May 2011 at 5:09

  • Changed title: combine_css should scan for syntax errors to avoid combining broken files
  • Changed state: New
  • Added labels: Type-Enhancement
  • Removed labels: Type-Defect
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

There is one issue which i am facing in css combine. I have two css call in my 
web page, which gets combined into a ".html" file and some time to ".js" file 
extension.  Also need to mention that, this is not happening every time. But 
because of this my page gets distorted.

ModPagespeedEnableFilters combine_css,extend_cache
ModPagespeedEnableFilters rewrite_css,rewrite_javascript

ModPagespeedEnableFilters rewrite_images
ModPagespeedEnableFilters remove_comments
ModPagespeedEnableFilters extend_cache
ModPagespeedEnableFilters collapse_whitespace

ModPagespeedEnableFilters move_css_to_head

~Thanks in advance 

Original comment by asif.iqu...@gmail.com on 4 Apr 2012 at 10:47

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

This sounds unrelated to the original bug here. Created new bug 412 for your 
report.

Original comment by sligocki@google.com on 4 Apr 2012 at 6:50

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

CombineCss now parses CSS files and refuses to combine those with parsing 
errors (as of r1512). As stated in the revision notes, this is a bit 
overzealous, so we reject a lot of valid CSS that we simply cannot parse. More 
work to come to refine this and make it more lenient.

Original comment by sligocki@google.com on 5 Apr 2012 at 5:14

  • Changed state: Fixed
  • Added labels: release-note
@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 22 May 2012 at 7:56

  • Added labels: Milestone-v22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.