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

Allow user to set which at_rules can contain properties #111

Closed
adamakhtar opened this issue Aug 1, 2014 · 4 comments
Closed

Allow user to set which at_rules can contain properties #111

adamakhtar opened this issue Aug 1, 2014 · 4 comments
Milestone

Comments

@adamakhtar
Copy link

First thanks for making this gem! Its coming in really handy for my application.

I've noticed that whilst Sanitize allows you to customize the white list of at_rules, its hardcoded to only allow @page and @font-face to contain properties.

Im using Prince (html to pdf kit) and it allows you define custom at_rules to help with header creation and page numbering.

Unfortunately Santize will strip their contents i.e.

# Custom sanitizer -> add @top-center as custom at_rule
custom = Sanitize::Config.merge( 
    Sanitize::Config::RELAXED,
    css: { at_rules: Sanitize::Config::RELAXED[:css][:at_rules] + %w{top-center} } )

css = <<-DOC
@page {
    @top-center { 
      vertical-align: top;
    }
  }
DOC

Sanitize::CSS.stylesheet(css, custom) # => "@page {    @top-center   }"

Resetting AT_RULES_WITH_PROPERTIES to include the custom at_rule fixes this.

Sanitize::CSS::AT_RULES_WITH_PROPERTIES = Set.new(%w[font-face page top-center])

What are your thoughts on allowing this to be configurable? If I have time I can provide a fix.

@adamakhtar adamakhtar changed the title Allow user to set which at_rules can define properties Allow user to set which at_rules can contain properties Aug 1, 2014
@rgrove
Copy link
Owner

rgrove commented Aug 2, 2014

Agreed, we should make the list of at-rules that allow properties configurable. Please feel free to send a PR! If you don't get around to it, I'll see if I can get this in for the next feature release.

@adamakhtar
Copy link
Author

Actually the result in my example above wasnt correct.

css = <<-DOC
@page {
    @top-center { 
      vertical-align: top;
    }
  }
DOC

Sanitize::CSS.stylesheet(css, custom) # -----> "@page \n"

Sanitize doesnt seem to allow the nesting of @top-center despite hardcoding it in AT_RULES_WITH_PROPERTIES.

It seems to handle nested @media better. But not perfect (notice .note contents are stripped )

css = <<-DOC 
@media print {
  #navigation { 
    display: none 
  }
  @media (max-width: 12cm)
    .note { float: none }
  }
}
DOC

Sanitize::CSS.stylesheet(css, Sanitize::Config::HIGHFIVE) # ----> "@media print {\n  #navigation { \n    display: none \n  }\n  @media (max-width: 12cm)\n    .note { }\n  }\n"

# .note{} is stripped.

Perhaps my assumption of how Sanitize works is incorrect. Can you explain why the above works better for @media and not at all for my custom defined @top-center?

Also just to make sure I'm not mistaken what is the difference between a style and a property i.e. AT_RULES_WITH_PROPERTIES VS AT_RULES_WITH_STYLES

@rgrove
Copy link
Owner

rgrove commented Aug 2, 2014

Sanitize doesnt seem to allow the nesting of @top-center despite hardcoding it in AT_RULES_WITH_PROPERTIES.

This works fine for me:

custom = Sanitize::Config.merge(
    Sanitize::Config::RELAXED,
    css: { at_rules: Sanitize::Config::RELAXED[:css][:at_rules] + %w{top-center} } )

css = <<-DOC
@page {
    @top-center {
      vertical-align: top;
    }
  }
DOC

Sanitize::CSS::AT_RULES_WITH_PROPERTIES = Set.new(%w[font-face page top-center])

Sanitize::CSS.stylesheet(css, custom)

The output is:

@page {
    @top-center {
        vertical-align: top;
    }
}

In your @media example, you're missing an opening { after the nested @media rule, so the CSS is broken. Once you add the missing {, it'll work fine.

@adamakhtar
Copy link
Author

Ahh sorry that was my mistake. I just realised I was doing

Sanitize::CSS::AT_RULES_WITH_PROPERTIES = Set.new(%w[top-center])

and not +=

works fine

@rgrove rgrove added this to the 3.1.0 milestone Aug 6, 2014
@rgrove rgrove modified the milestones: 3.2.0, 3.1.0 Apr 18, 2015
@rgrove rgrove closed this as completed in 65e95d6 Apr 20, 2015
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

2 participants