Implement mutation observers

[dzbarsky]

I've started some patches for this.

[jdm]

cc @nox

[nox]

Colour me interested. Why do you think mutation observers are the way to go for DOM ranges?

[dzbarsky]

Note that these aren't the DOM mutation observers, just an internal servo thing that lets ranges register a callback for when the document changes. It's similar to nsIMutationObserver in Gecko.

[jdm]

That sounds confusingly named, in that case :)

[dzbarsky]

True it could use a different name. MutationWatcher or something.

[nox]

@dzbarsky Would #6660 help?

[dzbarsky]

I'm not sure how that PR helps here. There's not really a great way for nodes to know that they're the start or end of some range.

[nox]

I made Rc<RangeInner> in Range because I intended to store a Vec<Weak<RangeInner>> in each node. I guess that's the wrong way to go at it?

[dzbarsky]

That's a lot of storage. I think it's better to store that Vec on the document itself, that's basically what I'm doing in #6639

[nox]

With a Vec on the document itself, children_changed() could still be used in the VirtualMethods implementation of Node to update any range associated to the context object, or actually the insert methods and friends could do it themselves too, couldn't they?

Why the indirection with this MutationObserver thing?

[dzbarsky]

I can see children_changed being used for some of the notifications, but I'm not sure how you'd handle some other cases. For example, there are steps to run when calling CharacterData#replaceData, or Node#normalize.
But yes, I think it would be cleaner to just do the steps directly in insert and remove methods.

As for the MutationObserver thing...
In Gecko, other things than just range hook into nsIMutationObserver but perhaps we don't care about those for now.

One thing to keep in mind is that NodeIterator also defines "removing steps" (https://dom.spec.whatwg.org/#nodeiterator).
So we would also need to store a Vec of NodeIterators. I was hoping to combine those but it looks like they need different notifications so it doesn't really make things any cleaner.

Having played around with it some I think the MutationObserver abstraction isn't necessary yet and just makes things more complicated for now so I'm tempted to drop it.

[nox]

I'm on the Range part.

[zbraniecki]

This depends on #4283

[jdm]

#1980 also covers this. We should close one of them.

[krishnakarthick1993]

Question regarding test cases : We have completed the initial steps and we are now moving to the subsequent steps.
I would like to know if we need any new test cases to be added to test our subsequent steps (or) is the previous test cases present in tests/wpt/web-platform-tests/dom/nodes good enough. Please let us know. Thanks.

[jdm]

I expect that the existing tests should provide enough coverage; my only worry is if they also require features that your project will not implement. It would be useful to create a simple test case that only uses the features that you are implementing in your project; it can live in tests/wpt/mozilla/tests/mozilla/.

[srivassumit]

Hi,

I am implementing the subsequent steps of the Mutation Observer API as described here: https://github.com/servo/servo/wiki/Mutation-observer-project

I am stuck at the step 3 of the notify mutation observers algorithm. I am not sure how to get the "copy of unit of related similar-origin browsing contexts' signal slot list. "

This is what I have implemented in the impl MicrotaskQueue of microtask.rs file so far:

    /// https://dom.spec.whatwg.org/#notify-mutation-observers
    /// Notify Mutation Observers.
    pub fn notifyMutationObservers() {
    	SCRIPT_THREAD_ROOT.with(|root| {
            let script_thread = unsafe { &*root.get().unwrap() };
            // Step 1
            script_thread.mutation_observer_compound_microtask_queued.borrow_mut() = false;
            // Step 2
            let notifyList = script_thread.mutation_observers.borrow_mut();
            // Step 3
            let signalList = ""; // Stuck;
            // Step 4
            // ??
        });
    	
    }

Can you please point us in the right direction?

Thanks.

[jdm]

Servo doesn't implement anything related to slots, so you can leave // TODO: step 3 (signal slot list) there.

[srivassumit]

While implementing the MutationObserver.observe method, In steps 3-6 it says that we need to throw a TypeError, but I could not find any specification for 'TypeError'.
I found 'TypeError' referenced in addEventListener and removeEventListener methods of EventTarget specification, but in the eventtarget.rs file I could not find and TypeError being thrown.
can you please suggest which error needs to be thrown here and/or some file where it is already thrown so that I can use that as a reference?

[jdm]

Unfortunately we don't implement step 1 of those APIs in Servo yet, or that would have been a good idea! Throwing exceptions occurs automatically when returning an Err value from DOM APIs, but also requires that the WebIDL method be marked [Throw]. The result looks like this: https://dxr.mozilla.org/servo/rev/7f825d2119a480a64b103e1d60a2d469af98d3de/components/script/dom/crypto.rs#53

[gosavipooja]

Hi,

I am trying to implement the subsequent steps of the Mutation Observer API as described here: https://github.com/servo/servo/wiki/Mutation-observer-project.

I am a bit confused regarding - make changing/appending/removing/replacing an attribute queue a mutation record via Attr::set_value, Element::push_attribute, and Element::remove_first_matching_attribute.

In order to change or append or remove or replace an attribute, I am unsure if I need to modify the existing methods like setAttribute or removeAttribute as seen in the element.rs file? Or I need to implement any such related methods in some other file?

Can you please help me? Thanks!

[jdm]

The algorithms at https://dom.spec.whatwg.org/#concept-element-attributes-change all involve queuing a mutation record as the first step before actually modifying the element's attribute. You'll want to modify the existing methods to add that behaviour, since they are the fundamental building blocks that other APIs like setAttribute and removeAttribute are built upon. Does that make sense?

[KiChjang]

@srivassumit Oh, tough luck. SCRIPT_THREAD_ROOT contains a *const ScriptThread pointer, so you definitely cannot borrow it as mutable. We will need to use std::cell::Cell to represent mutation_observer_compound_microtask_queued.

In other words, the field type of mutation_observer_compound_microtask_queued should be changed from being just a bool to a Cell<bool> instead. Then you will be able to do the following:

pub fn set_mutation_observer_compound_microtask_queued(value: bool) {
    SCRIPT_THREAD_ROOT.with(|root| {
        let script_thread = unsafe { &*root.get().unwrap() };
        script_thread.mutation_observer_compound_microtask_queued.set(value);
    })
}

[KiChjang]

@srivassumit For your observe method, I'm actually quite surprised that you need to mutate the MutationObserverInit parameter. Could you show more context in what you're trying to do?

[srivassumit]

@KiChjang For the Observe Method this is my code:

/// https://dom.spec.whatwg.org/#dom-mutationobserver-observe
    /// MutationObserver.observe method
    fn Observe(&self, target: &Node, options: &MutationObserverInit) -> Fallible<()> {
        // Step 1: If either options’ attributeOldValue or attributeFilter is present and
        // options’ attributes is omitted, set options’ attributes to true.
        if (options.attributeOldValue.is_some() || options.attributeFilter.is_some()) && options.attributes.is_none() {
            options.borrow_mut().attributes = Some(true);
        }
        // Step2: If options’ characterDataOldValue is present and options’ characterData is omitted,
        // set options’ characterData to true.
        if options.characterDataOldValue.is_some() && options.characterData.is_some() {
            options.borrow_mut().characterData = Some(true);
        }
        // Step3: If none of options’ childList, attributes, and characterData is true, throw a TypeError.
        if !options.childList && !options.attributes.unwrap() && !options.characterData.unwrap() {
            return Err(Error::Type("childList, attributes, and characterData not true".to_owned()));
        }
        // Step4: If options’ attributeOldValue is true and options’ attributes is false, throw a TypeError.
        if options.attributeOldValue.unwrap() && !options.attributes.unwrap() {
            return Err(Error::Type("attributeOldValue is true but attributes is false".to_owned()));
        }
        // Step5: If options’ attributeFilter is present and options’ attributes is false, throw a TypeError.
        if options.attributeFilter.is_some() && !options.attributes.unwrap() {
            return Err(Error::Type("attributeFilter is present but attributes is false".to_owned()));
        }
        // Step6: If options’ characterDataOldValue is true and options’ characterData is false, throw a TypeError.
        if options.characterDataOldValue.unwrap() && !options.characterData.unwrap() {
            return Err(Error::Type("characterDataOldValue is true but characterData is false".to_owned()));
        }
        // TODO: Step 7
        // TODO: Step 8
        Ok(())
    }

the same error is there for both of the following lines:
options.borrow_mut().attributes = Some(true);
and
options.borrow_mut().characterData = Some(true);

I was trying to set the value for attributes using this first:
options.attributes = Some(true)
but it gave error "cannot mutably borrow immutable field" so I tried using borrow_mut() instead.

[KiChjang]

Unfortunately, you cannot mutate the value of options at all. The best you can do here is to create a new MutationObserverInit struct and have it inherit all the field members of options. As for step 1 and 2, you may have to create let bindings to store those variables that the spec says to set.

[gosavipooja]

Hi,

In the step 7 of MutationObserver's observe,

For each registered observer registered in target’s list of registered observers whose observer is the context object:
Remove all transient registered observers whose source is registered.
Replace registered’s options with options.

So, here, the list of registered observers will be from the node.rs:
mutation_observers: DOMRefCell<Vec<JS<MutationObserver>>>,

How do I determine the transient registered observer. As per my understanding, I need to check if the source of the 'registered observer' exists in this vector. But how do I obtain the source? Correct me if I am wrong.

Also, the following statement confuses me -

registered observers whose observer is the context object

How do I obtain a registered observer's observer?
Thanks!

[jdm]

The link in the spec for registered observer points out that a registered observer's observer is just the MutationObserver object, so you don't need to do anything special besides this comparison: &*registered as *const MutationObserver == self as *const MutationObserver.

As for the transient observers, just leave a TODO like // TODO: remove matching transient registered observers. They require additional handling that is not relevant to this project's work right now.

[gosavipooja]

Hi,

I have some doubts in the usage of traits. I have encountered issues related to it at 2 places -

error: expected expression, found keyword `as`
     let name = Atom<<servo_atoms::AtomStaticSet> as Trait>::From::from(attr.local_name().to_string());

and in this as well :

for registered in &target.registered_mutation_observers_for_type() {
           if &*registered as *const MutationObserver == self as *const MutationObserver{
          //TODO: 
         }
  the trait core::iter::Iterator is not implemented for &&dom::bindings::cell::DOMRefCell<std::vec::Vec<dom::bindings::js::JS<dom::mutationobserver::MutationObserver>>>

Can you point me in the right direction w.r.t traits?

Thanks!

[jdm]

For the second, you want:
for registered in target.registered_mutation_observers_for_type().borrow().iter(), since you can't iterate over the DOMRefCell that encapsulates the actual vector. For the first, I think you might have been misled by some error messages? Try Atom::from(attr.local_name().to_string()) instead.

[krishnakarthick1993]

Hello Josh, I am trying to iterate over node’s list of registered observers ( for Step 3 of Queuing a mutation record) .

The following is my code:

pub fn queue_a_mutation_record(target: &Node,attr_type: Mutation) {
    //https://dom.spec.whatwg.org/#queueing-a-mutation-record
    // Step 1 :Let interested observers be an initially empty set of MutationObserver objects optionally paired with a string
    let mut interestedObservers: Vec<Root<MutationObserver>> = vec![];
    let mut pairedStrings: Vec<DOMString> = vec![];
    // Step 2: Let nodes be the inclusive ancestors of target.
    let mut nodes: Vec<Root<Node>> = vec![];
    for ancestor in target.inclusive_ancestors() {
        nodes.push(ancestor);
    }
    // Step 3: For each node in nodes
    for node in &nodes {
        for registered_observer in node.registered_mutation_observers_for_type().borrow_mut().iter() {
        }
    }
}

I tried using the above approach by writing for registered in node.registered_mutation_observers_for_type().borrow().iter() like for registered in target.registered_mutation_observers_for_type().borrow().iter()

On compilation, I get the following error

error: cannot borrow immutable borrowed content as mutable
  --> /home/kbalaji/Moz/servo/components/script/dom/mutationobserver.rs:45:36
   |
45 |         for registered_observer in node.registered_mutation_observers_for_type().borrow_mut().iter() {
   |                                    ^^^^ cannot borrow as mutable ( **node is highlighted** for cannot borrow as mutable error)

Even on replacing node with target ( just to check if target works) by replacing line for registered_observer in node.registered_mutation_observers_for_type().borrow_mut().iter() { with
for registered_observer in target.registered_mutation_observers_for_type().borrow_mut().iter() {

Compiler shows the following error:

error: cannot borrow immutable borrowed content `*target` as mutable
  --> /home/kbalaji/Moz/servo/components/script/dom/mutationobserver.rs:45:36
   |
33 | pub fn queue_a_mutation_record(target: &Node,attr_type: Mutation) {
   |                                        ----- use `&mut Node` here to make mutable
...
45 |         for registered_observer in target.registered_mutation_observers_for_type().borrow_mut().iter() {
   |                                    ^^^^^^ cannot borrow as mutable

Please let us know on how to approach this problem. Thank you.

[jdm]

You wrote borrow_mut(), not borrow().

[jdm]

Although it sounds like maybe your registered_mutation_observers_for_type() method is &mut self instead of &self? It's hard to be sure without being able to see the actual code you have.

[gosavipooja]

Hi,

Is there any way to access the registered's options (MutationObserverInit) via the MutationObserver object? I thought I will need it for this step -

Replace registered’s options with options.

Thanks!

[jdm]

I recommend adding members to MutationObserver that you initialize to the values present in the MutationObserverInit argument in the constructor.

[jdm]

(they will need to be wrapped in Cell or DOMRefCell types to allow mutating them later)

[gosavipooja]

Hi,

There is no MutationObserverInit argument in the constructor currently and if we add it, it shoots an error in the Bindings file:

error[E0061]: this function takes 3 parameters but 2 parameters were supplied
    --> servo/target/debug/build/script-9cf1e34119ea7ecd/out/Bindings/MutationObserverBinding.rs:1002:91
     |
1002 |           let result: Result<Root<MutationObserver>, Error> = MutationObserver::Constructor(&global, arg0);
     |                                                                                             ^^^^^^^^^^^^^ expected 3 parameters
     | 
    ::: servo/components/script/dom/mutationobserver.rs
     |
53   | /     pub fn Constructor(global: &Window, callback: Rc<MutationCallback>, options: DOMRefCell<MutationObserverInit>) -> Fallible<Root<MutationObserver>> {
54   | |         let observer = MutationObserver::new(global, callback);
55   | |         ScriptThread::add_mutation_observer(&*observer);
56   | |         Ok(observer)
57   | |     }
     | |___- defined here

Should we create another overridden constructor which also has MutationObserverInit argument? Or what way should we follow?
Thanks!

[krishnakarthick1993]

Hi Josh,
I am trying to implement Step 4.2 of Queuing a mutation record (If name and namespace are given, set record’s attributeName to name, and record’s attributeNamespace to namespace.)

My enum is

pub enum Mutation {
    Attribute { name: Atom, namespace: Namespace, oldValue: DOMString, newValue: DOMString}
}

Since I need name and namespace of Attribute of Mutation enum which is passed as an argument to my function , I tried using the match function like (http://rustbyexample.com/custom_types/enum.html)

My function is

    //Queuing a mutation record
    pub fn queue_a_mutation_record(target: &Node, attr_type: Mutation) {
        // Step 1
        let mut interestedObservers: Vec<Root<MutationObserver>> = vec![];
        let mut pairedStrings: Vec<DOMString> = vec![];
        // Step 2
        let mut nodes: Vec<Root<Node>> = vec![];
        for ancestor in target.inclusive_ancestors() {
            nodes.push(ancestor);
        }
        // Step 3
        for node in &nodes {
            for registered_observer in node.registered_mutation_observers_for_type().borrow().iter() {
               //let bool condition1 = (node!=target);
            }
        }
        // Step 4
        for observer in &interestedObservers {
            //Step 4.1
            let mut record= MutationRecord::new(target);
            //Step 4.2
            match attr_type {
                 Mutation::Attribute {name,namespace,oldValue,newValue} => { let mut given_name = name;let mut given_namespace = namespace;
               },
            }

        }
        }

On compilation I get

error[E0382]: use of partially moved value: `attr_type`
   --> /home/kbalaji/Moz/servo/components/script/dom/mutationobserver.rs:103:19
    |
103 |             match attr_type {
    |                   ^^^^^^^^^ value used here after move
104 |                Mutation::Attribute {name,namespace,oldValue,newValue} => { let mut given_name = name;let mut given_namespace = namespace;
    |                                     ---- value moved here
    |
    = note: move occurs because `attr_type.name` has type `string_cache::atom::Atom<servo_atoms::AtomStaticSet>`, which does not implement the `Copy` trait

error[E0382]: use of moved value: `attr_type.name`
   --> /home/kbalaji/Moz/servo/components/script/dom/mutationobserver.rs:104:37
    |
104 |                Mutation::Attribute {name,namespace,oldValue,newValue} => { let mut given_name = name;let mut given_namespace = namespace;
    |                                     ^^^^ value moved here in previous iteration of loop
    |
    = note: move occurs because `attr_type.name` has type `string_cache::atom::Atom<servo_atoms::AtomStaticSet>`, which does not implement the `Copy` trait

(similar errors for namespace,oldValue and newValue).

I tried including #[derive(Clone,Copy)] before my mutation and the following code

impl Copy for Mutation {}
impl Clone for Mutation { fn clone(&self) -> Self { *self } }

Compiler still throws

error[E0119]: conflicting implementations of trait `core::marker::Copy` for type `dom::mutationobserver::Mutation`:
  --> /home/kbalaji/Moz/servo/components/script/dom/mutationobserver.rs:35:1
   |
31 | #[derive(Clone,Copy)]
   |                ---- first implementation here
...
35 | impl Copy for Mutation {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `dom::mutationobserver::Mutation`

error[E0204]: the trait `Copy` may not be implemented for this type
  --> /home/kbalaji/Moz/servo/components/script/dom/mutationobserver.rs:31:16
   |
31 | #[derive(Clone,Copy)]
   |                ^^^^
32 | pub enum Mutation {
33 |     Attribute { name: Atom, namespace: Namespace, oldValue: DOMString, newValue: DOMString}
   |                 ---------- this field does not implement `Copy`

error[E0204]: the trait `Copy` may not be implemented for this type
  --> /home/kbalaji/Moz/servo/components/script/dom/mutationobserver.rs:35:6
   |
33 |     Attribute { name: Atom, namespace: Namespace, oldValue: DOMString, newValue: DOMString}
   |                 ---------- this field does not implement `Copy`
34 | }
35 | impl Copy for Mutation {}
   |      ^^^^

error[E0119]: conflicting implementations of trait `core::clone::Clone` for type `dom::mutationobserver::Mutation`:
  --> /home/kbalaji/Moz/servo/components/script/dom/mutationobserver.rs:36:1
   |
31 | #[derive(Clone,Copy)]
   |          ----- first implementation here
...
36 | impl Clone for Mutation { fn clone(&self) -> Self { *self } }
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `dom::mutationobserver::Mutation`

Please let us know on how to reference name and namespace of attr_type ( function argument).
Thank you.

[KiChjang]

I'm very confused why you have

pub enum Mutation {
    Attribute { name: Atom, namespace: Namespace, oldValue: DOMString, newValue: DOMString}
}

Can Attribute not be a struct of its own?

[krishnakarthick1993]

Hi,
Separately, I am not clear on what "type" means in "To queue a mutation record of type for target with one or more of (depends on type) ....." .

In Step 3 of Queuing a mutation record , type is supposed to be compared with "attributes", "characterData" and "childList".
Does type refer to record_type of a mutation record object ?. If so , should I pass a mutation record object as well as an argument to my function queue_a_mutation_record along with target and enum. This would enable me to access the type of mutation record through that object.
Please let us know. Thank you.

[jdm]

The "type" in the spec is equivalent to matching against the enum that we're using. To avoid the errors about partially moved values, you will want to use ref binding in the matches, and clone those values like let mut given_name = name.clone().

[jdm]

As for the question about MutationObserverInit, my mistake. I guess you will want sensible defaults for the new members in the constructor, and then set them to the real values in MutationObserver::Observe.

[Xanewok]

@jdm just want to make sure what's the status of this issue - it seems that effectively almost all of the listed work in https://github.com/servo/servo/wiki/Mutation-observer-project is done (I tried to go over them and mark done items, excuse the crude format), mostly by #16883?

It seems that what's left is:

• to implement void disconnect(), sequence<MutationRecord> takeRecords()
• remove transient observers (here and here)
• implement signal slots?

Asking because we planned to take this on as a team as a part of our university thesis project
cc @mrowqa @marmistrz

[marmistrz]

cc @taki-jaro

[jdm]

@Xanewok Signal slots are part of a standard that we don't implement, so those can be ignored. We're also missing character data notifications, and all of the failing tests that are named MutationObserver.

[jdm]

We have implemented mutation observers. If anything is missing or not working, we should open new issues for those items.