Repository: servo/servo
File: components/script/dom/html/htmldetailselement.rs (Rust)

Code before:

impl HTMLDetailsElementMethods<crate::DomTypeHolder> for HTMLDetailsElement {
    // https://html.spec.whatwg.org/multipage/#dom-details-open
    make_bool_getter!(Open, "open");

    // https://html.spec.whatwg.org/multipage/#dom-details-open
    make_bool_setter!(SetOpen, "open");
}

impl VirtualMethods for HTMLDetailsElement {
    fn super_type(&self) -> Option<&dyn VirtualMethods> {
        Some(self.upcast::<HTMLElement>() as &dyn VirtualMethods)
    }

    fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation, can_gc: CanGc) {
        self.super_type()
            .unwrap()
            .attribute_mutated(attr, mutation, can_gc);

        if attr.local_name() == &local_name!("open") {
            self.update_shadow_tree_styles(can_gc);

            let counter = self.toggle_counter.get() + 1;
            self.toggle_counter.set(counter);
            let old_state = if self.Open() { "closed" } else { "open" };
            let new_state = if self.Open() { "open" } else { "closed" };

            let this = Trusted::new(self);
            self.owner_global()
                .task_manager()
                .dom_manipulation_task_source()
                .queue(task!(details_notification_task_steps: move || {
                    let this = this.root();
                    if counter == this.toggle_counter.get() {
                        let event = ToggleEvent::new(
                            this.global().as_window(),
                            atom!("toggle"),
                            EventBubbles::DoesNotBubble,
                            EventCancelable::NotCancelable,
                            DOMString::from(old_state),
                            DOMString::from(new_state),
                            None,
                            CanGc::note(),
                        );
                        let event = event.upcast::<Event>();
                        event.fire(this.upcast::<EventTarget>(), CanGc::note());
                    }
                }));
            self.upcast::<Node>().dirty(NodeDamage::Other);
        }
    }

Review comment (suggestion):
Nit to only call `self.Open()` once:

```suggestion
            let (old_state, new_state) = if self.Open() { ("closed", "open" ) } else { ("open", "closed") };
```

In general, I am curious why this doesn't implement https://html.spec.whatwg.org/multipage/interactive-elements.html#details-toggle-task-tracker ? It appears to be only used in https://html.spec.whatwg.org/multipage/interactive-elements.html#queue-a-details-toggle-event-task which appears to me takes care of changing the details value while handling the `toggle` event.

Code after:

impl HTMLDetailsElementMethods<crate::DomTypeHolder> for HTMLDetailsElement {
    // https://html.spec.whatwg.org/multipage/#dom-details-open
    make_bool_getter!(Open, "open");

    // https://html.spec.whatwg.org/multipage/#dom-details-open
    make_bool_setter!(SetOpen, "open");
}

impl VirtualMethods for HTMLDetailsElement {
    fn super_type(&self) -> Option<&dyn VirtualMethods> {
        Some(self.upcast::<HTMLElement>() as &dyn VirtualMethods)
    }

    fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation, can_gc: CanGc) {
        self.super_type()
            .unwrap()
            .attribute_mutated(attr, mutation, can_gc);

        if attr.local_name() == &local_name!("open") {
            self.update_shadow_tree_styles(can_gc);

            let counter = self.toggle_counter.get() + 1;
            self.toggle_counter.set(counter);
            let (old_state, new_state) = if self.Open() {
                ("closed", "open")
            } else {
                ("open", "closed")
            };

            let this = Trusted::new(self);
            self.owner_global()
                .task_manager()
                .dom_manipulation_task_source()
                .queue(task!(details_notification_task_steps: move || {
                    let this = this.root();
                    if counter == this.toggle_counter.get() {
                        let event = ToggleEvent::new(
                            this.global().as_window(),
                            atom!("toggle"),
                            EventBubbles::DoesNotBubble,
                            EventCancelable::NotCancelable,
                            DOMString::from(old_state),
                            DOMString::from(new_state),
                            None,
                            CanGc::note(),
                        );
                        let event = event.upcast::<Event>();
                        event.fire(this.upcast::<EventTarget>(), CanGc::note());
                    }
                }));