feat: Add `keep_at_rules` option by kamilzych · Pull Request #485 · Stranger6667/css-inline

kamilzych · 2025-06-27T11:41:39Z

Rel #265 (comment)

codspeed-hq · 2025-06-27T11:51:21Z

CodSpeed Performance Report

Merging #485 will not alter performance

_{Comparing kamilzych:feat/keep-at-rules (bdd9bfe) with master (96a47bc)}

Summary

✅ 6 untouched benchmarks

codecov · 2025-06-27T11:51:27Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.11%. Comparing base (bd04285) to head (bdd9bfe).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #485      +/-   ##
==========================================
+ Coverage   88.43%   89.11%   +0.68%     
==========================================
  Files          18       18              
  Lines        1919     2040     +121     
==========================================
+ Hits         1697     1818     +121     
  Misses        222      222

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Stranger6667

Great start! Definitely on the right track :) I left a few comments

css-inline/src/html/attributes.rs

css-inline/src/html/serializer.rs

Stranger6667 · 2025-06-27T15:02:15Z

css-inline/src/html/serializer.rs


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

+                // TODO this part is the one that I don't like the most


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:

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.

Are there cases when head is not present? Maybe in HTML fragments? see inline_fragment

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

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

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?

css-inline/src/parser.rs

Stranger6667 · 2025-07-19T15:30:18Z

Hi @kamilzych

Let me know if you need any assistance or if you plan to continue working on this PR. I'd be happy to help or take over in case if you don't plan to continue this PR.

kamilzych · 2025-07-19T17:35:08Z

@Stranger6667 Hi! Apologies for lack of activity, I simply had no time, life happened ¯\_(ツ)_/¯

I've just pushed a commit which addresses some of your comments. I'd love to finish the whole PR but I'm not sure when I have enough time to move this forward.
That said, if you have time to take it over, I'm absolutely fine with it. If you don't, I'll do my best to finish it ASAP and in such case I'd appreciate a review of the mentioned commit and perhaps some guidance about the next steps 🙏

Stranger6667 · 2025-07-19T18:43:13Z

Awesome! Thank you so much! :) I’ll be on vacation till the end of next week, but I’ll review the changes in any event :) Let’s see how it goes

Stranger6667 · 2025-07-19T21:38:38Z

Amazing to see that we already have a working version! There are a few minor things left; there is no time pressure, so I'm just noting what is left to do, and maybe I'll push some changes to it in the next few evenings:

Stranger6667 · 2025-07-20T06:50:17Z

As for the next steps, I think most of them are about adding new config option to bindings, so they compile. This flag is the same as others in terms of behavior, so it should be more or less straightforward.

about the error, we can add an extra check to inline_fragment

Stranger6667 · 2025-07-20T06:52:13Z

css-inline/src/main.rs


    --keep-link-tags
        Keep "link" tags after inlining.
+


There is a small place above where this flag should be added too. The handle_boolean_flag function

Fixed in 960715e

kamilzych · 2025-07-24T10:07:33Z

@Stranger6667 Hi again! I should have some time tomorrow to work on this again. Updating CHANGELOG, README and other docs should be easy. I can also take a look into

Error for inline_fragment + keep_at_rules?

and perhaps add a test(s) for it and other cases.

The only question I have right now is about bindings. Do you have any docs, guidance, or previous PR that could show me the ropes?

Stranger6667 · 2025-07-24T13:51:35Z

@kamilzych

Hi! I’ll have access to my laptop in a few hours and will provide you with more details :)

Stranger6667 · 2025-07-24T14:56:38Z

For bindings, all the TODO entries are similar. The core of them is:

Fill the options structure with the missing keep_at_rules
Extract the keep_at_rules value the same way as it goes for other boolean flags like keep_style_tags
Update language-specific annotations / docstrings

Depending on the language, it requires a bit different annotations, but you can just copy them from keep_style_tags.

For Ruby, extend the get_kwargs call by adding the field name to the array of strings + adding Option<bool> to one of the generics so the name of the new kwarg corresponds to that new Option<bool> position.

For Python, you need to add keep_at_rules = False to the signature macro + update the docstring the same way, so the positions match.

For C, you need to extend CssInlinerOptions with one new boolean and that is it.

For Java, extract a new boolean from the environment:

    let keep_at_rules = env.get_bool_field(&cfg, "keepAtRules")?;

And add it to the options builder.

For JavaScript, extend options::Options with this new boolean field, then the deserialization logic will be automatically derived. Also, you'll need to update the InlineOptions interface inside the INLINE constant. Then running npm run build:wasm && npm run build:wasm-web inside bindings/javascript should be enough to regenerate everything needed.

Let me know if I missed anything, will be happy to elaborate :)

Stranger6667 · 2025-07-25T09:02:58Z

One minor detail about Ruby: bindings use the latest release from crates.io to avoid packaging issues. It is suboptimal, but I am fine with having this PR point to the local crate, so we see everything passes. Then I'll update it before the release, and maybe add a better solution to avoid breaking releases for Ruby bindings.

Stranger6667 · 2025-07-25T09:11:14Z

css-inline/tests/test_inlining.rs

+</body>
+</html>"#;
+    let inlined = inliner.inline(html).unwrap();
+    let expected = "<html><head><style>@media (max-width: 600px)  { h1 { font-size: 18px; } }</style>\n\n</head>\n<body>\n<h1 style=\"color: blue;\">Hello</h1><p style=\"margin: 10px;\">World</p>\n\n</body></html>";


I see there is a double space - (max-width: 600px) {. I guess we can keep what is in the original string without adding extra space - it should be a bit cleaner + a bit faster

Unless it breaks some corner cases

Fixed in 430ca64

Had to add it back at the end though be2c653 😅 Without it there's no space between multiple @media declarations, like this
@media (max-width: 600px) { ... }@media (max-width: 400px) { ... }

kamilzych · 2025-07-25T10:36:12Z

@Stranger6667 Regarding error when inlining fragments. I've just checked and it seems that keep_style_tags=true doesn't throw any error when inlining fragment. If I understand correctly, it is pretty much the same case, right?
If so, perhaps it's better to stick with this approach for unified experience and simply ignore keep_at_rules for now when inlining fragments? Changing this to throw errors for both flags could be a separate issue. What do you think?

here's the test I used for testing

#[test]
fn inline_fragment_keep_style_tags() {
    let inliner = CSSInliner::options().keep_style_tags(true).build();
    const HTML: &str = r#"<main>
<h1>Hello</h1>
<section>
<p>who am i</p>
</section>
</main>"#;

    const CSS: &str = r#"
p {
    color: red;
}

h1 {
    color: blue;
}
"#;
    let inlined = inliner.inline_fragment(HTML, CSS).unwrap();
    assert_eq!(inlined, "<main>\n<h1 style=\"color: blue;\">Hello</h1>\n<section>\n<p style=\"color: red;\">who am i</p>\n</section>\n</main>");
}

Stranger6667 · 2025-07-25T11:57:10Z

@kamilzych I completely agree with you! lets handle that part separately

Stranger6667 · 2025-07-25T12:55:27Z

Awesome! The last failure with Ruby bindings could be resolved by running cargo generate-lockfile in bindings/ruby + copying the Cargo.lock file to ext/css_inline

kamilzych · 2025-07-25T14:49:53Z

Awesome! The last failure with Ruby bindings could be resolved by running cargo generate-lockfile in bindings/ruby + copying the Cargo.lock file to ext/css_inline

Unfortunately, this is where I failed 🥲

After changing bindings/ruby/ext/css_inline/Cargo.toml to this

[dependencies.css-inline]
path = "../../../../css-inline"
version = "*"

running cargo generate-lockfile fails with package collision in the lockfile:

If I keep version 0.16 the command generates Lockfile, but then it will still use 0.16 from crates.io (at least that's what I think :))

Stranger6667 · 2025-07-25T14:56:52Z

No worries! I will take a look :) I think at this point everything should be resolved and the feature is ready :) thank you so much for working on it! I plan to merge it if there are no unresolved issues and will sort out the ruby part later

kamilzych · 2025-07-25T15:20:13Z

Thank you too for all the patience, guidance and help 🙏 I had a lot of fun when working on it!

In the meantime, I created #514

I will also take a look at unit tests, might add more.

kamilzych · 2025-07-25T15:48:37Z

@Stranger6667 I can already see some issues. Some are simply incorrect behaviour which I will try to fix, but for one I have a question. Let's consider such test:

    let html = html!("@media (max-width: 767px) { padding: 0;} h1 {background-color: blue;}", "<h1>Hello world!</h1>");
    let options = InlineOptions {
        inline_style_tags: false,
        keep_style_tags: false,
        keep_at_rules: true,
        ..Default::default()
    };
    let inliner = CSSInliner::new(options);
    let result = inliner.inline(&html).unwrap();
    assert_eq!(
        result,
        "???"
    )

What should we expect for such options setup? Right now, the result is <html><head></head><body><h1>Hello world!</h1></body></html> so keep_at_rules is ignored.

Stranger6667 · 2025-07-25T16:37:37Z

Thank you too for all the patience, guidance and help 🙏 I had a lot of fun when working on it!

I am glad to hear it! :)

Re: config options combo. From a use case point of view, I think the user might want to remove everything except at-rules, so I'd expect a style element with only the @media part.

As keep_style_tags defaults to false, it is equivalent to options().inline_style_tags(false).keep_at_rules(true). In this scenario, I read it like "don't inline anything, but keep @-rules" + the default behavior is to remove styles, so it kinda checks out. I see where the contradiction is, but I am not sure whether it would be surprising for the user if such a combo will remove all except @-rules. What do you think?

P.S., adding an unused private field could be a good idea to avoid a public API to construct this without InlineOptions

kamilzych · 2025-07-25T16:54:47Z

Yeah, with keep_style_tags: false && keep_at_rules: true it does work as you described and I think that setting inline_style_tags to false shouldn't change anything. I'll fix it.

adding an unused private field could be a good idea to avoid a public API to construct this without InlineOptions

Not sure if I follow, where this private field should be added? 🤔

Stranger6667 · 2025-07-25T17:02:03Z

Thanks! :)

I think adding _private: () to InlineOptions should be the way to go, but it could be done separately, as such a change would only affect what the user could do and is not directly related to the new config option.

The idea is that having this private field allows for fewer breaking changes. Right now, dependencies could build this struct directly with or without using Default::default() - a private field will make it impossible to build this struct directly, and the only possible way will be using a builder or ::default(). For this reason, adding new config options will not break the dependencies

kamilzych · 2025-07-25T18:22:23Z

Ah, I see. Makes sense, although I'm not sure if it should be a part of this PR 🤔

FYI, I've just committed a failing test, fixing it seems to be a bit harder than I thought and I have to run. I hope to find some time next week to work on it again. Thanks again! 🙏

Stranger6667 · 2025-07-25T18:35:46Z

Sure thing! The private field thing is definitely for later!

Thank you again for working on this :)
I'll take a deeper look over the weekend and hope that I will be able to finish things that are in the TODO list here :) Have a great weekend

Signed-off-by: Dmitry Dygalo <dmitry@dygalo.dev>

Stranger6667 · 2025-07-26T11:05:22Z

I think we are ready to go! :) I am so happy to see this feature implemented! :) Great work!

I will deal with Ruby separately

kamilzych · 2025-07-26T14:10:22Z

Awesome, glad to hear it! Perhaps I'll add more unit tests for corner cases (e.g. at-rules in external CSS, various flags' configs etc) in the future. Besides, once you release it, I'll test this feature performance-wise with real-world case (quite complex HTML with some at-rules I need to keep in output HTML).
Great collabo! 🎉

Stranger6667 · 2025-07-26T14:14:24Z

Great! Also, feel free to add that HTML as a benchmark here (if it could be shared publicly). The structure there could be extended so we can benchmark the new config option too (e.g. add optional "keep_at_rules": true to the new benchmark)

kamilzych force-pushed the feat/keep-at-rules branch from 155e7b6 to 64161f7 Compare June 27, 2025 11:47

kamilzych mentioned this pull request Jun 27, 2025

Keep @media query CSS for responsive #265

Closed

Stranger6667 requested changes Jun 27, 2025

View reviewed changes

kamilzych force-pushed the feat/keep-at-rules branch from ffe46ee to c4adfc7 Compare July 19, 2025 17:27

kamilzych force-pushed the feat/keep-at-rules branch from c4adfc7 to fdfd7c8 Compare July 19, 2025 22:14

Stranger6667 reviewed Jul 20, 2025

View reviewed changes

kamilzych force-pushed the feat/keep-at-rules branch 2 times, most recently from 5ea0ba8 to 6156284 Compare July 25, 2025 07:51

feat: Add keep_at_rules option

f190f3b

kamilzych force-pushed the feat/keep-at-rules branch from 6156284 to 67574d3 Compare July 25, 2025 08:25

Stranger6667 reviewed Jul 25, 2025

View reviewed changes

kamilzych added 3 commits July 25, 2025 13:07

refactor(keep-at-rules): refactor keep-at-rules

b902376

docs(keep-at-rules): Add docs

5d6aad5

fix(keep-at-rules): Remove double space

430ca64

kamilzych force-pushed the feat/keep-at-rules branch 2 times, most recently from 733dc23 to 104de47 Compare July 25, 2025 12:21

fix(keep-at-rules): Update bindings

0a77f50

kamilzych force-pushed the feat/keep-at-rules branch from 104de47 to 0a77f50 Compare July 25, 2025 12:26

feat(keep-at-rules): Handle keep-at-rules flag in main.rs

960715e

kamilzych mentioned this pull request Jul 25, 2025

Consider throwing error when using fragments inlining with some flags #514

Open

feat(keep-at-rules): Add back pushing space when parsing at_rule block

cc509a2

kamilzych force-pushed the feat/keep-at-rules branch from be2c653 to cc509a2 Compare July 25, 2025 17:15

kamilzych added 2 commits July 25, 2025 19:43

feat(keep-at-rules): Add test for CLI

d459eb5

feat(keep-at-rules): Add failing test

79e61d7

Stranger6667 added 3 commits July 26, 2025 12:44

chore: Handle corner case

a77caa8

Signed-off-by: Dmitry Dygalo <dmitry@dygalo.dev>

chore(ruby): Update lockfile

eeaee84

Signed-off-by: Dmitry Dygalo <dmitry@dygalo.dev>

chore(ruby): Update lockfile

bdd9bfe

Signed-off-by: Dmitry Dygalo <dmitry@dygalo.dev>

Stranger6667 marked this pull request as ready for review July 26, 2025 11:04

Stranger6667 merged commit 9d76579 into Stranger6667:master Jul 26, 2025
57 of 76 checks passed

mrdziuban mentioned this pull request Oct 17, 2025

feat: Add option to minify CSS #559

Merged

4 tasks

kamilzych deleted the feat/keep-at-rules branch January 4, 2026 18:10


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

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

Uh oh!

Conversation

kamilzych commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #485 will not alter performance

Summary

Uh oh!

codecov bot commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Stranger6667 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Stranger6667 commented Jul 19, 2025

Uh oh!

kamilzych commented Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stranger6667 commented Jul 19, 2025

Uh oh!

Stranger6667 commented Jul 19, 2025

Uh oh!

Stranger6667 commented Jul 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamilzych commented Jul 24, 2025

Uh oh!

Stranger6667 commented Jul 24, 2025

Uh oh!

Stranger6667 commented Jul 24, 2025

Uh oh!

Stranger6667 commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamilzych commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stranger6667 commented Jul 25, 2025

Uh oh!

Stranger6667 commented Jul 25, 2025

Uh oh!

kamilzych commented Jul 25, 2025

Uh oh!

Stranger6667 commented Jul 25, 2025

Uh oh!

kamilzych commented Jul 25, 2025

Uh oh!

kamilzych commented Jul 25, 2025

Uh oh!

Stranger6667 commented Jul 25, 2025

Uh oh!

kamilzych commented Jul 25, 2025

kamilzych commented Jun 27, 2025 •

edited

Loading

codspeed-hq bot commented Jun 27, 2025 •

edited

Loading

codecov bot commented Jun 27, 2025 •

edited

Loading

kamilzych commented Jul 19, 2025 •

edited

Loading

kamilzych commented Jul 25, 2025 •

edited

Loading