WIP: Added additional arg for turning styles into properties #139
Conversation
|
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. |
There was a problem hiding this comment.
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 :)
| @@ -299,7 +310,9 @@ impl<'a> CSSInliner<'a> { | |||
| .attributes | |||
| .try_borrow_mut() | |||
| { | |||
| if let Some(existing_style) = attributes.get_mut("style") { | |||
| if self.options.styles_as_attributes { | |||
There was a problem hiding this comment.
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 callattributes.insertwith 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 intoattributes
There was a problem hiding this comment.
Okay so within the conditional branch I created, we will have 2 conditional branches?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
I will find some time this weekend to address these, thanks! |
Co-authored-by: Dmitry Dygalo <[email protected]>
Co-authored-by: Dmitry Dygalo <[email protected]>
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 |
Stranger6667
force-pushed
the
master
branch
2 times, most recently
from
60d11b2 to
dab7908
Compare
Stranger6667
force-pushed
the
master
branch
4 times, most recently
from
0629ccb to
5004947
Compare
|
@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 |
|
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 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! |
Resolves #138