Reland "Add popover attribute"

[josepharhar]

Previous PR: #8221

Feature proposal issue: #7785
Explainer: https://open-ui.org/components/popup.research.explainer

• At least two implementers are interested (and none opposed):
  • Chrome
  • Firefox: The Popover API mozilla/standards-positions#698
  • Safari: The Popover API WebKit/standards-positions#74
• Tests are written and can be reviewed and commented upon at:
  • https://wpt.fyi/results/html/semantics/popovers
• Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1307772
  • Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1808823
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=250171

---

/browsers.html ( diff )
/dnd.html ( diff )
/dom.html ( diff )
/form-elements.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/input.html ( diff )
/interaction.html ( diff )
/interactive-elements.html ( diff )
/rendering.html ( diff )
/semantics-other.html ( diff )
/webappapis.html ( diff )
/popover.html ( diff )

[mfreed7]

So it would appear that these are the only open conversation threads from the prior PR:

• Add popover attribute #8221 (comment)
• Add popover attribute #8221 (comment)

[josepharhar]

As per the resolution in openui/open-ui#342 (comment) I have added the "aftertoggle" event which is like "beforetoggle" but is fired asynchronously

[mfreed7]

As per the resolution in openui/open-ui#342 (comment) I have added the "aftertoggle" event which is like "beforetoggle" but is fired asynchronously

This was at the request of @smaug---- but had good consensus in the meeting. The important use cases are a) programmatically setting focus (via imperative logic) both before the popover shows and returning it after it hides, and b) allowing similar code (that uses .focus()) to handle focus management during the popover show and also later on.

[josepharhar]

As per the HTML meeting today, I renamed "aftertoggle" to "toggle" because it matches the details element, and I prevented it from being fired on removal

[annevk]

Still spotting a couple issues, including one that's assert-related.

The toggle event index entry needs to be updated to say it's used for more than the details element.

[annevk]

Can't you hit this assert if a click event listener on a button element removes the popover target?

[josepharhar]

I tried adding a test for this, and realized that the popover target attribute activation behavior is missing some calls to "check popover validity". I added the checks, so this shouldn't be possible anymore.

[domenic]

To recap a bit on what we discussed in that meeting (and got agreement on from the various attendees):

• There are use cases for both a before-toggling sync event, and an after-toggling async event. The OpenUI group has ideated about these, but hasn't used them in the origin trial or demo or similar. But, @smaug---- has many years of experience with XUL popups using the equivalent events, and attests that they are important. So we should have both.

• For naming, we think a pair beforetoggle and toggle is ideal, if and only if we can match the semantics of toggle to its existing usage for <details>.

  • And, it appears we can. Everyone was satisfied with popover="" matching the <details> semantics for toggle, including: does not fire on removal, and fires async from a task (with no animation frame shenanigans).

  • This pattern of event name reuse is pretty common throughout web specs, e.g. load, cancel, input, ...

  • We think it's OK if popover-related toggle events use a special interface with properties communicating the states, even if <details> doesn't for now. We were open to expanding the usage of this new interface to <details> in the future.

  • We did not discuss expanding beforetoggle to <details> in the future, but I think that might be nice too. I think it's less important though.

  • We did not discuss the naming of the event interface and its properties. The current spec PR has PopoverToggleEvent, currentState, and newState. I think that doesn't really work if we're planning to use it for both beforetoggle and toggle, or if we're planning to expand it to <details>'s toggle in the future. I think ToggleEvent, oldState, and newState are probably the best candidates. What does the group think?

  • We did not discuss whether the group was happy matching <details>'s toggle coalescing behavior, which I just remembered. (Step 1 here.) Is this OK with the group? In particular I think this means you could sometime get events where newState and oldState are equal, because that's how <details> behaves today: clicking it twice fires a toggle event even though nothing changed.

  • We did not discuss what happens with <details popover>. I think it works fine: if someone does that confusing thing, they might get toggle events for two separate reasons (a click, or a showPopover()). That might be confusing, but they did it to themselves.

• To be clear, there is no event fired on removal from the DOM. That might be tackled by separate efforts like Make it possible to observe connected-ness of a node dom#533.

Note: Given how many things I ended up noting above as un-discussed, it might be prudent to schedule another sync meeting of some sort for the toggle event, unless everyone quickly converges on the same answers. To make timezones easier you can sacrifice my participation; my main perspective is that we should stick to "beforetoggle and toggle if and only if we can match the semantics of toggle to its existing usage for <details>". If we have compelling reasons to depart (e.g., coalescing behavior is undesirable for popovers) then we should probably switch to popover-specific names.

[mfreed7]

To recap a bit on what we discussed in that meeting (and got agreement on from the various attendees):

Thanks for summarizing the discussion in such detail, and for raising the new points! Universally, I think your conclusions about the "extra" stuff sound totally reasonable. In particular:

• We did not discuss the naming of the event interface and its properties. The current spec PR has PopoverToggleEvent, currentState, and newState. I think that doesn't really work if we're planning to use it for both beforetoggle and toggle, or if we're planning to expand it to <details>'s toggle in the future. I think ToggleEvent, oldState, and newState are probably the best candidates. What does the group think?

Your proposed naming (ToggleEvent, with attributes oldState and newState) sounds good to me. In particular, oldState instead of currentState clears up some ambiguity and potential confusion. So +1.

• We did not discuss whether the group was happy matching <details>'s toggle coalescing behavior, which I just remembered. (Step 1 here.) Is this OK with the group? In particular I think this means you could sometime get events where newState and oldState are equal, because that's how <details> behaves today: clicking it twice fires a toggle event even though nothing changed.

I had forgotten about this behavior. Since we're re-using the toggle event and want it to work the same way, I think it's totally reasonable to also include the coalescing behavior for popover-fired toggle events. So the behavior would be that if a toggle event is already queued for a given popover element, cancel that event, and queue another one with both oldState and newState equal to the new state.

• We did not discuss what happens with <details popover>. I think it works fine: if someone does that confusing thing, they might get toggle events for two separate reasons (a click, or a showPopover()). That might be confusing, but they did it to themselves.

+1, I think they will indeed get two events, and that's ok.

[josepharhar]

Ok, I just pushed a patch for the following:

1. rename PopoverToggleEvent to ToggleEvent
2. stores the async toggle event in a task so it can be canceled when a new toggle event is queued before the previous one fires.
3. renames "currentState" to "oldState"
4. stores "oldState" with the task so that when a toggle event task gets canceled, we keep the "oldState". This means that synchronously hiding then showing a popover fires one toggle event with both oldState and newState set to "closed". I believe this is what Domenic suggested in his last comment.

[josepharhar]

@annevk, any objections to the recent event changes? I've incorporated all of the points in Domenic's last comment into the PR. Do you have any other things that you still need to review?

[annevk]

I rebased this to resolve some merge conflicts with recent focus changes. I also refactored the coalescing to use a shared algorithm. And further addressed a couple nits. (So please pull in those changes before doing more work on this.)

While looking around I noticed that the Pointer Events integration has an unresolved comment from @foolip that needs to be addressed as it likely impacts the text in this PR: w3c/pointerevents#460 (review).

If that is addressed and my changes look good I think we can land this @josepharhar. I will be out next week, but I'll be around until 7pm UTC+1 today or so. (Also, let me take this opportunity to encourage you to hang out at https://whatwg.org/chat. I'm still behind on GitHub notifications so I might not always notice when something is ready for review. And it's generally nice to be able to chat through certain aspects of a change or proposal.)

[josepharhar]

I addressed the pointerevents comment and updated this PR

[annevk]

@josepharhar thank you so much for your patience and excellent work on getting this feature standardized! May it make many end users (and web developers) happy. 🎉

[mfreed7]

🎉 Thanks to everyone involved in getting this landed! A PR so large that it broke Github several times.