Add pop-up feature

[josepharhar]

Feature proposal issue: https://github.com/whatwg/html/issues/7785 Explainer: https://open-ui.org/components/popup.research.explainer

TODO list:

• Add more descriptive text for web developers and for implementers. @domenic, can you point me to some examples?
• Add examples like the ones here: https://html.spec.whatwg.org/multipage/interactive-elements.html#the-details-element

• [ ] At least two implementers are interested (and none opposed):
  • Chrome
  • …
• [x] Tests are written and can be reviewed and commented upon at:
  • https://wpt.fyi/results/html/semantics/popups
• [ ] Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1307772
  • Firefox: …
  • Safari: …

---

/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 ) /obsolete.html ( diff ) /parsing.html ( diff ) /rendering.html ( diff ) /semantics-other.html ( diff ) /webappapis.html ( diff ) /pop-up.html ( diff )

[josepharhar]

I see that there are many build errors after build.sh says "Running conformance checker...", but when I run build.sh locally, it just stops after "Success!".

How do I run the conformance checker when building locally?

[domenic]

The conformance checker is at https://github.com/validator/validator and has a variety of installation options. You need to run it on the output index.html file.

[josepharhar]

The conformance checker is at https://github.com/validator/validator and has a variety of installation options. You need to run it on the output index.html file.

Thanks, I was able to run it! It looks like the PR builds now

[josepharhar]

@domenic I added some examples in https://github.com/whatwg/html/pull/8221/commits/81acbc77fb7f55592e164ab4c7a2e6cba03ce17e and https://github.com/whatwg/html/pull/8221/commits/9e0421c76e82132537c90e7da6e04885689c79de. Can you point me to any examples of "descriptive text for web developers and for implementers" that you mentioned earlier?

[josepharhar]

Thanks for the review! I'm going to be out until monday so I'll try to get to it then

[josepharhar]

Thanks for the review @domenic! I have addressed all your comments

[josepharhar]

@domenic mind taking another look? Anything else I can do to help move this forward?

[domenic]

It's on my list, sorry! I had a week of TPAC then a week of vacation, so I'm currently down to 20 flagged work emails and 24 flagged GitHub emails :).

[annevk]

Drive-by: element and attribute indices haven't been updated as far as I can tell. (FWIW, I plan on doing a more careful review.)

[josepharhar]

show/hide naming (under discussion in Open UI I guess, not sure how that's going)

Looks like we might rename them to popupshow and popuphide: https://github.com/openui/open-ui/issues/607

Element vs. HTMLElement; i.e., behavior for unknown, SVG, and MathML elements

@mfreed7 Why did you implement all of the popup behavior in Element instead of HTMLElement? What do you think the behavior should be for unknown, SVG, and MathML elements?

[mfreed7]

@mfreed7 Why did you implement all of the popup behavior in Element instead of HTMLElement? What do you think the behavior should be for unknown, SVG, and MathML elements?

My general logic has been "why not?". I.e. we had the same debate in the TAG about limiting this feature to only some element types. And if there's a good reason to limit it, great. But if the only reason is that we don't know what it'll be used for, I think web developers will figure that out. For example, why not a pop-up SVG? I could think of some use cases where an icon (a "like button" for example) is made with SVG and would like to be a pop-up.

Side note: this is currently broken in Chromium, but pending this discussion, I'll fix that. Just a missing set of rules in the UA stylesheet.

[domenic]

Well, generally the HTML spec only defines features for HTML; for example that's why the CSS UA stylesheet only applies to HTML attributes. See e.g. https://cool-massive-appendix.glitch.me/xhtml.xhtml for a demo of how hidden="" only applies to HTML elements, not to SVG or to custom-namespace elements.

I suspect we'd have to make more extensive changes than just CSS UA stylesheets, at least at the spec level, if we wanted to support this feature on other-namespace elments.

[mfreed7]

Well, generally the HTML spec only defines features for HTML; for example that's why the CSS UA stylesheet only applies to HTML attributes. See e.g. https://cool-massive-appendix.glitch.me/xhtml.xhtml for a demo of how hidden="" only applies to HTML elements, not to SVG or to custom-namespace elements.

I suspect we'd have to make more extensive changes than just CSS UA stylesheets, at least at the spec level, if we wanted to support this feature on other-namespace elments.

Ha. Yeah that’s a very good point. Ok, that serves, at least for me, as a good enough reason to move this to HTMLElement. If we want to expand later, we can always do that.

[mfreed7]

This is a very big PR and I can't review it all in one go. Submitting some comments for things I found in my first sitting.

Yep, it's huge, sorry about that. We've got a plan to break the next one (for <selectmenu>) up into pieces. Thanks for reviewing!!

[josepharhar]

Ha. Yeah that’s a very good point. Ok, that serves, at least for me, as a good enough reason to move this to HTMLElement. If we want to expand later, we can always do that.

Ok, I changed the uses of "all Elements" to "all HTML elements", hopefully that's good enough.

[josepharhar]

As per the resolution in https://github.com/openui/open-ui/issues/617#issuecomment-1278147334 I have removed popup=hint from this PR. Hopefully the algorithms still make sense, particularly hide-all-pop-ups-until

[foolip]

Oh my, the diff isn't loading again. I'm reviewing against a local checkout and will have to leave comments here instead:

• The "pop-up visibility state" needs to have an initial state, probably hidden? This could optionally be folded into the list of element state including "pop-up previously focused element", or at least put alongside to group element state.
• In the "Pop-up light dismiss" algorithm, we need to check if the event is trusted. Also consider what should happen if event.stopImmediatePropagation() was called, should the escape key press then be ignored? How about if event.preventDefault() is called? Consider if writing this more in the style of activation triggering input event would remove room for interpretation/confusion on such questions.
• Attribute change steps are defined for elements, but are here defined for the attribute. Defining those steps for all HTML elements would fix this.
• An "InvalidStateError", not a "InvalidStateError"
• "Otherwise, set shouldRestoreFocus to false" is a no-op, it's already false.
• Is "focused area of the document" always an element? The rest of the spec is written as if it might not be.
• "Recalculate styles and update layout for document" -- is this an async process, so that calling element.hidePopUp() immediately after element.showPopUp() should throw an exception? If so, this isn't unambiguous as written IMHO.
• Since hidePopUp() may wait for animations to finish, should it return a promise?

[josepharhar]

The "pop-up visibility state" needs to have an initial state, probably hidden? This could optionally be folded into the list of element state including "pop-up previously focused element", or at least put alongside to group element state.

Ok, I made it initially hidden.

Also consider what should happen if event.stopImmediatePropagation() was called, should the escape key press then be ignored?

This should prevent the event from bubbling up to the document node, and would implicitly prevent light dismiss from being run.

How about if event.preventDefault() is called?

Good question. @mfreed7 do you think we should check if the default has been prevented in the popup light dismiss handler? As far as I can tell, event.preventDefault() won't currently affect popup light dismiss.

Attribute change steps are defined for elements, but are here defined for the attribute. Defining those steps for all HTML elements would fix this.

Done

An "InvalidStateError", not a "InvalidStateError"

Done

"Otherwise, set shouldRestoreFocus to false" is a no-op, it's already false.

Done

Is "focused area of the document" always an element? The rest of the spec is written as if it might not be.

Should I say "DOM anchor of the focused area of the document" like this instead? https://html.spec.whatwg.org/#dom-documentorshadowroot-activeelement

"Recalculate styles and update layout for document" -- is this an async process, so that calling element.hidePopUp() immediately after element.showPopUp() should throw an exception? If so, this isn't unambiguous as written IMHO.

This is synchronous. I don't need to add any exceptions right?

Since hidePopUp() may wait for animations to finish, should it return a promise?

@mfreed7 what do you think? I see that currently we have a pseudo selector that changes when the popup is done hiding, but no events get fired or anything.

Oh my, the diff isn't loading again. I'm reviewing against a local checkout and will have to leave comments here instead:

Ok, I squashed and rebased again. From now on I will squash and rebase every time I commit anything new, so this shouldn't happen again. If it does, please ping me.

[foolip]

Is "focused area of the document" always an element? The rest of the spec is written as if it might not be.

Should I say "DOM anchor of the focused area of the document" like this instead? https://html.spec.whatwg.org/#dom-documentorshadowroot-activeelement

Yeah, copying that wording sounds good. Then you know it's a Node, but it might not be an Element. If it isn't, then what?

"Recalculate styles and update layout for document" -- is this an async process, so that calling element.hidePopUp() immediately after element.showPopUp() should throw an exception? If so, this isn't unambiguous as written IMHO.

This is synchronous. I don't need to add any exceptions right?

If it's synchronous no changes are needed. If you have some tests that would fail if it wasn't synchronous, I don't think it needs to be explicitly noted in the spec that it's sync. (Whereas implicit async doesn't really work, thus my question.)

Oh my, the diff isn't loading again. I'm reviewing against a local checkout and will have to leave comments here instead:

Ok, I squashed and rebased again. From now on I will squash and rebase every time I commit anything new, so this shouldn't happen again. If it does, please ping me.

👍

[mfreed7]

Since hidePopUp() may wait for animations to finish, should it return a promise?

@mfreed7 what do you think? I see that currently we have a pseudo selector that changes when the popup is done hiding, but no events get fired or anything.

This is an open discussion here: https://github.com/whatwg/html/issues/8386

I think it's likely we'll eventually want to fire events after the show/hide process completes also. I don't think a promise is the right thing though, since there are many non-JS ways that a pop-up can be hidden, e.g. an ancestor pop-up gets hidden, causing the pop-up stack to all get hidden.

[josepharhar]

I made the spec throw exceptions due to invalid popover attribute state a while ago, but the chromium implementation still returns due to the issues mason listed here: https://github.com/whatwg/html/pull/8221#discussion_r992838111 @domenic what do you think about returning instead of throwing exceptions?

[domenic]

If we uniformly return and never throw exceptions, that's also OK with me. It's the issue where we sometimes return, and sometimes throw exceptions, that was problematic.

[mfreed7]

One OpenUI resolution, driven by this spec PR, was to merge the popoverhide and popovershow events into a single "toggle" event. Having implemented this change in Chromium, it feels fairly clean. But there's one question about how this plays with existing users of the "toggle" event.

The desired behavior for popover is for this "toggle" event to be a ToggleEvent event, containing a state attribute which takes on values "opening" or "closing". That makes it very easy for developers to use the event:

myPopover.addEventListener('toggle', (e) => {
  if (e.state === "closing") {
    // Do closing stuff
  } else {
    // Do opening stuff
  }
});

Without the state attribute, the alternative is to use the current state of the popover, which gets confusing. A "closing" transition starts with the popover in the "open" state, and vice versa. Additionally, in some variations of animations support, the popover can even be in a third "animating" state. So having the easy context of which direction the toggle is going seems very helpful to developers, to avoid footguns.

Having said all of that, the question I'm raising here is: is it "ok" to re-use the "toggle" event here and in <details>, when the <details> event just uses a generic Event object? It seems like maybe yes, since I notice that the "error" event can fire both Event or ErrorEvent. But I wanted to make sure. @domenic @annevk

[domenic]

There's no problem, indeed, with using different interfaces for the same event.

However, this has brought to light the worry that this toggle event has a different timing than <details>'s toggle event, it seems. For <details>, since toggle is fired from a task, the state of the element at the time of the event firing will be the "destination" state. Whereas it sounds like for popover, the plan is to fire it synchronously, so that the state of the element at the time of the event firing is the "source" state?

I think that's not a great situation. Possible solutions:

• Fire toggle async, to be symmetric with <details>. Probably this breaks important use cases you've considered, but just mentioning it for completeness.

• Rename to beforetoggle. Potentially rename the event.state attribute to something clearer, like event.futureState or event.destinationState or similar. Potentially add the counterpart, event.currentState or event.sourceState or similar.

Also in general I'm a bit confused by the state/"closing"/"opening" setup. I would expect "open" and "closed" to be states, and "closing"/"opening" to be actions which transition between states. Or are there four states, actually?

[foolip]

I found two uses of previously focused element which only exists for dialog elements, and suggested what I think is the fix.

However, that raises a question: do we need two separate slots for this, or could we have a single "previously focused element" concept? Are there tests that depend on them being separate and having different values?

[foolip]

A few more questions on "previously focused":

• Can "popover previously focused element" really only be an HTML element? In the Chromium implementation it's an Element and it looks like it comes from document.FocusedElement()
• Is the distinction between focused used by dialog and "document's focused area of the document's DOM anchor" used by popover significant?

[mfreed7]

There's no problem, indeed, with using different interfaces for the same event.

Thanks for the confirmation. 😄

However, this has brought to light the worry that this toggle event has a different timing than <details>'s toggle event, it seems. For <details>, since toggle is fired from a task, the state of the element at the time of the event firing will be the "destination" state. Whereas it sounds like for popover, the plan is to fire it synchronously, so that the state of the element at the time of the event firing is the "source" state?

That's correct, and as you mentioned, there are important use cases (e.g. starting animations) for firing it synchronously before the transition.

• Rename to beforetoggle. Potentially rename the event.state attribute to something clearer, like event.futureState or event.destinationState or similar. Potentially add the counterpart, event.currentState or event.sourceState or similar.

This makes total sense to me, and is likely clearer to developers about the timing. I think I'd like to go this route.

On the name of the attribute, you'd prefer something about the end state, and not the transition itself? Personally I find action==="opening" vs. futureState=="open" to be easier to understand, but only by a little. I'm fine either way. I suppose if we take your second suggestion and add both currentState and newState (better than futureState?) then it isn't too bad.

Also in general I'm a bit confused by the state/"closing"/"opening" setup. I would expect "open" and "closed" to be states, and "closing"/"opening" to be actions which transition between states. Or are there four states, actually?

I agree, state was likely a bad choice. Something like action or transition would be better. There are only two transitions here, "opening" and "closing".

---

TL;DR: here's the new direction I'll go, based on your feedback (edited: ToggleEvent-->BeforeToggleEvent):

• event name is beforetoggle, which uses BeforeToggleEvent:

interface BeforeToggleEvent : Event {
    constructor(DOMString type, optional BeforeToggleEventInit eventInitDict = {});
    readonly attribute DOMString currentState; // "open" or "closed"
    readonly attribute DOMString newState;     // "open" or "closed", always the opposite of currentState
};

[josepharhar]

However, that raises a question: do we need two separate slots for this, or could we have a single "previously focused element" concept? Are there tests that depend on them being separate and having different values?

"previously focused element" is only present on the dialog element. I could keep it like this or I could... rename "popover previously focused element" to "previously focused element" and remove the definition in the dialog element's section... what do you think?

Can "popover previously focused element" really only be an HTML element? In the Chromium implementation it's an Element and it looks like it comes from document.FocusedElement()

I changed it to Element

Is the distinction between focused used by dialog and "document's focused area of the document's DOM anchor" used by popover significant?

I don't think so. I would change it to be just "focused" like the dialog element has but I'm worried that domenic or someone asked me to word it like this - not 100% certain though.

[domenic]

On the name of the attribute, you'd prefer something about the end state, and not the transition itself?

I'm not sure, actually. I think I would be OK with either, as long as the name for the property were clearer. I just got thrown off a lot by state. Maybe it's worth asking developers which would be more intuitive?

But, the sketch you have (including the names currentState and newState) is very clear, so maybe we should just stick with that. To be explicit, your IDL looks great and I would be happy with that.

I don't think so. I would change it to be just "focused" like the dialog element has but I'm worried that domenic or someone asked me to word it like this - not 100% certain though.

Yeah, I asked for Joey to increase precision here. The way dialog did it is totally imprecise and confusing. Ideally we should fix that at some point.

If we could unify these into a single slot, which used the precise wording, then that would probably be nice. Of course we'd need to test the interactions for <dialog popover>.

[mfreed7]

But, the sketch you have (including the names currentState and newState) is very clear, so maybe we should just stick with that. To be explicit, your IDL looks great and I would be happy with that.

Great, thanks for the feedback. I'm also inclined to stick with the new currentState and newState language. I initially didn't like it as much, but after implementing it and writing some event listeners, it feels pretty clean.

However, that raises a question: do we need two separate slots for this, or could we have a single "previously focused element" concept? Are there tests that depend on them being separate and having different values?

"previously focused element" is only present on the dialog element. I could keep it like this or I could... rename "popover previously focused element" to "previously focused element" and remove the definition in the dialog element's section... what do you think?

I'm in favor of combining "previously focused element" for both dialog and popover. I don't think they need separate slots, since only one behavior (dialog or popover) can be active at a time.