script: Implement `ToggleEvent` and use for `<details>` element #40271

lukewarlow · 2025-10-29T20:37:20Z

Implement ToggleEvent and use for details element

Testing: Covered by existing WPTs

mrobinson

Looks good. Just a few nits:

components/script/dom/toggleevent.rs

components/script_bindings/webidls/ToggleEvent.webidl

lukewarlow · 2025-10-30T13:46:20Z

I've pushed those changes up, I squashed because of my WebKit brain, is there a preference to squash or push up seperate commites to address comments for servo?

mrobinson · 2025-10-30T14:30:28Z

There's no preference. Squashing is fine. I usually squash, but now and then I leave them as separate commits if the original PR is very large and I think it will possibly make a followup review easier.

jdm · 2025-10-31T05:39:10Z

   ▶ TIMEOUT [expected OK] /html/semantics/interactive-elements/the-details-element/name-attribute.html
  │   ▶ TIMEOUT [expected FAIL] handling of insertion of elements into group
  │   │   → Test timed out
  └   └ NOTRUN [expected FAIL] basic handling of mutually exclusive details when the element isn't connected

Also need to update

servo/tests/wpt/mozilla/tests/mozilla/interfaces.https.html

Line 13 in 9aa6842

test_interfaces([

with the new ToggleEvent interface name.

lukewarlow · 2025-10-31T08:28:05Z

It seems odd that this changes the results of that exclusivity test, I'll take a look, it's possible it previously failed because of the incorrect event type and now it's getting further.

simonwuelker · 2025-10-31T11:16:27Z

I don't think we support mutually exclusive details elements yet.

TimvdLippe

Some nits, mostly curious why parts of the spec aren't implemented. Not sure about the status of this PR though, given that it was scheduled for merge before. So feel free to ignore these for now and fix them in a follow-up PR.

components/script/dom/html/htmldetailselement.rs

components/script/dom/toggleevent.rs

TimvdLippe · 2025-11-02T09:18:44Z

components/script/dom/toggleevent.rs

+        self.source.as_ref()?;
+
+        if let Some(current_target) = self.event.GetCurrentTarget() {
+            return self.source.as_ref().and_then(|source| {
+                let retargeted = source.upcast::<EventTarget>().retarget(&current_target);
+                retargeted.downcast::<Element>().map(DomRoot::from_ref)
+            });
+        }
+
+        self.source.as_ref().and_then(|source| {
+            let document = source.upcast::<Node>().GetOwnerDocument().unwrap();
+            let retargeted = source
+                .upcast::<EventTarget>()
+                .retarget(document.upcast::<EventTarget>());
+            retargeted.downcast::<Element>().map(DomRoot::from_ref)
+        })


The spec doesn't state that we should use the owner document if currentTarget is null. Should we instead return None when that's the case? E.g. something like

Suggested change

self.source.as_ref()?;

if let Some(current_target) = self.event.GetCurrentTarget() {

return self.source.as_ref().and_then(|source| {

let retargeted = source.upcast::<EventTarget>().retarget(&current_target);

retargeted.downcast::<Element>().map(DomRoot::from_ref)

});

}

self.source.as_ref().and_then(|source| {

let document = source.upcast::<Node>().GetOwnerDocument().unwrap();

let retargeted = source

.upcast::<EventTarget>()

.retarget(document.upcast::<EventTarget>());

retargeted.downcast::<Element>().map(DomRoot::from_ref)

})

let source = self.source.as_ref()?;

let current_target = self.event.GetCurrentTarget()?;

let retargeted = source.upcast::<EventTarget>().retarget(&current_target);

retargeted.downcast::<Element>().map(DomRoot::from_ref)

This is based on the WebKit implementation for command event which uses the same spec language and that in turn was based on I think chromium behaviour. I'll double check if this is erroneous or not.

components/script/dom/toggleevent.rs

lukewarlow · 2025-11-02T16:20:02Z

Ah yeah that test uses oldState and newState so was failing and is now timing out. I'll update the expectations file now.

TimvdLippe · 2025-11-02T16:27:27Z

You also need to update

servo/tests/wpt/mozilla/tests/mozilla/interfaces.https.html

Line 13 in 9aa6842

test_interfaces([

per the above comment

Signed-off-by: Luke Warlow <lwarlow@igalia.com>

TimvdLippe

Merging as-is and feedback will be addressed in a follow-up

TimvdLippe · 2025-11-02T16:45:53Z

components/script/dom/toggleevent.rs

+        let document = source.upcast::<Node>().GetOwnerDocument().unwrap();
+        let retargeted = source
+            .upcast::<EventTarget>()
+            .retarget(document.upcast::<EventTarget>());
+        retargeted.downcast::<Element>().map(DomRoot::from_ref)


Can you file a spec bug that this should be updated in the spec given all major browsers implement it this way?

Yeah I'll double check what the state of things is and file an appropriate issue.

TimvdLippe · 2025-11-02T17:38:21Z

Tests with unexpected results:
  ▶ TIMEOUT [expected OK] /html/semantics/interactive-elements/the-details-element/name-attribute.html

Hm I guess this is intermittent flaky :/ Will file an issue

lukewarlow · 2025-11-02T18:14:38Z

Hopefully the pr to implement that behaviour as a follow up will resolve it quite quickly.

…ee (#40314) Within the same tree, only one `<details>` element with the same name may be open at a time. Before this change, this invariant was not enforced. I've added a `HashMap` to `Document` and `ShadowRoot` which maps from a name to the a list of details elements with the same name. This map allows us to find conflicting details elements without having to traverse the whole tree. Of course this only works when the tree is a document tree or a shadow tree, so we still have to fall back to `traverse_preorder` in some cases (which I believe to be uncommon). This is ready for review, but I'd like to wait until #40271 is merged to not cause unnecessary merge conflicts. Testing: New web platform tests start to pass --------- Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>

lukewarlow requested a review from gterzian as a code owner October 29, 2025 20:37

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 29, 2025

lukewarlow force-pushed the details-toggleevent branch from a6c38dd to 3bae3e2 Compare October 29, 2025 20:47

mrobinson changed the title ~~Implement ToggleEvent and use for details element~~ script: Implement ToggleEvent and use for <details> element Oct 29, 2025

mrobinson approved these changes Oct 29, 2025

View reviewed changes

components/script/dom/toggleevent.rs Outdated Show resolved Hide resolved

components/script/dom/toggleevent.rs Outdated Show resolved Hide resolved

components/script_bindings/webidls/ToggleEvent.webidl Outdated Show resolved Hide resolved

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 29, 2025

lukewarlow force-pushed the details-toggleevent branch from 3bae3e2 to da37e7d Compare October 30, 2025 13:44

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 30, 2025

lukewarlow force-pushed the details-toggleevent branch from da37e7d to c3234b0 Compare October 30, 2025 13:45

mrobinson approved these changes Oct 30, 2025

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 30, 2025

mrobinson added this pull request to the merge queue Oct 30, 2025

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 30, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 30, 2025

servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 30, 2025

mukilan added this pull request to the merge queue Oct 31, 2025

servo-highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 31, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 31, 2025

servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 31, 2025

simonwuelker mentioned this pull request Oct 31, 2025

script: Enforce exclusivity between <details> elements in the same tree #40314

Merged

TimvdLippe requested changes Nov 2, 2025

View reviewed changes

servo-highfive added the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Nov 2, 2025

lukewarlow force-pushed the details-toggleevent branch from c3234b0 to fe343c4 Compare November 2, 2025 16:20

servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 2, 2025

lukewarlow force-pushed the details-toggleevent branch from fe343c4 to 1735b78 Compare November 2, 2025 16:29

Implement ToggleEvent and use for details element

715b01b

Signed-off-by: Luke Warlow <lwarlow@igalia.com>

lukewarlow force-pushed the details-toggleevent branch from 1735b78 to 715b01b Compare November 2, 2025 16:37

TimvdLippe approved these changes Nov 2, 2025

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 2, 2025

TimvdLippe added this pull request to the merge queue Nov 2, 2025

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 2, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 2, 2025

servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 2, 2025

TimvdLippe added this pull request to the merge queue Nov 2, 2025

servo-highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 2, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 2, 2025

servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Nov 2, 2025

TimvdLippe added this pull request to the merge queue Nov 2, 2025

servo-highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 2, 2025

Merged via the queue into servo:main with commit 8ca4ecb Nov 2, 2025
44 checks passed

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 2, 2025

Uh oh!

script: Implement ToggleEvent and use for <details> element #40271

script: Implement ToggleEvent and use for <details> element #40271

Uh oh!

Conversation

lukewarlow commented Oct 29, 2025

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukewarlow commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrobinson commented Oct 30, 2025

Uh oh!

Uh oh!

Uh oh!

jdm commented Oct 31, 2025

Uh oh!

lukewarlow commented Oct 31, 2025

Uh oh!

simonwuelker commented Oct 31, 2025

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TimvdLippe Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

lukewarlow Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukewarlow commented Nov 2, 2025

Uh oh!

TimvdLippe commented Nov 2, 2025

Uh oh!

TimvdLippe left a comment

Choose a reason for hiding this comment

Uh oh!

TimvdLippe Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

lukewarlow Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TimvdLippe commented Nov 2, 2025

Uh oh!

Uh oh!

lukewarlow commented Nov 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

script: Implement `ToggleEvent` and use for `<details>` element #40271

script: Implement `ToggleEvent` and use for `<details>` element #40271

lukewarlow commented Oct 30, 2025 •

edited

Loading