Handling class names in a node

[Kiyoshika]

I know there was someone interested in working on the CSS parser (not sure at what point, but also low priority right now) which made me think about the current ability to handle class names in an element.

I think the next contribution I planned to make was adding a couple utilities for checking, adding/removing classes from an element.

For instance,

<div class="one two three"></div>

This element has three classes, but we could also programmatically (through javascript later) add/remove/toggle classes.

My proposal is adding a new struct

pub struct ElementClass {
    /// a map of classes: key = name, value = is_active
    /// the is_active is used to toggle a class (JavaScript API)
    class_map: HashMap<String, bool>
}

impl ElementClass {
    /// Check if class name exists
    pub fn contains(&self, name: String);

    /// Add a new class (if already exists, does nothing)
    pub fn add(&self, name: String);

    /// Remove a class (does nothing if not exists)
    pub fn remove(&self, name: String);

    /// Toggle a class active/inactive
    pub fn toggle(&self, name: String);

    /// Set explicitly if a class is active or not
    pub fn set_active(&self, is_active: bool);
}

If I'm not mistaken, in the current implementation of the parser (as of 01 Oct 23) class would be an attribute, so my thought is after a tag is closed (in the above example, when </div> is reached) maybe we take the value of the class attribute and use that to construct the ElementClass instance which can be stored in the Node as a property

pub struct Node {
    // ...
    pub classes: ElementClass,
    // ...
}

Then we'd be able to use it when we're trying to apply CSS classes, something like:

if node.classes.contains("one".to_string()) {
    // apply class logic
}

Are there any thoughts/suggestions/issues on this before I start the work on it?

[Kiyoshika]

aside: typically there may not be many class names used in an element at once, if we are concerned about the memory footprint of a hash map, my original idea was to use a Vec<(String, bool)>

[mihna123]

This seems good to me. I'm currently working on a css parser which will return a CSSOM but I'm not sure how we will combine the two parsers together?

[Kiyoshika]

ok, if @jaytaph is also good, I will self-assign an issue to implement this

at the moment I am also unsure, I think it will be the user agent / browser client that integrates the two maybe through a combination of searching for a tag name or named tag ID (which I've been wanting to add a way to query for IDs/names in a DOM tree a bit later) and checking which classes to apply (if any), but that's just a very high level thought

[emwalker]

It's often possible to cast String to &str, which doesn't require an allocation. How hard would it be to modify the struct interface to use &str instead?

impl ElementClass {
    /// Check if class name exists
    pub fn contains(&self, name: &str);

    /// Add a new class (if already exists, does nothing)
    pub fn add(&self, name: &str);

    /// Remove a class (does nothing if not exists)
    pub fn remove(&self, name: &str);

    /// Toggle a class active/inactive
    pub fn toggle(&self, name: &str);

    /// Set explicitly if a class is active or not
    pub fn set_active(&self, is_active: bool);
}

// in calling code, just an example ...
let name = "one".to_owned(); // name is String

if klass.contains(&name) { 
  // ...
}

[Kiyoshika]

The benefit of &str is that it's better supported for string literals, but not sure in the future if there will be a lot of string literals used (e.g., by the parser) or if they are Strings returned by some other struct

[Kiyoshika]

Now that I think about it a bit, if we take a &str and since the map (or vector, whatever we decide) takes a String, would we have to create another one prior to inserting it?

[emwalker]

One nice goal for the parser would be to be zero copy (probably hard at first, and not ergonomic because of lifetimes, but a good goal to aim for nonetheless). With zero-copy, the input would be allocated at the start, and then not cloned from that point on, until you're actually creating the DOM.

The challenge with transferring ownership in a function interface is that it might be convenient to use the argument later on outside of the function without doing a clone to pass the argument in. Ideally you'd only transfer ownership if the purpose of the interface is to consume the argument. If we're using String as an argument elsewhere, we might want to switch to &str.

struct Name(String);

let name = Name("one".to_owned());

// Using &str, no clone required if you want to use `name` again later on
if klass.contains(&name.0) {
  // ...
}

// Using String, requires a clone if you don't want to consume `name`
if klass.contains(name.0.clone()) {
  // ...
}

Maps generally want an owned version of a struct for the key (so you have to clone), although there might be clever ways around this. But we're only aiming to minimize allocations, not get rid of them altogether.

[emwalker]

I haven't looked closely, but in the case of your interface, some of the methods might actually want to consume the input, so no need to change this detail if that's the case. Also, lifetimes can make things really tricky, especially when coming up to speed on Rust, so better to just clone for now if lifetimes get fiddly; it should be possible to dial things in later.

[Kiyoshika]

those are fair points. yeah, I'm not sure 100% how this interface will be used throughout the parser / user-agent so it's hard to predict lifetimes, but I think we can just try using &str as a parameter and clone on insertion and if we need to change things up, it shouldn't be a hard refactor