CSSStyleDeclaration implementation #680

RaphUn · 2025-05-22T00:03:22Z

Added also a css parser and a css value analyzer util

github-actions · 2025-05-22T00:03:33Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

krichprollsch · 2025-05-22T06:35:53Z

src/browser/cssom/css_parser.zig

@@ -0,0 +1,308 @@
+const std = @import("std");


The license header is missing.

krichprollsch · 2025-05-22T06:36:34Z

src/browser/cssom/css_style_declaration.zig

+    CSSRule,
+};
+
+const CSSRule = struct {};


shouldn't be pub const?

I also wasn't sure, but I checked, and it doesn't have to be (I made XMLHttpRequest non public and everything worked fine)

krichprollsch · 2025-05-22T07:40:29Z

src/browser/cssom/css_parser.zig

+        const declaration = CSSDeclaration.init(name, final_value, is_important);
+        try declarations.append(declaration);


I think I would remove CSSDeclaration.init function and use instead

try declarations.append(.{ .name = name, .value = value, .is_important = is_important, });

krichprollsch · 2025-05-22T07:41:24Z

src/browser/cssom/css_parser.zig

+                const declaration = CSSDeclaration.init(trimmed_name, final_value, is_important);
+                try declarations.append(declaration);


krichprollsch · 2025-05-22T07:42:54Z

src/browser/cssom/css_style_declaration.zig

+    }
+
+    pub fn get_cssText(self: *const CSSStyleDeclaration, state: *SessionState) ![]const u8 {
+        var buffer = std.ArrayList(u8).init(state.arena);


you should use the state.call_arena if the buffer must live only build the string returned by the function.

krichprollsch · 2025-05-22T07:44:00Z

src/browser/cssom/css_style_declaration.zig

+            if (prop.priority) try writer.writeAll(" !important");
+            try writer.writeAll("; ");
+        }
+        return buffer.toOwnedSlice();


since we are using an arena and we know it, you can direclyt retun buffer.items to avoid a useless double allocation.

krichprollsch · 2025-05-22T07:44:32Z

src/browser/cssom/css_style_declaration.zig

+        const writer = buffer.writer();
+        for (self.order.items) |name| {
+            const prop = self.store.get(name).?;
+            const escaped = try CSSValueAnalyzer.escapeCSSValue(state.arena, prop.value);


same about the call_arena

krichprollsch · 2025-05-22T07:45:57Z

src/browser/cssom/css_style_declaration.zig

+        var store_it = self.store.iterator();
+        while (store_it.next()) |entry| {
+            state.arena.free(entry.key_ptr.*);
+            state.arena.free(entry.value_ptr.value);


in arena, free are noop. So this is useless...

karlseguin · 2025-05-22T00:59:52Z

src/browser/cssom/css_parser.zig

+};
+
+pub const CSSParserState = enum {
+    seekName,


I think we would normally use snake_case for these, like a variable.

karlseguin · 2025-05-22T01:00:56Z

src/browser/cssom/css_parser.zig

+        };
+    }
+
+    pub fn parseDeclarations(text: []const u8, allocator: std.mem.Allocator) ![]CSSDeclaration {


As a general convention in Zig code, the allocator tends to be the first parameter (or the 2nd one if it's a method, where the 1st has to be the receiver).

karlseguin · 2025-05-22T01:04:34Z

src/browser/cssom/css_parser.zig

+
+    pub fn parseDeclarations(text: []const u8, allocator: std.mem.Allocator) ![]CSSDeclaration {
+        var parser = init();
+        var declarations = std.ArrayList(CSSDeclaration).init(allocator);


There's a chance the "managed" structures get removed from Zig. So we tend to favor std.ArrayListUmamanged(CSSDeclaration). But I can see here how that would be a bit annoying, since you pass it around, you'd also have to pass the allocator around.

I can think of 3 options.

1 - leave it as-is, we can deal with it if ever the stdlib removes this. I'm fine with this.

2 - Pass the allocator wherever you pass &declarations

3 - Add the allocator and declarations as fields to CSSParser, then you don't need to pass either around. You could create a helper. self.addDeclaration(...):

fn addDeclaration(self: *CSSParser, decl: CSSDeclaration) !void { return self.declarations.append(self.allocator, decl);

karlseguin · 2025-05-22T02:15:46Z

src/browser/cssom/css_parser.zig

+                            parser.state = .inQuotedValue;
+                        } else if (c == '\'') {
+                            parser.state = .inSingleQuotedValue;
+                        } else if (c == 'u' and parser.position + 3 < text.len and


I think this is equivalent:

else if (c == 'u' and std.mem.startsWith(u8, text[parser.position...], URL_PREFIX)) {

karlseguin · 2025-05-22T02:22:54Z

src/browser/cssom/css_parser.zig

+
+        try parser.finalize(text, &declarations);
+
+        return declarations.toOwnedSlice();


A bit of an eternal debate here. If this was a library, this would probably be right. But within our code, we know that allocator is actually an arena, so all this is doing is creating another copy of the slice that we don't actually need.

You can keep it as-is, which makes this code more universally usable.

You could also return declarations.items and "leak" the ArrayList..but it wouldn't really be leaking because we know we're using an arena. If you opt to go this route, you could rename allocator to arena, just so that you make it clear you expect callers to provide an arena-backed allocator. If you go this route, you might as well remove the deinit at the top of the function.

It would make your testing a bit more complicated, but instead of const testing = std.testing, you can do:

const testing = @import("../../testing.zig");

And then do:

test "CSSParser - Property with !important" { defer testing.reset(); // this resets the testing.arena_allocator const declarations = try CSSParser.parseDeclarations(text, testing.arena_allocator); // ... }

karlseguin · 2025-05-22T03:51:18Z

src/browser/cssom/css_value_analyzer.zig

+        "linear-gradient", "radial-gradient", "conic-gradient", "translate", "rotate", "scale", "skew", "matrix",
+    };
+
+    pub fn isKnownKeyword(value: []const u8) bool {


Re case-sensitivity...

if you assume the longest special value is 15 characters, i.e. radial-gradient, you could:

// 15 is our longest special value // TODO: would be nice not to have this automatically derived from our definition somehow? if (value.len > 15) { return false; } var buf: [15]u8 = undefined; const v = std.ascii.lowerString(&buf, value);

and then use v instead of value to get case-insensitive comparison. You'll have to make sure all the constants are lowercase, e.g. Hz -> hz

At this point, you could look at alternatives to looping, like std.meta.stringToEnum(e.g. https://github.com/lightpanda-io/browser/blob/main/src/browser/mime.zig#L174)..but I've benchmarked this before and the difference tends to be pretty small (but more obvious for short values, since the SIMD operation of std.mem.eql can't be leveraged, which is what we have here).

Well, I think the case-sensitivity needs to be fixed, we can optimize at a later time if needed.

karlseguin · 2025-05-22T03:54:19Z

src/browser/cssom/css_value_analyzer.zig

+        "none", "solid", "dotted", "dashed", "double", "groove", "ridge", "inset", "outset",
+    };
+
+    const ColorNames = [_][]const u8{


PascalCase is generally used for Types.

For non-global consts, I think we do color_names.

karlseguin · 2025-05-22T03:58:13Z

src/browser/html/elements.zig

+        .order = .{},
+    },
+
+    pub fn get_style(e: *parser.ElementHTML, state: *SessionState) !*CSSStyleDeclaration {


Can you add some basic tests for this? Nothing nearly as exhaustive as what you've already done, but maybe just calling el.style.setProperty with a straightforward key-value and then asserting the value with el.style.getPropertyValue

karlseguin · 2025-05-22T04:12:39Z

src/browser/cssom/css_value_analyzer.zig

+
+const std = @import("std");
+
+pub const CSSValueAnalyzer = struct {


I know the tests in css_style_declarations are good, but this is very unit-testing-friendly code where you can easily test a number of edge / error cases. I could see test cases that cover things like weird numbers, case sensitivity, various whitespaces, invalid quoting, etc, being quite useful to prevent any regression if we update the code in the future.

karlseguin · 2025-05-22T04:16:37Z

src/browser/cssom/css_value_analyzer.zig

+    pub fn startsWithFunction(value: []const u8) bool {
+        for (Functions) |func| {
+            if (value.len >= func.len + 1 and
+                std.mem.startsWith(u8, value, func) and


Could include the ( in the constants of Functions ?

RaphUn · 2025-05-23T12:52:06Z

I have read the CLA Document and I hereby sign the CLA

Use expectEqual where possible deduplicate finalize and finishDeclaration

Don't dupe value if it doesn't need to be quoted.

RaphUn added 2 commits May 22, 2025 01:51

Added support for CSSStyleDeclaration API

913568a

fixed self fetching for HTMLElement

450e345

krichprollsch reviewed May 22, 2025

View reviewed changes

karlseguin reviewed May 22, 2025

View reviewed changes

RaphUn added 5 commits May 24, 2025 01:59

various fixes according to PR review

0972710

Merge branch 'main' into css_style_declaration

9d122bd

removed unnecessary call to free

5dbdf83

added basic style test to HTMLElement

a2e0fd2

Merge branch 'main' into css_style_declaration

bd0fa14

RaphUn requested review from karlseguin and krichprollsch May 24, 2025 01:32

karlseguin added 3 commits May 24, 2025 10:08

Rename allocator to arena to make the intent more clear

6ce24b3

Use expectEqual where possible deduplicate finalize and finishDeclaration

Use fetchRemove and getOrPut to streamline map manipulation

1fe2bf5

Prefix tests (easier to filter, i.e. make test F="CSSValue")

bed394d

Don't dupe value if it doesn't need to be quoted.

karlseguin merged commit d2c13ed into main May 25, 2025
9 checks passed

karlseguin deleted the css_style_declaration branch May 25, 2025 11:34

github-actions bot locked and limited conversation to collaborators May 25, 2025

		const declaration = CSSDeclaration.init(name, final_value, is_important);
		try declarations.append(declaration);


		try parser.finalize(text, &declarations);

		return declarations.toOwnedSlice();


		const std = @import("std");

		pub const CSSValueAnalyzer = struct {

CSSStyleDeclaration implementation #680

CSSStyleDeclaration implementation #680

Uh oh!

Conversation

RaphUn commented May 22, 2025

Uh oh!

github-actions bot commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

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!

RaphUn commented May 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented May 22, 2025 •

edited

Loading