Making node.attributes.insert more responsive to the DOM

[Kiyoshika]

Right now we have document.set_named_id to add a named ID to the DOM to allow for querying (think document.getElementById()) and this will also modify the underlying ID attribute in the element.

However, it is also possible to directly modify node.attributes.insert("id", "somevalue") which does not touch the DOM tree at all and this can cause problems (e.g., trying to query the DOM for the ID somevalue would return nothing in this case)

To fix this problem, I wanted to modify the ElementData.ElementAttributes.insert method to handle the special cases, such as

// ElementData.rs for ElementAttributes
pub(crate) fn insert(&mut self, name: &str, value: &str) {
    // handle special attributes
    match name {
        "id" => {
            if let Some(old_id) = self.attributes.get("id") {
                self.element_data.node.document.named_id_elements.remove(old_id);
            }
            self.element_data.node.document.named_id_elements.insert(value);
        }
        "class" => {
            // something similar to above, which is not added to DOM yet for querying, but will soon
        }
        _ => {}
    }

    self.attributes.insert(name, value);
}

And then we can remove the set_named_id method from the Document. That way there's no possible confusion about how to "properly" modify an attribute and MUST be done via a node's attribute insert method.

However, here's where the design problem comes in and want some advice. To implement it like above, we'd be introducing a lot of shared pointers, and ideally I wouldn't want these optional, so you must provide a document when creating a node, and must provide a node when creating an ElementData. But this might complicate things and I'm not sure what some good alternatives are. One other alternative I can think of is having every substruct contain a pointer to the Document, at least that would get rid of the huge chain of self.element_data.node.document.

It's messy because ElementAttribute is deeply nested, however we would eventually need a reference to Document within node for some other things coming up anyways (specifically thinking of insert_before) and I think for some things Joshua was working on.

Anyways, long rambling aside, does anyone have a good idea how to handle this?

We could avoid this by traversing the tree to find the ID (and soon, class names) but I didn't really like that idea which is why I introduced the named_id_elements map in the first place.

[emwalker]

That way there's no possible confusion about how to "properly" modify an attribute and MUST be done via a node's attribute insert method.

I like this general approach. Don't use an Option<Something> if Something is actually required. Try to get the order of operations right to make that work.

One other alternative I can think of is having every substruct contain a pointer to the Document, at least that would get rid of the huge chain of self.element_data.node.document.

• Chromium keeps a reference to the document in its elements. E.g., HTMLDivElement accepts a Document that it appears to hold on to (1, 2, 3, 4, 5)
• Servo also keeps a reference to the parent document as well. E.g., HTMLDivElement (1, 2, 3)

Still figuring out how to navigate the Gecko code base. But ultimately I wouldn't be surprised if we end up needing a mutable ref-counted reference to the document in a field that is stored in nodes at some point. There might be a cleaner alternative available in Rust that I'm not aware of.

[Kiyoshika]

But ultimately I wouldn't be surprised if we end up needing a mutable ref-counted reference to the document in a field that is stored in nodes at some point.

While planning out some of my future changes, that's what I think is also needed. Some operations require context that a node by itself doesn't have (the example I keep giving is insert_before and similar.) And the other special case is what I've mentioned with the attribute and its id and class fields which have a special relationship with the DOM (to allow easy querying.)

I'm thinking maybe we require a document pointer whenever creating a node which is passed down to any of its dependencies that also require it (such as an HTML element, in this case.)

I have not had time yet to fully explore this but it's the first thing on my "to do" right now