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.

Sign up for GitHub

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

WIP: Added additional arg for turning styles into properties #139

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

WIP: Added additional arg for turning styles into properties #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
209036e
Added additional arg for turning styles into properties
kastolars Jan 23, 2022
8a2a072
Added line for handling new styles as property option
kastolars Jan 23, 2022
cb4dd0b
Added builder function and changed process_css extra_css argument as …
kastolars Jan 23, 2022
8f75d56
Changed naming of props to attributes
kastolars Jan 24, 2022
7a9c350
Updated python bindings with new flag
kastolars Jan 24, 2022
8d61dc4
Updated wasm bindings and python bindings tests with new flag
kastolars Jan 24, 2022
0959ecc
Changed comment to 'separate attributes'
kastolars Jan 30, 2022
1b9bc34
Change text to 'separate attributes'
kastolars Jan 30, 2022
3365a79
Added branch for inserting attributes
kastolars Feb 1, 2022
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
18 changes: 12 additions & 6 deletions bindings/python/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,12 @@ fn parse_url(url: Option<String>) -> PyResult<Option<url::Url>> {
})
}

/// CSSInliner(inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None)
/// CSSInliner(inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None, styles_as_attributes=False)
///
/// Customizable CSS inliner.
#[pyclass]
#[pyo3(
text_signature = "(inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None)"
text_signature = "(inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None, styles_as_attributes=False)"
)]
struct CSSInliner {
inner: rust_inline::CSSInliner<'static>,
Expand All @@ -85,13 +85,15 @@ impl CSSInliner {
base_url: Option<String>,
load_remote_stylesheets: Option<bool>,
extra_css: Option<String>,
styles_as_attributes: Option<bool>,
) -> PyResult<Self> {
let options = rust_inline::InlineOptions {
inline_style_tags: inline_style_tags.unwrap_or(true),
remove_style_tags: remove_style_tags.unwrap_or(false),
base_url: parse_url(base_url)?,
load_remote_stylesheets: load_remote_stylesheets.unwrap_or(true),
extra_css: extra_css.map(Cow::Owned),
styles_as_attributes: styles_as_attributes.unwrap_or(false),
};
Ok(CSSInliner {
inner: rust_inline::CSSInliner::new(options),
Expand All @@ -115,12 +117,12 @@ impl CSSInliner {
}
}

/// inline(html, inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None)
/// inline(html, inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None, styles_as_attributes=False)
///
/// Inline CSS in the given HTML document
#[pyfunction]
#[pyo3(
text_signature = "(html, inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None)"
text_signature = "(html, inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None, styles_as_attributes=False)"
)]
fn inline(
html: &str,
Expand All @@ -129,24 +131,26 @@ fn inline(
base_url: Option<String>,
load_remote_stylesheets: Option<bool>,
extra_css: Option<&str>,
styles_as_attributes: Option<bool>,
) -> PyResult<String> {
let options = rust_inline::InlineOptions {
inline_style_tags: inline_style_tags.unwrap_or(true),
remove_style_tags: remove_style_tags.unwrap_or(false),
base_url: parse_url(base_url)?,
load_remote_stylesheets: load_remote_stylesheets.unwrap_or(true),
extra_css: extra_css.map(Cow::Borrowed),
styles_as_attributes: styles_as_attributes.unwrap_or(false),
};
let inliner = rust_inline::CSSInliner::new(options);
Ok(inliner.inline(html).map_err(InlineErrorWrapper)?)
}

/// inline_many(htmls, inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None)
/// inline_many(htmls, inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None, styles_as_attributes=False)
///
/// Inline CSS in multiple HTML documents
#[pyfunction]
#[pyo3(
text_signature = "(htmls, inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None)"
text_signature = "(htmls, inline_style_tags=True, remove_style_tags=False, base_url=None, load_remote_stylesheets=True, extra_css=None, styles_as_attributes=False)"
)]
fn inline_many(
htmls: &PyList,
Expand All @@ -155,13 +159,15 @@ fn inline_many(
base_url: Option<String>,
load_remote_stylesheets: Option<bool>,
extra_css: Option<&str>,
styles_as_attributes: Option<bool>,
) -> PyResult<Vec<String>> {
let options = rust_inline::InlineOptions {
inline_style_tags: inline_style_tags.unwrap_or(true),
remove_style_tags: remove_style_tags.unwrap_or(false),
base_url: parse_url(base_url)?,
load_remote_stylesheets: load_remote_stylesheets.unwrap_or(true),
extra_css: extra_css.map(Cow::Borrowed),
styles_as_attributes: styles_as_attributes.unwrap_or(false),
};
let inliner = rust_inline::CSSInliner::new(options);
inline_many_impl(&inliner, htmls)
Expand Down
3 changes: 3 additions & 0 deletions bindings/python/tests-py/test_inlining.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ def test_invalid_base_url():
base_url=provisional.urls() | st.none(),
load_remote_stylesheets=st.booleans() | st.none(),
extra_css=st.text() | st.none(),
styles_as_attributes=st.booleans() | st.none(),
)
@settings(max_examples=1000)
def test_random_input(
Expand All @@ -98,6 +99,7 @@ def test_random_input(
base_url,
load_remote_stylesheets,
extra_css,
styles_as_attributes,
):
with suppress(ValueError):
inliner = css_inline.CSSInliner(
Expand All @@ -106,5 +108,6 @@ def test_random_input(
base_url=base_url,
load_remote_stylesheets=load_remote_stylesheets,
extra_css=extra_css,
styles_as_attributes=styles_as_attributes,
)
inliner.inline(document)
4 changes: 4 additions & 0 deletions bindings/wasm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ struct Options {
base_url: Option<String>,
load_remote_stylesheets: bool,
extra_css: Option<String>,
styles_as_attributes: bool,
}

impl Default for Options {
Expand All @@ -83,6 +84,7 @@ impl Default for Options {
base_url: None,
load_remote_stylesheets: true,
extra_css: None,
styles_as_attributes: false,
}
}
}
Expand All @@ -105,6 +107,7 @@ impl TryFrom<Options> for rust_inline::InlineOptions<'_> {
base_url: parse_url(value.base_url)?,
load_remote_stylesheets: value.load_remote_stylesheets,
extra_css: value.extra_css.map(Cow::Owned),
styles_as_attributes: value.styles_as_attributes,
})
}
}
Expand All @@ -129,6 +132,7 @@ interface InlineOptions {
base_url?: string,
load_remote_stylesheets?: boolean,
extra_css?: string,
styles_as_attributes?: boolean,
}

export function inline(html: string, options?: InlineOptions): string;
Expand Down
23 changes: 21 additions & 2 deletions css-inline/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ pub struct InlineOptions<'a> {
// Python wrapper for `CSSInliner` and `&str` in Rust & simple functions on the Python side
/// Additional CSS to inline.
pub extra_css: Option<Cow<'a, str>>,
/// Whether to break down styles into separate attributes
pub styles_as_attributes: bool,
}

impl<'a> InlineOptions<'a> {
Expand All @@ -87,6 +89,7 @@ impl<'a> InlineOptions<'a> {
base_url: None,
load_remote_stylesheets: true,
extra_css: None,
styles_as_attributes: false,
}
}

Expand Down Expand Up @@ -125,6 +128,13 @@ impl<'a> InlineOptions<'a> {
self
}

/// Insert styles as attributes
#[must_use]
pub fn styles_as_attributes(mut self, styles_as_attributes: bool) -> Self {
self.styles_as_attributes = styles_as_attributes;
self
}

/// Create a new `CSSInliner` instance from this options.
#[must_use]
pub const fn build(self) -> CSSInliner<'a> {
Expand All @@ -141,6 +151,7 @@ impl Default for InlineOptions<'_> {
base_url: None,
load_remote_stylesheets: true,
extra_css: None,
styles_as_attributes: false,
}
}
}
Expand Down Expand Up @@ -284,7 +295,7 @@ impl<'a> CSSInliner<'a> {
}
}
if let Some(extra_css) = &self.options.extra_css {
process_css(&document, extra_css, &mut styles);
process_css(&document, extra_css.as_ref(), &mut styles);
}
for (node_id, styles) in styles {
// SAFETY: All nodes are alive as long as `document` is in scope.
Expand All @@ -299,7 +310,15 @@ impl<'a> CSSInliner<'a> {
.attributes
.try_borrow_mut()
{
if let Some(existing_style) = attributes.get_mut("style") {
if self.options.styles_as_attributes {
Copy link
Owner

Choose a reason for hiding this comment

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

Here we'd have a similar situation to the branchs below:

  • If there is no "style" attribute, then go over styles (similar to line 319) and call attributes.insert with rule name / value.
  • Otherwise, the existing "style" attribute should be popped from attributes, merged with collected styles, and then those "final" styles should be inserted into attributes

Copy link
Author

Choose a reason for hiding this comment

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

Okay so within the conditional branch I created, we will have 2 conditional branches?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep

Copy link
Author

Choose a reason for hiding this comment

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

Okay so I think I got the first bullet point completed, I'm not quite sure if I understand the second one yet, but it's also a bit late for me haha. Will take a look tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

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

Awesome! I think we can use attribute.remove straight away instead of get_mut (it returns Option), so the output doesn't have the "style" attribute.

For the second point, the workflow would look like the other branch with attributes.insert, but the existing "style" value should be merged together with ones collected from "style" tags (or other places).

At the moment, merge_styles outputs Result<String> which is not exactly what is needed. Instead, there could be a new function that will insert new attributes into attributes instead of pushing them to a string.

Something like this

fn insert_as_attributes(
  existing_style: &str, 
  new_styles: &AHashMap<String, (Specificity, String)>, 
  attributes: &mut Attributes
) -> Result<()> {
    let mut input = cssparser::ParserInput::new(existing_style);
    let mut parser = cssparser::Parser::new(&mut input);
    let declarations = cssparser::DeclarationListParser::new(&mut parser, parser::CSSDeclarationListParser);
    let mut buffer: SmallVec<[String; 8]> = smallvec![];
    for declaration in declarations {
        let (name, value) = declaration?;
        attributes.insert(name, value);
        buffer.push(name.to_string());
    }
    for (property, (_, value)) in new_styles {
        if !buffer.contains(property) {
            attributes.insert(property, value)
        }
    }
    Ok(())
}

And the last branch would look like this:

insert_at_attributes(&existing_style.value, &styles, &mut attributes)

I didn't test these code samples, but they should be alright :) The main idea is that existing "style" attribute styles are inserted as separate attributes immediately as they have the maximum priority, then the other ones are inserted only if they aren't clashing with already inserted ones.

Then, there is a small corner case with quoting the value (see the replace_double_quotes macro). Before inserting, each property name & value should be checked:

if name.starts_with("font-family") && memchr::memchr(b'"', value.as_bytes()).is_some() {
    attributes.insert(name, &value.replace('"', "\'"))
} else {
    attributes.insert(name, value)
};

which could be also a small macro (the naming probably isn't much important - maybe insert_unquoted?)

Then, I think a few tests would be nice to have :) And a changelog entry

if let Some(_existing_style) = attributes.get_mut("style") {
// TODO
} else {
for (name, (_, value)) in styles {
attributes.insert(name, value);
}
}
} else if let Some(existing_style) = attributes.get_mut("style") {
*existing_style = merge_styles(existing_style, &styles)?;
} else {
let mut final_styles = String::with_capacity(128);
Expand Down
6 changes: 6 additions & 0 deletions css-inline/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ OPTIONS:

--extra-css
Additional CSS to inline.

--styles-as-attributes
Will insert styles as separate attributes.
"#
)
.as_bytes();
Expand All @@ -58,6 +61,7 @@ struct Args {
extra_css: Option<String>,
load_remote_stylesheets: bool,
files: Vec<String>,
styles_as_attributes: bool,
}

fn parse_url(url: Option<String>) -> Result<Option<url::Url>, url::ParseError> {
Expand All @@ -83,6 +87,7 @@ fn main() -> Result<(), Box<dyn Error>> {
base_url: args.opt_value_from_str("--base-url")?,
extra_css: args.opt_value_from_str("--extra-css")?,
load_remote_stylesheets: args.contains("--load-remote-stylesheets"),
styles_as_attributes: args.opt_value_from_str("--styles-as-attributes")?.unwrap_or(false),
files: args.free()?,
};
let options = InlineOptions {
Expand All @@ -91,6 +96,7 @@ fn main() -> Result<(), Box<dyn Error>> {
base_url: parse_url(args.base_url)?,
load_remote_stylesheets: args.load_remote_stylesheets,
extra_css: args.extra_css.as_deref().map(Cow::Borrowed),
styles_as_attributes: args.styles_as_attributes,
};
let inliner = CSSInliner::new(options);
if args.files.is_empty() {
Expand Down