Should popover/dialog show/hide when already shown/hidden throw?

[jakearchibald]

Current state

• close() on a closed <dialog>.
• show() on an open <dialog>, unless that dialog is in the popover showing state.

…do not throw.

Whereas:

• showModal() on an open dialog
• show() on an open dialog that is also in the popover showing state.
• showPopover() on an open popover
• hidePopover() on a closed popover

…throw an error.

So the current state of things is that dialog is inconsistent with itself, and popover is inconsistent with dialog.

Elsewhere on the platform

Looking at other cases of hide/remove/delete in the web platform, none of these throw:

• new Set().delete('foo')
• ('hello').replace('world', '')
• [].pop()
• document.createElement('div').removeAttribute('foo')
• document.createElement('div').remove()
• document.createElement('div').classList.remove('foo')
• document.createElement('div').removeEventListener(() => {})
• document.createElement('div').classList.replace('foo', 'bar') (although it does return false)

Counter examples:

• el.removeChild(otherEl) throws if otherEl is not a child of el.
• el.removeAttributeNode(attributeNode) throws if attributeNode is not an attribute node of el.
• document.exitFullscreen() throws if no element is fullscreen.

removeChild and removeAttributeNode are pretty old APIs that developers tend to avoid in favour of friendlier equivalents.

exitFullscreen is the most interesting example, since it's related to top level.

Looking at 'add' cases:

• new Set(['foo', 'bar']).add('foo')
• el.classList.add('foo')
• el.addEventListener(callback)

All behave set-like. As in, if the item is already in the set, it doesn't throw, and it doesn't change the order of items. It's a no-op.

el.requestFullscreen(options) on an already-fullscreen element will adapt to changes in options, but it will not fire a fullscreenchange event.

el.append(otherEl) will remove otherEl from its parent, and add otherEl as the last child of el. This happens even if otherEl is already the last child of el, and it's observable in a bunch of ways, including mutation observers. But this is specifically 'append', not 'add'.

The current mindset in the frameworks world is to let developer declare the state they want, and the framework figures out what needs to change to get to that state. None of the frameworks throw if the developer re-declares the current state.

Proposal for <dialog>

It's pretty weird that show() followed by showModal() will throw, whereas showModal() followed by show() will no-op.

Option 1: 'show' throws if already shown

show() or showModal() followed by show() or showModal() will throw.

This is at least consistent between the two methods, but it doesn't seem to fit with the majority of the platform. You could say the behaviour here is justified in being unusual due to the different ways a dialog can be shown, but that wouldn't be consistent with requestFullscreen(options) either.

Option 2: 'show' is a no-op if already shown

With show() or showModal() followed by show() or showModal(), the second call will be a no-op.

This is at least consistent between the two methods, but it seems weird that showModal() would no-op resulting in a not-modal dialog.

Option 3: Update the type of 'show'

show() followed by showModal() will 'upgrade' the dialog to a modal dialog.

showModal() followed by show() will 'downgrade' the dialog to a non-modal dialog.

This feels consistent with how requestFullscreen(options) will react to changes in options even if the element is already fullscreen.

Option 4: showModal() is a more specific version of show()

show() followed by showModal() will 'upgrade' the dialog to a modal dialog.

showModal() followed by show() is a no-op, because the dialog is already shown.

This seems less flexible, but it feels like it makes sense. We could add showModeless() in future to do the more specific thing, or add an option like show({ mode: 'modeless' }).

Proposal for popover

I think we should match the majority of the platform, and open-when-already-open, and close-when-already-closed, should not throw.

Option 1: Second showPopover() moves popover to the top

This could be achieved by hiding then re-showing the popover, which would be like el.append(). If the developer is calling showPopover(), it seems like they feel this popover is important at this time, which suggests move-to-top is the right thing to do.

If this makes sense, showModal on <dialog> should do the same.

Option 2: Second showPopover() is a no-op

That's consistent with most of the platform.

Proposal for popover on <dialog>

Option 1: <dialog> cannot popover

Similar to requestFullscreen, calling showPopover() on a <dialog> will always throw, even if the dialog isn't open. That removes the overlap between these two features, and it's consistent with fullscreen.

Option 2: Avoid both features being active at the same time

• If show() is not a no-op, then it should throw if the element is in the popover showing state.
• If showModal() is not a no-op, then it should throw if the element is in the popover showing state.
• If showPopover() is not a no-op, then it should throw if the element is a dialog, and open.

I'm not sure this complexity is worth it, and it's inconsistent with requestFullscreen.

[annevk]

cc @nt1m @josepharhar @mfreed7

[keithamus]

I’m inclined to agree - at least with nooping. Guarding these calls is a little unergonomic as iirc the only way to check if a popover is open is via .matches(':open'). With <dialog> one can check open but that’s not present on popovers.

I wonder if it makes sense to alias dialogs show/hide to showPopover/hidePopover? Since it enacts the same mechanics iirc?

[jakearchibald]

I wonder if it makes sense to alias dialogs show/hide to showPopover/hidePopover? Since it enacts the same mechanics iirc?

The algorithms are pretty different, so I don't think that's possible.

[josepharhar]

Option 1: 'show' throws if already shown
show() or showModal() followed by show() or showModal() will throw.

This sounds good to me, I wonder why show() isn't doing that. I'm also more supportive of modifying show() than showModal().

Option 1: cannot popover
Similar to requestFullscreen, calling showPopover() on a will always throw, even if the dialog isn't open. That removes the overlap between these two features, and it's consistent with fullscreen.

This sounds good to me, especially if thats what requestFullscreen does. I don't think that popover on dialog elements is a use case that makes any sense.

Option 2: Second showPopover() is a no-op
That's consistent with most of the platform.

But calling dialog.showModal() a second time also throws an exception, I don't see why we can't do the same thing. If a majority of some APIs from the DOM spec don't throw exceptions when they could, I don't find that very motivating.

I’m inclined to agree - at least with nooping. Guarding these calls is a little unergonomic as iirc the only way to check if a popover is open is via .matches(':open'). With one can check open but that’s not present on popovers.

I guess that's a good point. If yall really want to make it noop then I won't stop you.

I wonder if it makes sense to alias dialogs show/hide to showPopover/hidePopover? Since it enacts the same mechanics iirc?

Yeah I don't think this is a good idea since we are defining showPopover/hidePopover on all HTML elements regardless of the presence of the popover attribute. If we renamed/aliased them to show/hide, then every element would have a show and hide method even if it isn't a popover which would be confusing. It would even conflict with dialog's show method.

[jakearchibald]

@josepharhar

Option 1: 'show' throws if already shown
show() or showModal() followed by show() or showModal() will throw.

This sounds good to me, I wonder why show() isn't doing that. I'm also more supportive of modifying show() than showModal().

Can you provide some detail around why you're happy with that option, given the inconsistency with most of the web platform?

[keithamus]

If we renamed/aliased them to show/hide, then every element would have a show and hide method even if it isn't a popover which would be confusing.

I more meant that for <dialog> elements show() and showPopover() would do the same thing. It was not a proposal to add show() to HTMLElement.

[josepharhar]

Can you provide some detail around why you're happy with that option, given the inconsistency with most of the web platform?

showModal() serves more of a purpose than show(), and there is even a consideration to deprecate/remove show(): #7707 (comment)
Modifying show() is also less work than modifying showModal(), close(), showPopover(), and hidePopover().

I found the dialog element APIs more motivating than DOM APIs. How about looking at some more HTML APIs?

• HTMLInputElement.showPicker doesn't throw if it's already open, so theres one point for no-oping.
• HTMLDetailsElement doesn't have a method to open or close, it's just controlled by the attribute, which obviously doesn't throw when you tell it to open twice.

I couldn't think of any other examples.

el.requestFullscreen(options) on an already-fullscreen element will adapt to changes in options, but it will not fire a fullscreenchange event.

Ah, I missed this detail that requestFullscreen does not throw when the element is already fullscreen.

I guess it makes sense to make them not throw. It sounds like you're in favor of making dialog.show(), dialog.showModal(), dialog.close(), element.showPopover(), and element.hidePopover() never throw based on whether they are already open or closed?

[josepharhar]

I more meant that for elements show() and showPopover() would do the same thing. It was not a proposal to add show() to HTMLElement.

Sorry I misinterpreted. So dialog.showPopover() would be the same as dialog.show()? Or dialog.showModal()...? It still sounds funny since dialogs are not popovers. Would the dialog element need the popover attribute? Would the presence of the popover attribute affect what dialog.showPopover() does?

[jakearchibald]

Modifying show() is also less work than modifying showModal(), close(), showPopover(), and hidePopover().

I found the dialog element APIs more motivating than DOM APIs.

We shouldn't avoid doing the right thing because it's more work. And we shouldn't assume that <dialog> is doing the right thing just because it already exists.

It sounds like you're in favor of making dialog.show(), dialog.showModal(), dialog.close(), element.showPopover(), and element.hidePopover() never throw based on whether they are already open or closed?

Absolutely, since that's how most of the platform works. There are multiple options for how to do that though (options 2-4).

[keithamus]

Would the dialog element need the popover attribute? Would the presence of the popover attribute affect what dialog.showPopover() does?

This raises another question which I don't think @jakearchibald's OP covers. Right now showPopover()/hidePopover() will throw if the element does not have either popover="manual" or popover="auto". What should the behaviour there be?

[mfreed7]

Thanks for raising the detailed issue here, @jakearchibald. (Side note for others, this was briefly discussed on #9036.)

Ok, given the good case you've made for consistency with the rest of the platform, I vote for these choices:

• <dialog> Option 4: showModal() is a more specific version of show()
• Popover Option 2: Second showPopover() {and hidePopover()} is a no-op
• <dialog popover> Option 2: Avoid both features being active at the same time

In terms of the current spec, I believe the only needed changes would be:

• Remove the exception for showModal() on an already-open dialog. This becomes a no-op.
• Remove the exception for showPopover() on an already-open popover. This becomes a no-op.
• Remove the exception for hidePopover() on an already-closed popover. This becomes a no-op.

The behaviors for <dialog popover> listed for Option 2 are what the current spec says to do.

[mfreed7]

This raises another question which I don't think @jakearchibald's OP covers. Right now showPopover()/hidePopover() will throw if the element does not have either popover="manual" or popover="auto". What should the behaviour there be?

These throw NotSupportedException because those operations aren't supported on something that isn't a popover. That feels consistent with other APIs, doesn't it? E.g. ElementInternals.form throws NotSupportedException if the CE isn't a FACE.