Skip to content

feat: Add keep_at_rules option #485

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion css-inline/src/html/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@ pub(crate) fn hash_class_name(name: &[u8]) -> u64 {
}

/// A collection of HTML attributes.
#[derive(Debug)]
// TODO there must be a way for not having to instantiate Attributes class, see serializer.rs
#[derive(Debug, Default)]
pub(crate) struct Attributes {
pub(crate) attributes: Vec<html5ever::Attribute>,
/// The 'class' attribute value is separated for performance reasons.
Expand Down
6 changes: 5 additions & 1 deletion css-inline/src/html/document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,9 +288,11 @@ impl Document {
styles: DocumentStyleMap<'_>,
keep_style_tags: bool,
keep_link_tags: bool,
keep_at_rules: bool,
at_rules: Option<String>,
mode: InliningMode,
) -> Result<(), InlineError> {
serialize_to(self, writer, styles, keep_style_tags, keep_link_tags, mode)
serialize_to(self, writer, styles, keep_style_tags, keep_link_tags, keep_at_rules, at_rules, mode)
}

/// Filter this node iterator to elements matching the given selectors.
Expand Down Expand Up @@ -341,6 +343,8 @@ mod tests {
IndexMap::default(),
false,
false,
false,
None,
InliningMode::Document,
)
.expect("Failed to serialize");
Expand Down
41 changes: 41 additions & 0 deletions css-inline/src/html/serializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,17 @@
styles: DocumentStyleMap<'_>,
keep_style_tags: bool,
keep_link_tags: bool,
keep_at_rules: bool,
at_rules: Option<String>,
mode: InliningMode,
) -> Result<(), InlineError> {
let sink = Sink::new(
document,
NodeId::document_id(),
keep_style_tags,
keep_link_tags,
keep_at_rules,
at_rules,
mode,
);
let mut ser = HtmlSerializer::new(writer, styles);
Expand All @@ -34,6 +38,9 @@
node: NodeId,
keep_style_tags: bool,
keep_link_tags: bool,
// TODO this is not used here but I might need it for simplifying serialization logic below
keep_at_rules: bool,
at_rules: Option<String>,
inlining_mode: InliningMode,
}

Expand All @@ -43,13 +50,17 @@
node: NodeId,
keep_style_tags: bool,
keep_link_tags: bool,
keep_at_rules: bool,
at_rules: Option<String>,
inlining_mode: InliningMode,
) -> Sink<'a> {
Sink {
document,
node,
keep_style_tags,
keep_link_tags,
keep_at_rules,
at_rules,
inlining_mode,
}
}
Expand All @@ -60,6 +71,8 @@
node,
self.keep_style_tags,
self.keep_link_tags,
self.keep_at_rules,
self.at_rules.clone(),
self.inlining_mode,
)
}
Expand Down Expand Up @@ -114,6 +127,22 @@

serializer.start_elem(&element.name, &element.attributes, style_node_id)?;

// TODO this part is the one that I don't like the most
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure about this one, even though it is a sound approach.

The first thought that I have is that the least surprising thing would be to keep @ rules in their original places, instead of pushing everything into a single node. This won't work for external stylesheets, so we need to store them somewhere anyway.

On the other hand, this approach is better for performance, and probably it would not be super surprising for the user anyway if we put everything under one <style> tag.

So, in the end, I am leaning towards this approach.

A few other notes:

  1. We can reduce the performance hit of checking each node name by having two versions of serialize - the first one will work as before this PR. The new one will check for head and once it is found, it will switch to the other version of serialize that won't check the tag name. At the top level we check if at_rules are present and then call one of these two versions - I think it should minimize the impact as head is usually in the very beginning of the nodes vector.
  2. Are there cases when head is not present? Maybe in HTML fragments? see inline_fragment

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, if the user chose to keep style tags - I think we can only extract @ rule from link, as the old style tags will be around anyway

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something I had no time to look closer into yet

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the current benchmark results, the performance hit is negligible, so we can skip this. For fragments, likely we can't really do it, as there is no regular HTML structure generally. Maybe we can return an error when the user sets an option to keep at rules, but uses inline_fragment?

if element.name.local == local_name!("head") {
if let Some(at_rules) = &self.at_rules {
if !at_rules.is_empty() {
serializer.start_elem(
&QualName::new(None, ns!(html), local_name!("style")),
&Attributes::default(),
None,
)?;

Check warning on line 138 in css-inline/src/html/serializer.rs

View check run for this annotation

Codecov / codecov/patch

css-inline/src/html/serializer.rs#L138

Added line #L138 was not covered by tests
serializer.write_text(at_rules)?;
serializer
.end_elem(&QualName::new(None, ns!(html), local_name!("style")))?;
}

Check warning on line 142 in css-inline/src/html/serializer.rs

View check run for this annotation

Codecov / codecov/patch

css-inline/src/html/serializer.rs#L142

Added line #L142 was not covered by tests
}
}

self.serialize_children(serializer)?;

serializer.end_elem(&element.name)?;
Expand Down Expand Up @@ -575,6 +604,8 @@
IndexMap::default(),
true,
false,
false,
None,
InliningMode::Document,
)
.expect("Should not fail");
Expand All @@ -594,6 +625,8 @@
IndexMap::default(),
false,
false,
false,
None,
InliningMode::Document,
)
.expect("Should not fail");
Expand All @@ -613,6 +646,8 @@
IndexMap::default(),
false,
false,
false,
None,
InliningMode::Document,
)
.expect("Should not fail");
Expand All @@ -632,6 +667,8 @@
IndexMap::default(),
false,
false,
false,
None,
InliningMode::Document,
)
.expect("Should not fail");
Expand All @@ -654,9 +691,13 @@
IndexMap::default(),
false,
false,
false,
None,
InliningMode::Document,
)
.expect("Should not fail");
assert_eq!(buffer, b"<!DOCTYPE html><html><head></head><body data-foo=\"&amp; &nbsp; &quot;\"></body></html>");
}

// TODO adding a test case for `keep_at_rules` would be nice
}
43 changes: 35 additions & 8 deletions css-inline/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ pub struct InlineOptions<'a> {
pub keep_style_tags: bool,
/// Keep "link" tags after inlining.
pub keep_link_tags: bool,
/// Keep "@..." and wrap them into a "style" tag.
pub keep_at_rules: bool,
/// Used for loading external stylesheets via relative URLs.
pub base_url: Option<Url>,
/// Whether remote stylesheets should be loaded or not.
Expand Down Expand Up @@ -123,6 +125,13 @@ impl<'a> InlineOptions<'a> {
self
}

/// Override whether "@..." rules should be kept after processing.
#[must_use]
pub fn keep_at_rules(mut self, keep_at_rules: bool) -> Self {
self.keep_at_rules = keep_at_rules;
self
}

/// Set base URL that will be used for loading external stylesheets via relative URLs.
#[must_use]
pub fn base_url(mut self, base_url: Option<Url>) -> Self {
Expand Down Expand Up @@ -184,6 +193,7 @@ impl Default for InlineOptions<'_> {
inline_style_tags: true,
keep_style_tags: false,
keep_link_tags: false,
keep_at_rules: false,
base_url: None,
load_remote_stylesheets: true,
#[cfg(feature = "stylesheet-cache")]
Expand Down Expand Up @@ -432,14 +442,29 @@ impl<'a> CSSInliner<'a> {
.max(16),
);
let mut rule_list = Vec::with_capacity(declarations.capacity() / 3);
for rule in cssparser::StyleSheetParser::new(
&mut parser,
&mut parser::CSSRuleListParser::new(&mut declarations),
)
.flatten()
{
rule_list.push(rule);
}
let at_rules = if self.options.keep_at_rules {
let mut at_rules = String::new();
for rule in cssparser::StyleSheetParser::new(
&mut parser,
&mut parser::AtRuleFilteringParser::new(&mut declarations, &mut at_rules),
)
.flatten()
{
rule_list.push(rule);
}
// TODO debug it
Some(at_rules)
} else {
for rule in cssparser::StyleSheetParser::new(
&mut parser,
&mut parser::CSSRuleListParser::new(&mut declarations),
)
.flatten()
{
rule_list.push(rule);
}
None
};
// This cache is unused but required in the `selectors` API
let mut caches = SelectorCaches::default();
for (selectors, (start, end)) in &rule_list {
Expand Down Expand Up @@ -496,6 +521,8 @@ impl<'a> CSSInliner<'a> {
styles,
self.options.keep_style_tags,
self.options.keep_link_tags,
self.options.keep_at_rules,
at_rules,
mode,
)?;
Ok(())
Expand Down
6 changes: 6 additions & 0 deletions css-inline/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
inline_style_tags: bool,
keep_style_tags: bool,
keep_link_tags: bool,
keep_at_rules: bool,
base_url: Option<String>,
extra_css: Option<String>,
output_filename_prefix: Option<OsString>,
Expand All @@ -70,6 +71,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
inline_style_tags: true,
keep_style_tags: false,
keep_link_tags: false,
keep_at_rules: false,
base_url: None,
extra_css: None,
output_filename_prefix: None,
Expand Down Expand Up @@ -199,6 +201,9 @@ OPTIONS:
--keep-link-tags
Keep "link" tags after inlining.
--keep-at-rules
Keep "@..." rules after inlining.
--base-url
Used for loading external stylesheets via relative URLs.
Expand Down Expand Up @@ -284,6 +289,7 @@ OPTIONS:
inline_style_tags: args.inline_style_tags,
keep_style_tags: args.keep_style_tags,
keep_link_tags: args.keep_link_tags,
keep_at_rules: args.keep_at_rules,
base_url,
load_remote_stylesheets: args.load_remote_stylesheets,
#[cfg(feature = "stylesheet-cache")]
Expand Down
78 changes: 78 additions & 0 deletions css-inline/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,81 @@ impl<'i> cssparser::QualifiedRuleParser<'i> for CSSDeclarationListParser {
type QualifiedRule = Declaration<'i>;
type Error = ();
}

pub(crate) struct AtRuleFilteringParser<'d, 'i, 'o> {
declarations: &'d mut Vec<Declaration<'i>>,
at_rules: &'o mut String,
}

impl<'d, 'i, 'o> AtRuleFilteringParser<'d, 'i, 'o> {
#[inline]
pub(crate) fn new(
declarations: &'d mut Vec<Declaration<'i>>,
at_rules: &'o mut String,
) -> AtRuleFilteringParser<'d, 'i, 'o> {
AtRuleFilteringParser {
declarations,
at_rules,
}
}
}

impl<'i> cssparser::QualifiedRuleParser<'i> for AtRuleFilteringParser<'_, 'i, '_> {
type Prelude = &'i str;
type QualifiedRule = QualifiedRule<'i>;
type Error = ();

fn parse_prelude<'t>(
&mut self,
input: &mut cssparser::Parser<'i, 't>,
) -> Result<Self::Prelude, cssparser::ParseError<'i, Self::Error>> {
Ok(exhaust(input))
}

fn parse_block<'t>(
&mut self,
prelude: Self::Prelude,
_: &ParserState,
input: &mut cssparser::Parser<'i, 't>,
) -> Result<Self::QualifiedRule, cssparser::ParseError<'i, Self::Error>> {
let mut parser = CSSDeclarationListParser;
let parser = cssparser::RuleBodyParser::new(input, &mut parser);
let start = self.declarations.len();
for item in parser.flatten() {
self.declarations.push(item);
}
Ok((prelude, (start, self.declarations.len())))
}
}

impl<'i> cssparser::AtRuleParser<'i> for AtRuleFilteringParser<'_, 'i, '_> {
type Prelude = &'i str;
type AtRule = QualifiedRule<'i>;
type Error = ();

fn parse_prelude<'t>(
&mut self,
name: cssparser::CowRcStr<'i>,
input: &mut cssparser::Parser<'i, 't>,
) -> Result<Self::Prelude, cssparser::ParseError<'i, Self::Error>> {
// TODO pushing @ feels odd, there should be a less dumb way of doing it?
self.at_rules.push_str("@");
self.at_rules.push_str(&name);
Ok(exhaust(input))
}

fn parse_block<'t>(
&mut self,
prelude: Self::Prelude,
_start: &ParserState,
input: &mut cssparser::Parser<'i, 't>,
) -> Result<Self::AtRule, cssparser::ParseError<'i, Self::Error>> {
// TODO same here, pushing braces manually feels odd
let start = self.declarations.len();
self.at_rules.push_str(prelude);
self.at_rules.push_str(" {");
self.at_rules.push_str(exhaust(input));
self.at_rules.push_str("}");
Ok((prelude, (start, self.declarations.len())))
}
}
Loading
Loading