WIP: Added additional arg for turning styles into properties

[kastolars]

Resolves #138

[Stranger6667]

Thanks! Great first iteration :) I left a small comment about naming & will comment a bit more about the next step a bit later

[Stranger6667]

Also, you got to adjust Python bindings to keep the CI green :) The changes should be similar to the changes you've already made

[kastolars]

Also, you got to adjust Python bindings to keep the CI green :) The changes should be similar to the changes you've already made

I will give it a shot and ping you if I have any trouble.

[Stranger6667]

Great! I added a few notes :)

Also, the "Features & Configuration" section of the README file would need some adjustments as well to include the new config option :)

[Stranger6667]

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

[kastolars]

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

[Stranger6667]

Yep

[kastolars]

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.

[Stranger6667]

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

[kastolars]

I will find some time this weekend to address these, thanks!

[kastolars]

Great! I added a few notes :)

Also, the "Features & Configuration" section of the README file would need some adjustments as well to include the new config option :)

For some reason when I open the README.md in my IDE, it only has one line and says "../README.md"... Not sure what that's about

[Stranger6667]

For some reason when I open the README.md in my IDE, it only has one line and says "../README.md"... Not sure what that's about

I think it happens if you open README.md which is inside the css-inline directory because it is a symlink. Opening README.md from the repository root should work fine :) Let me know if the issue persists

[caseyjhol]

@Stranger6667 I tried resolving the merge conflicts to see if I could get this across the finish line, but unfortunately I'm not familiar enough with Rust or the codebase to pull it off. It would also be helpful if, for this implementation, we ensured only attributes that are possible are added (e.g. we don't want to set bgcolor on a div).

[Stranger6667]

@caseyjhol

First of all, I appreciate your effort! I'd be happy to assist with moving it forward.

At this point, the difference between the branches is far too big for a straightforward conflict resolution. Since then, the inlining itself has moved to the serialization phase (i.e. when we write tags & their attributes to the output buffer). The idea is to write those attributes at the very end of the element's serialization instead of writing "style" as done above that line. It will require reworking the style merging process a bit, but it should be more or less similar to the current implementation. We can leave the performance nuances aside for now, but for the record, I think that generalizing the merge behavior via a separate trait is the way to go (i.e. depending on the config option we will just pass a different marker to HtmlSerializer).

I think the first step could be opening a new PR with a few cherry-picked commits e.g. CLI option & config option and then adding new things as mentioned above. I'll try to add some more details to the original issue (or the new PR) and maybe a few more comments to the codebase to make it clearer.

As for ensuring the proper attributes - I am all for it, I think it is a great feature to have! However, it could be better to split it into a separate PR.

Let me know what you think about it!