Use ToggleEvent for <details> element toggle event

[nt1m]

• At least two implementers are interested (and none opposed):
  • WebKit
  • Chrome
• Tests are written and can be reviewed and commented upon at:
  • WebKit export of https://bugs.webkit.org/show_bug.cgi?id=252373 web-platform-tests/wpt#41549
• Implementation bugs are filed:
  • Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1416738
  • Gecko: https://bugzilla.mozilla.org/show_bug.cgi?id=1817122
  • WebKit: https://bugs.webkit.org/show_bug.cgi?id=252373
  • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
  • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
• MDN issue is filed: …

(See WHATWG Working Mode: Changes for more details.)

---

/index.html ( diff )
/indices.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )
/popover.html ( diff )

[domenic]

Normative text LGTM, just some editorial nits (which I'm happy to take care of before merging).

Should we move https://html.spec.whatwg.org/#the-toggleevent-interface somewhere more shared? No obvious candidates to me, but maybe https://html.spec.whatwg.org/#activation ? Thoughts appreciated.

[mfreed7]

Thanks for putting this PR up!

[domenic]

Thanks for the work here! I found a few nits, several of which I see are also worth fixing for the popover text.

We also need to resolve #8893 (review) , i.e. move the ToggleEvent definition to a shared location.

Additionally, https://html.spec.whatwg.org/#the-toggleevent-interface has some popover-specific stuff. I think we can just remove most of it, and end up with

The oldState and newState attributes must return the values they are initialized to.

[sideshowbarker]

Question: When the details element open attribute is added or changed, should the “DOM manipulation task source” be used for firing the event, or should the “user interaction task source” be used?

The existing spec text for popover uses the “user interaction task source” for that event — but in the patch in this PR branch as currently written, I have the details case using the “DOM manipulation task source”.

[sideshowbarker]

Assuming we should be using the “user interaction task source” for both the details case and the popover case, it seems like we could have a shared “Queue a toggle event task” algorithm — rather than separate “Queue a details toggle event task” and “Queue a popover toggle event task” algorithms.

Or even if the two cases used a different task source, we could still have s shared “Queue a toggle event task” algorithm but just with the task source as an additional input parameter to the algorithm.

But of course it’s simpler if we can just make the same task source be used in both cases.

[domenic]

Yeah, I noticed this divergence. As part of this change let's get implementers to sign off on aligning the task sources used. In practice task sources are very hard to observe so this should be fine.

Actually it looks like both Chromium and WebKit already use DOM manipulation for popover. So this is just a fix where the current spec is bad:

• WebKit
• Chromium

And I think using DOM manipulation is best anyway, since the flow is generally something happens in DOM -> fire the event, which is sometimes derived from user interaction -> something happens in DOM but sometimes not.

[sideshowbarker]

OK, I’ve pushed a change that moves both details and popover to using a shared “Queue a toggle event task” algorithm — with DOM manipulation as the task source.

[domenic]

This is great! However, the unification has revealed a bit of a collision. The popover section defines that every element has a toggle task tracker. And the details section defines that each details element has a toggle task tracker. Are they supposed to be the same, or separate? What is everyone implementing?

If they are supposed to be the same, then:

• We should move the definition of every element's toggle task tracker to the toggle event section
• We should link to that field definition from all the relevant places
• We should be sure there are WPTs for what happens when you do both kinds of "toggles" on a <details popover>. The result will be strange, I think, but maybe the idea is you deserve strangeness for using such a strange pattern.

If they are supposed to be different, then:

• We should separately define "popover toggle task tracker" fields on every element in the popover section, and "details toggle task tracker" fields on details elements only.
• We need to find some way of having the different algorithm calls operate on different fields. This is a balance between avoiding duplicate code and keeping things readable. Some ideas:
  • "Queue a toggle event task" takes getTracker and setTracker algorithms.
    • Con: call sites are kind of verbose. Queue a toggle event task given element, "open", "closed", steps that return element's details toggle task tracker, and steps given newValue that set element's details toggle task tracker to newValue.
    • Potential mitigation: small wrapper algorithms?
  • "Queue a toggle event task" takes a boolean useDetailsTracker, and adds appropriate branches depending on its value. (E.g. "If useDetailsTracker is true, set element's [details toggle task tracker] to null; otherwise, set element's [popover toggle task tracker] to null.")
  • Just split the algorithms and try to remember to keep them in sync.

[sideshowbarker]

This is great! However, the unification has revealed a bit of a collision. The popover section defines that every element has a toggle task tracker. And the details section defines that each details element has a toggle task tracker.

aha yeah

Are they supposed to be the same, or separate? What is everyone implementing?

I haven’t looked at the Blink implementation, but in the WebKit implementation, they are separate.

…
If they are supposed to be different, then:

• We should separately define "popover toggle task tracker" fields on every element in the popover section, and "details toggle task tracker" fields on details elements only.

I think that’s what we have now — minus my 196bd83 commit (which I’ve now backed out).

…

• Just split the algorithms and try to remember to keep them in sync.

That seems like the most practical option. And I think that’s what the current state of this PR branch now has.

They’re relatively short and simple algorithms, so keeping them in sync shouldn’t be that difficult.

[sideshowbarker]

And I think using DOM manipulation is best anyway, since the flow is generally something happens in DOM -> fire the event, which is sometimes derived from user interaction -> something happens in DOM but sometimes not.

OK — the algorithm for details in this branch uses DOM manipulation. Changing the corresponding algorithm for popover should maybe be handled in a separate PR?

[domenic]

Changing the corresponding algorithm for popover should maybe be handled in a separate PR?

I'm OK either way, but yeah, a separate PR is probably a bit cleaner.

[sideshowbarker]

OK, I’ve pushed an update that defines “details toggle task tracker” and “popover toggle task tracker” as each being a separate “toggle task tracker”.

[sideshowbarker]

Changing the corresponding algorithm for popover should maybe be handled in a separate PR?

I'm OK either way, but yeah, a separate PR is probably a bit cleaner.

OK, I’ve opened opened #9630 for that

[domenic]

LGTM. It would be ideal if we could get tests for <details popover> behaviors, although I don't have any specific suggestions, and I don't think it blocks merging this.

[nt1m]

Thank you @sideshowbarker & @domenic for putting this across the finish line! \o/

[mfreed7]

LGTM. It would be ideal if we could get tests for <details popover> behaviors, although I don't have any specific suggestions, and I don't think it blocks merging this.

There is this test, but it doesn't currently test events. It could fairly easily be updated to do so, however.