Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add element.popoverOpen #9054

Closed
jakearchibald opened this issue Mar 21, 2023 · 17 comments
Closed

Add element.popoverOpen #9054

jakearchibald opened this issue Mar 21, 2023 · 17 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: popover The popover attribute and friends

Comments

@jakearchibald
Copy link
Contributor

Previous discussion: #9045 (comment)

Right now, the way to detect if a popover is open is:

element.matches(':open');

This isn't too hard, although it will throw in browsers that don't support :open. A bigger issue is that it isn't very discoverable.

Instead:

partial interface HTMLElement {
  readonly attribute boolean popoverOpen;
};

…would also be falsy in browsers that don't support the feature, and would be discoverable via DX features like autocomplete.

We may want an equivalent for <dialog>, but that depends on the outcome of #9045.

@keithamus
Copy link
Contributor

keithamus commented Mar 21, 2023

We may want an equivalent for <dialog>, but that depends on the outcome of #9045.

<dialog> already has open, which is the equivalent, right?

partial interface HTMLDialogElement {
  [CEReactions] attribute boolean open;
}

https://html.spec.whatwg.org/multipage/interactive-elements.html#the-dialog-element

Or you mean it should have one for :modal too?

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Mar 21, 2023

I think it depends on how serious the effort to deprecate non-modal dialogs is, and how incompatible the two types of 'open' are after #9045.

@scottaohara
Copy link
Collaborator

deprecating non-modal dialogs wouldn't be a good idea, imo. popover isn't a replacement for them after all. rather, if one wants a dialog with popover behaviors, they should be using <dialog popover=...>

@jakearchibald
Copy link
Contributor Author

Let's keep this thread about element.popoverOpen. Like I said, an equivalent for <dialog> depends on things settling in other issues.

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Mar 21, 2023

Turns out that the intent of :open is not just to match open popovers, but also other kinds of openness, such as <details open> (thanks @bkardell for sending me the resolution). So element.matches(':open') isn't an effective way to detect if the element is an open popover.

There are even cases where a single element, such as <details popover>, can be two kinds of :open at the same time.

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 21, 2023

Turns out that the intent of :open is not just to match open popovers, but also other kinds of openness, such as <details open> (thanks @bkardell for sending me the resolution). So element.matches(':open') isn't an effective way to detect if the element is an open popover.

There are even cases where a single element, such as <details popover>, can be two kinds of :open at the same time.

This is a good point, and a good case for adding popoverOpen. You could always use .matches(':open:not([open])') but that's a mouthful and not ergonomic.

I'm supportive of adding popoverOpen, defined roughly as:

The popoverOpen IDL attribute returns true if the element's popover visibility state is showing, or false otherwise.

In other words, this isn't sugar for .matches(':open') but is specific to Popovers that are open as a popover, which allows you to more easily disambiguate the <dialog popover> and <details popover> cases.

@jakearchibald
Copy link
Contributor Author

You could always use .matches(':open:not([open])') but that's a mouthful and not ergonomic.

And not future-proof, since new kinds of 'open' may be introduced.

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Mar 21, 2023
@annevk
Copy link
Member

annevk commented Mar 22, 2023

Is the use cases here to avoid throwing? What about going in the opposite direction, wouldn't that require this to be a ti-state?

@jakearchibald
Copy link
Contributor Author

I'd like to avoid throwing, but that's being discussed in #9045.

The use-case is just knowing whether the popover is open or not.

What's the third state? "Not a popover"?

@annevk
Copy link
Member

annevk commented Mar 22, 2023

I guess you can check .popover !== null in addition as well. (We should have made that always return a string. 😟)

@jakearchibald
Copy link
Contributor Author

It isn't too late to change stuff like that, but new Image().crossOrigin is null, so it feels somewhat platform consistent. Or is that pattern to be avoided these days?

@annevk
Copy link
Member

annevk commented Mar 22, 2023

We are inconsistent on this, e.g., referrerPolicy will return the empty string. I generally think it's better if getters return a single type, provided the value space fits within that type, as is the case here.

@domenic
Copy link
Member

domenic commented Mar 22, 2023

I like the nullable version. Empty strings being a third way for the platform to represent "not present" (after undefined and null) is confusing, and especially confusing given that popover="" in HTML (i.e., empty string content attribute) means something different.

@annevk
Copy link
Member

annevk commented Mar 22, 2023

Yeah sorry, I didn't mean to suggest using the empty string. "no-popover" would be clearer.

@domenic
Copy link
Member

domenic commented Mar 22, 2023

I still think that's weird unless we make popover="no-popover" work. Which I don't see a reason for; the absence of the content attribute is a better way of expressing the lack of popover. I don't see where this "nullable IDL attributes are bad" principle is coming from.

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 31, 2023

+1 at least personally that .popover === null feels very clear to me.

Back to the OP for this issue, given recent changes, the definition of element.popoverOpen can now be 100% synonymous with element.matches(':popover-open'), I think.

@nt1m
Copy link
Member

nt1m commented Jun 6, 2023

I think this can be closed given element.matches(':popover-open') can be used? Please feel free to re-open if you think element.popoverOpen should still be a thing.

@nt1m nt1m closed this as completed Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: popover The popover attribute and friends
Development

No branches or pull requests

7 participants