[popup] Naming for show and hide events

[josepharhar]

@domenic pointed out here that the show and hide event names may be too generic: whatwg/html#8221 (comment)

I feel bad using the generic event names "show" and "hide" for something pop-up specific? Like, if I do el.hidden = true, I would expect this to fire a hide event.

I couldn't find any discussion about these event names in the list of issues at the bottom of the explainer. Does anyone know how we decided on these names? Does anyone have any thoughts?

[Kaiido]

That also struck me when reading the PR at whatwg/html. I believe it's particularily surprising since this event is also available on window. Reading window.onshow = (evt) => {... one would probably expect it to refer to the pageshow event rather than a popup in the document.

If I can start the bikeshedding, I guess popupshow and popuphide would be quite simple and clearer.

[mfreed7]

The counterpoint, I guess, might be that if later there are other non-pop-up things that show and hide, it'd be nice to re-use an easy set of events like show and hide.

If we indeed need to rename these, then popupshow and popuphide seem the most obvious and straightforward.

[domenic]

other non-pop-up things that show and hide

Well, I think <div>s show and hide pretty frequently already, when you e.g. change them from display: none to display: block...

[mfreed7]

other non-pop-up things that show and hide

Well, I think <div>s show and hide pretty frequently already, when you e.g. change them from display: none to display: block...

Then lots of show/hide events! Just kidding. Probably best to go with popupshow and popuphide. I don't see any other downsides, and it does make the context more clear.

Any objections?

[css-meeting-bot]

The Open UI Community Group just discussed [popup] Naming for show and hide events, and agreed to the following:

• RESOLVED: we would like to support event names that are general, and are supported by anything that supports the `:open` and `:closed` pseudo classes. We need to determine the best names for these events.

The full IRC log of that discussion

<gregwhitworth> Topic: [popup] Naming for show and hide events
<gregwhitworth> github: https://github.com//issues/607
<JonathanNeal> masonf: I would like to avoid bike shedding if we can. The issue is that the names we have for the events — `show` and `hide` are too generic.
<flackr> q+
<masonf> Proposed resolution: rename `show` to `popupshow` and `hide` to `popuphide`.
<JonathanNeal> masonf: This might be confusing. A developer might try to wire up one of these events and wonder why they don’t fire when something like `display:none` ‘hides’ the element.
<gregwhitworth> ack flackr
<gregwhitworth> That was what I was going to say
<una> q+
<JonathanNeal> flackr: we have these generic terms for CSS. The other option is to follow that convention?
<JonathanNeal> q+
<JonathanNeal> masonf: I suppose we could. That would add events to the dialog element. In principle, it does sound reasonable.
<gregwhitworth> ack una
<gregwhitworth> q+
<JonathanNeal> una: from a dev experience perspective, I would not want to have to know to use `show`, `open`, etc. I would definitely vote to make it as easy as possible to guess what the name is as they are coding. If we have normalized this in CSS, I think it would benefit the API to differentiate that.
<JonathanNeal> masonf: this has been discussed, and, in the context of open and close. The distinction made, particularly to dialog, that close is different than hidden and "open" and "shown" are different. Something can be "open" and not "shown". Like when the content is open but not displayed.
<JonathanNeal> masonf: having said that, for popup, we decided on shown and hidden. some parts of that ship have sailed.
<JonathanNeal> flackr: these would align with our css concept, tho?
<JonathanNeal> masonf: in that vain, would you suggest we rename `showPopup` to `openPopup`
<JonathanNeal> scotto_: there will always be one outlier.
<JonathanNeal> una: I agree there are cases where they are unique. However, when the naming doesn’t have that reasoning and it’s generic, we should try to minimize the number of variations in the syntax.
<emilio> <dialog> uses `show` tho...
<JonathanNeal> masonf: that’s a fair point.
<gregwhitworth> ack JonathanNeal
<una> @emilio 🥲
<gregwhitworth> JonathanNeal: I wanted to ask a question
<gregwhitworth> JonathanNeal: this open keyword being used as an event and CSS psuedo class being well aligned or not
<gregwhitworth> JonathanNeal: open, as a event, doesn't imply I suppose what it does under the hood
<gregwhitworth> JonathanNeal: the name can be evoked as an event
<gregwhitworth> JonathanNeal: the psuedo class can only be applied to something that can be opened or closed
<gregwhitworth> JonathanNeal: if that's not a shared sentiment then I agree with flackr and una on normalizing only if there isn't a footgun where an event doesn't limit itself to an element that can open or close
<flackr> qq+
<gregwhitworth> JonathanNeal: I do understand why you may want them to be different but if they can be normalized
<gregwhitworth> flackr: my hope is if we go with open and closed, they would be mappable that this element has moved to the state where that CSS is applied and that consistent meaning that you're looking for
<masonf> q?
<gregwhitworth> ack flackr
<Zakim> flackr, you wanted to react to JonathanNeal
<JonathanNeal> I’m back to scribing now.
<masonf> Proposed resolution: rename `show` to `opened` and `hide` to `closed`. These events should be fired for anything that supports the `:open` and `:closed` CSS pseudo classes.
<JonathanNeal> Sorry, I was listening. in the other way listening.
<gregwhitworth> ack gregwhitworth
<JonathanNeal> gregwhitworth: currently, it says `show` and `hide` which sound similar to dialog. So, should it be past-tense?
<JonathanNeal> masonf: there may be different cancel-ability.
<JonathanNeal> gregwhitworth: when we show it, is this when it is fired?
<JonathanNeal> masonf: technically, just before
<JonathanNeal> masonf: no matter how its invoked, this event gets fired
<tantek> q+
<JonathanNeal> gregwhitworth: why are we past tense then?
<masonf> Proposed resolution: rename `show` to `open` and `hide` to `closed`. These events should be fired for anything that supports the `:open` and `:closed` CSS pseudo classes.
<JonathanNeal> masonf: I am now convinced to go the other way.
<gregwhitworth> ack tantek
<JonathanNeal> tantek: CSS defines states. Events define transitions, which are verbs.
<emilio> There is a precedent for `:focus`
<JonathanNeal> tantek: I would not align them in names, and I would consider that an error. Aligning them conceptually is helpful. Trying to use the same name is different. They are not the same thing.
<emilio> (the event is focus / blur)
<JonathanNeal> masonf: this begs the question: the particular verb you use depends on the timing of the event.
<JonathanNeal> masonf: and the verb you choose depends on the timing.
<JonathanNeal> tantek: agreed
<gregwhitworth> q+
<JonathanNeal> tantek: the timing should be captured as well to communicate to the developer the lifecycle of what is happening
<JonathanNeal> masonf: any recommendations here? both of these events happen before the thing.
<JonathanNeal> tantek: I don’t have a specific recommendation for a specific name. But I would look at other events in classic HTML support, like loading documents and images, and see where the event gets fired and see if the names that were chosen at the time have any consistent pattern to them.
<JonathanNeal> tantek: I think some of them do have a consistent pattern that we may be able to reuse.
<JonathanNeal> tantek: sorry to answer your question with another question, but I hope it provides a path to a solution.
<JonathanNeal> gregwhitworth: +1 to what tantek said. My key thing is; I would keep with the theme of "open" and "close".
<JonathanNeal> gregwhitworth: when we generalize it (and I hate to delay stuff), say, theoretically, we outline these when they are fired and we decide the event is `beforeopen`.
<JonathanNeal> gregwhitworth: I am in support in being specific, but to Una and Jonathan’s point, I would prefer they are close.
<tantek> q+ to note possible difference between show/hide and open/close even in an event context
<flackr> q+
<gregwhitworth> ack gregwhitworth
<gregwhitworth> ack tantek
<Zakim> tantek, you wanted to note possible difference between show/hide and open/close even in an event context
<JonathanNeal> tantek: the other thought I had regarding this — I was in the CSSWG when this was talked about — that there is a semantic distinction between visually showing and hiding something and twidling something open or closed.
<JonathanNeal> tantek: on the other hand, something like showing or hiding a dialog is literally about showing or hiding those things.
<JonathanNeal> tantek: If these are different, I do not believe they should use the same name.
<JonathanNeal> tantek: it should reflect the semantic state of the component, even if it doesn’t reflect the css name, because the event and the css state may be describing two different things.
<JonathanNeal> masonf: if it is `display:none` it will be open but not visible.
<JonathanNeal> gregwhitworth: the 90% use case will be that it is ‘visually’ open
<gregwhitworth> ack flackr
<JonathanNeal> flackr: I wanted to make an argument for the timing of this. I think that, given the way we do animations, where we dispatch the event and then see if animations were added, implies that the pseudo class matching has already change, because you can already start animations on animations change.
<JonathanNeal> flackr: this aligns with `:focus` when you fire the event when it matches. I think this aligns, so the names are good.
<JonathanNeal> masonf: this event is explicitly fired first to allow for any animations, and then the selector matches once those have been completed.
<JonathanNeal> flackr: the fact that when you fire the event is not significant to that, because adding animations would not be related to the pseudo-class change.
<JonathanNeal> masonf: that’s a good point.
<masonf> Proposed resolution: we would like to support event names that are general, and are supported by anything that supports the `:open` and `:closed` pseudo classes. We need to determine the best names for these events.
<JonathanNeal> gregwhitworth: I don’t think we’ll be able to resolve on this. There is a lot of good insight.
<tantek> my understanding is that the event/JS aspect is lower level and thus happens first to allow "patching", while the pseudo-class state is higher-level
<bkardell_> I opened https://github.com//issues/621 as gregwhitworth requested earlier - sorry it is long :(
<JonathanNeal> gregwhitworth: I’m supportive of this last resolution.
<JonathanNeal> masonf: It might not be a great solution for all the other elements. I'm supportive of trying.
<JonathanNeal> tantek: I think part of the timing bit derives from JS being lower level, and being able to change things before all the CSS rules get changed, which are the higher level.
<JonathanNeal> tantek: the events I see as callback hooks to change or enhance behaviors that would otherwise be declarative.
<JonathanNeal> masonf: I am generally agreeing. We will need `beforeopen`, `afteropen`. There’s even an issue for that, to having paired events on either side.
<masonf> RESOLVED: we would like to support event names that are general, and are supported by anything that supports the `:open` and `:closed` pseudo classes. We need to determine the best names for these events.
<JonathanNeal> gregwhitworth: seems like we’re heading in a direction to have events tiered on either side.

[mfreed7]

I filed an issue with WHATWG/dom, since this is now a more general feature.

whatwg/html#8386

[mfreed7]

I'm bringing this back due to conversation on the html issue. I'm beginning to think we should just add popovershow and popoverhide, and not add more general events. They don't seem useful, upon further thought.

[mfreed7]

One thought I had was to perhaps combine these two events into a single popover event:

popover.addEventListener('popover',() => {
  if (popover.matches(':closed')) {
    // This is a "show" event, because the popover
    // is going from :closed to :open
  } else {
    // This is a "hide" event, because the popover
    // is going from :open to :closed
  }
});

It takes more work to write the event handler, and it might be a bit confusing that the "show" event is detected by looking for :closed (because the event is fired before the state changes). Even more confusing (and perhaps worth a tweak to the hide animation sequence) is that for the "hide" event, neither :open nor :closed will match. The code I wrote above will work correctly, but if written the other way (looking for :open) it won't work.

But people seem to like short, easy to remember names, and popover is definitely that. The behavior is roughly equivalent to the toggle event for the <details> element.

Thoughts?

[domenic]

I like that direction. I think it will be less work for some use cases, e.g. event handlers that mainly want to sync the state change with some other part of the page and thus would have to listen to both events anyway.

Could we reuse the toggle event name? I guess we'd have to figure out what happens for <details popover> though. It'd be nice if we could make it work, but not sure it's worth it... Another option is popovertoggle as the event name.

To make writing such handlers easier, it might be worth considering adding an IDL attribute, e.g. popover.popoverOpen (or popoverState as an enum if it should be tri-state).

[scottaohara]

yah... though this has been humming along as a global attribute, I'd highly question why someone would even try to do <details popover>. Seems it doesn't even work correctly right now, as with the popover attribute it is still visually rendered even if it hasn't been invoked - but even if it did, someone would need a button to invoke the popup, which if not in the open state by default, would then require someone to then toggle open the content they just toggled open? that's some weird behavior... but arguably less weird than what can happen if contenteditable is put on the details element.

[mfreed7]

yah... though this has been humming along as a global attribute, I'd highly question why someone would even try to do <details popover>. Seems it doesn't even work correctly right now, as with the popover attribute it is still visually rendered even if it hasn't been invoked

This seems to be triggered by normalize.css containing this rule:

menu, article, aside, details, footer, header, nav, section {
    display: block;
}

but even if it did, someone would need a button to invoke the popup, which if not in the open state by default, would then require someone to then toggle open the content they just toggled open? that's some weird behavior... but arguably less weird than what can happen if contenteditable is put on the details element.

Well, you could do <details popover open> to have it start out open? I think the rest would still work though, right? I.e. when the <details> is shown (via .showPopover()), it'll still function as a normal details, where the user can toggle it. I hate ruling out elements, but if you think this is just too confusing, perhaps we could make <details popover> a disallowed element. It would be the only one at this point.

I like that direction. I think it will be less work for some use cases, e.g. event handlers that mainly want to sync the state change with some other part of the page and thus would have to listen to both events anyway.

True, but I'd guess most common use cases need to know hide vs. show.

Could we reuse the toggle event name? I guess we'd have to figure out what happens for <details popover> though. It'd be nice if we could make it work, but not sure it's worth it... Another option is popovertoggle as the event name.

Given the above, I do like "toggle" as an event name that we can re-use!

To make writing such handlers easier, it might be worth considering adding an IDL attribute, e.g. popover.popoverOpen (or popoverState as an enum if it should be tri-state).

This has been brought up before, and this shape of the event might push even more for something like popover.popoverOpen. On the tri-state question, the third state would just be when popover isn't a popover (doesn't have the popover attribute). We could make it a nullable boolean? Or just return false in that case?

[domenic]

On the tri-state question, the third state would just be when popover isn't a popover (doesn't have the popover attribute). We could make it a nullable boolean? Or just return false in that case?

I thought there was some third state while it was doing the animation, where neither :open nor :closed matches?

[mfreed7]

On the tri-state question, the third state would just be when popover isn't a popover (doesn't have the popover attribute). We could make it a nullable boolean? Or just return false in that case?

I thought there was some third state while it was doing the animation, where neither :open nor :closed matches?

Yeah, I was thinking popoverOpen would be just sugar for popover.matches(':open'), but you're right it would be more powerful to expose the full three states.

Perhaps instead of popoverOpen, we go with popoverState, with the following (nullable) enumerated values:

1. "open" === matches(':open')
2. "closed" === matches(':closed')
3. "transitioning" === !matches(':open') && !matches(':closed')
4. null (when element doesn't have popover attribute)

[domenic]

That would make sense to me. Although, coming back to the event, I wonder if it will have expected values when the event fires? E.g. maybe it will always equal "transitioning" during that time, which would not be so useful?

[mfreed7]

That would make sense to me. Although, coming back to the event, I wonder if it will have expected values when the event fires? E.g. maybe it will always equal "transitioning" during that time, which would not be so useful?

Yeah, I worried about that also. For the "show" transition, given the order of operations, the popoverState will still be "closed", so that's ok. As currently spec'd and implemented, during the "hide" transition, the event is fired just after the popoverState changes to "transitioning", which is unfortunate. But looking at the logic, I'm thinking we could/should reverse that, so that the state would still be "open" at that point. Then things here would be fairly clean, right?

[domenic]

Yeah, that sounds right. Although I guess all this complexity further convinces me that we should solve this without popover being a weird tri-state thing, as I communicated in whatwg/html#7785 (comment)