whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.11k stars 2.67k forks source link

Support `popovertarget` on custom elements with `role="button"` #9110

Open jpzwarte opened 1 year ago

jpzwarte commented 1 year ago

If I read the spec correctly, then popoverTargetElement will return null unless the node is a <button>?

That seems quite limited and incorrect?

See https://html.spec.whatwg.org/multipage/popover.html#the-popover-target-attributes

To get the popover target element given a [Node](https://dom.spec.whatwg.org/#interface-node) node, perform the following steps. They return an [HTML element](https://html.spec.whatwg.org/multipage/infrastructure.html#html-elements) or null.

1. If node is not a [button](https://html.spec.whatwg.org/multipage/forms.html#concept-button), then return null.

Edited: changed the title to better reflect the issue. This basically forces design systems to implement the popovertarget behavior themselves. Or work around it by adding a <button popovertarget="..."> to the light DOM.

keithamus commented 1 year ago

It is not just <button> but also <input type="button">. This is intentional. Triggers need to be able to receive focus, and be presented as a page control to assistive technologies. It is a similar restriction to <form>s which may only be submitted by a participating <button type="submit"> or <input type="submit">.

/cc @scottaohara @mfreed7 who can probably reference exact discussions on this.

jpzwarte commented 1 year ago

@keithamus just checking but what about custom elements? Do they work as well as long as the have role="button"?

keithamus commented 1 year ago

Elements with a role=button will not work because only button & input have the mixin.

You can use the imperative API of popover.showPopover()/popover.hidePopover()/popover.togglePopover(), or alternatively the custom element can have a shadowdom with a button (but then you'll still need to use the imperative cross-shadow element assignment .popoverTargetElement).

jpzwarte commented 1 year ago

This sucks for design systems using custom elements. For example:

<sl-button popovertarget="popover">Toggle popover</sl-button>
<sl-popover id="popover">I'm a popover</sl-popover>

So this scenario would not work. Is there a good reason why this couldn't work?

keithamus commented 1 year ago

An element like <sl-button> could reflect the attribute to a button in the underlying ShadowDOM. Alternatively this could make for a motivating use case for built-in customised elements, so having <button is="sl-button" popovertarget="popover"></button>.

https://github.com/openui/open-ui/issues/302 goes in to some of the reasons behind the design especially in relation to custom elements, and may be worth a read:

In the spirit of semantic, accessible markup, it is important to restrict which elements can take the popup attribute. A user may not expect to trigger a popup by "invoking" a <div>, so we'd like to avoid encouraging these sorts of patterns.

In the discussion, someone brought up that a custom element could be a trigger for a popup, and that the current restrictions felt overly restrictive as a result. The question for devs writing these types of custom elements is: does anything block you from calling show() on the popup in script for this custom element, or can script suffice to wire up this interaction?

(BTW this needs label https://github.com/whatwg/html/labels/topic%3A%20popover)

jpzwarte commented 1 year ago

Even though custom elements are a JS API, if I can I would much rather implement things declaratively. So I'm not saying I can't make things work using the current spec (I can using the <button> in the light DOM as you suggest), I would much rather use the same API as people would use without a design system. That way I have to write less code, and users of my design system don't need to learn yet another API.

So nothing is blocking me from doing it imperatively, but the DX of doing it declaratively is much better.

So is there any specific reason why custom elements with role="button" could not be supported?

keithamus commented 1 year ago

Yeah FWIW I'm not suggesting it couldn't (or even wouldn't) be supported but I'm explaining the current rationale. Currently role=button cannot be supported because the mixin only applies to <button> and <input>. Adding the attributes globally to all HTML elements would allow non semantic elements to become invokers which works against accessibility best practices.

Westbrook commented 1 year ago

This is yet again another great reason to native behavior mixins to be surfaced to custom element authors (and in a declarative way). I know that's a whole other spec space, but we can't add new features like this if at ever turn we're going to be doing so without conscious awareness for the many related parts of the spec. Some might say is="..." would be the path forward here, but regardless of whether Apple ever support customized built-ins, you'd also have to add the ability to add a shadow root to <button> to be able to mimic this context with the component model that had been standard in browsers for years now. We really need to find a good way to stop having APIs called "declarative" when they don't take into account the full declarative model of the web platform.

y-commit69 commented 1 year ago

Sorry if it's a dumb question, so in order to use popover api in the Declarative shadow dom, we have to use js?

keithamus commented 1 year ago

In order to use the popover API cross-root, you will need to use JS. If the popover API is used entirely in light DOM, or entirely within a shadow root, then it will work fine. So the following examples work fine:

<my-element>
  <template shadowrootmode="open">
    <button popovertarget="my-popover">Open</button>
    <div popover id="my-popover"></div>
  </template>
</my-element>
<my-element>
    <button popovertarget="my-popover">Open</button>
    <div popover id="my-popover"></div>
</my-element>

It is not possible to do this:

<my-element>
  <template shadowrootmode="open">
    <button popovertarget="my-popover">Open</button>
  </template>
  <div popover id="my-popover"></div>
</my-element>

Instead it requires JavaScript to assign the element cross root:

<my-element>
  <template shadowrootmode="open">
    <button popovertarget="my-popover">Open</button>
  </template>
  <div popover id="my-popover"></div>
  <script>
    class MyElement extends HTMLElement {
      static {
        customElements.define('my-element', this)
      }

      connectedCallback() {
        this.shadowRoot.querySelector('button').popoverTargetElement = this.querySelector('#my-popover')
      }
    }
  </script>
</my-element>

The popovertarget attribute expects an ID that is in the same root. Cross-root IDs and cross-root attribute reflection is an open topic around custom elements, but it should be discussed in another issue - this issue is about the limitation of popovertarget only being applicable to input and button elements.

y-commit69 commented 1 year ago

@keithamus

Wow thank you for the detailed reply!!

annevk commented 1 year ago

We should make this work for custom element submit buttons, which don't exist yet: https://github.com/WICG/webcomponents/issues/814. And maybe there should be a way to say that a custom element is a plain button as well.

Westbrook commented 1 year ago

@annevk it would be great for there to be a way for a custom element to (say that it is a button and) participate in this API. ๐Ÿ‘๐Ÿผ ๐Ÿ‘๐Ÿผ ๐Ÿ‘๐Ÿผ

If role wasn't enough, an approach that included a declarative API of another form, even when paired with shadow DOM, should be thought of as an important part of any DOM proposal. Do you have any thoughts on an approach that would be acceptable here?

jpzwarte commented 1 year ago

So https://open-ui.org/components/selectmenu/#default-behavior has something like this:

<button behavior="button">Open</button>

"The behavior="button" attribute on the inner

keithamus commented 1 year ago

I wonder if flipping customised built in elements on their head would be useful? <x-button is=button></x-button> is arguably still SOLID vs <button is=x-button>.

annevk commented 1 year ago

Do you have any thoughts on an approach that would be acceptable here?

I think we should take the approach suggested for submit buttons and generalize it a tiny bit so it can be used for both buttons and submit buttons.

I.e., something like this.#internals.button = "submit" (or "button" for a plain button).

osdiab commented 6 months ago

Hi - based on the Chrome Developer Blog post it sounds like the popover API is potentially to be useful for tooltip interactions. But if popovertarget has to be on a <button> element, then that means that an <a> element can't have a popover because <a> elements can't have <button> children and vice-versa to my understanding. This is pretty crippling for my use cases as it is pretty common in my company's UIs to have tooltips on links. I'm taking from this that the Popover API isn't actually really made to support tooltips despite the Chrome Developer Blog post's suggestion, is this the case? Thanks!

mfreed7 commented 6 months ago

Hi - based on the Chrome Developer Blog post it sounds like the popover API is potentially to be useful for tooltip interactions. But if popovertarget has to be on a <button> element, then that means that an <a> element can't have a popover because <a> elements can't have <button> children and vice-versa to my understanding. This is pretty crippling for my use cases as it is pretty common in my company's UIs to have tooltips on links. I'm taking from this that the Popover API isn't actually really made to support tooltips despite the Chrome Developer Blog post's suggestion, is this the case? Thanks!

For tooltip invoking, the interesttarget proposal is meant to be the answer. But this issue isnโ€™t the right place to discuss that use case. Feel free to look around the open issues on OpenUi, or open one yourself if you think something is missing there.

lukewarlow commented 5 months ago

We should make this work for custom element submit buttons, which don't exist yet: https://github.com/WICG/webcomponents/issues/814. And maybe there should be a way to say that a custom element is a plain button as well.

Big +1 to this idea I think it's worth taking a step back when thinking about it because there's lots of possible options, like is there a way to ask for the default styling of the button?, would setting it to a button make it focusable automatically?, should these instead be separated out (focusability is useful for other more generic types of UI).

Westbrook commented 4 months ago

I.e., something like this.#internals.button = "submit" (or "button" for a plain button).

As [popover] has full x-browser support and a growing number of related features, it would be great to ensure that a custom element button didn't need to be a "submit button" in some way just to achieve the ability to use these APIs. How can the community support moving this conversation forward? As this lands and expands and CSS Anchoring does as well, we're going to see more and more requests for this to "just workโ„ข๏ธ".

keithamus commented 4 months ago

We briefly discussed a solution that could work for this in the hack week discussion on AOM reference targets. @lukewarlow or @alice might be able to say more but I think with a combination of inward/outward targets things like popovertarget should be able to work in combination with shadowroots

Westbrook commented 4 months ago

Cross-shadow root is one thing, I mean a big thing!

However, if <x-button> does not have a <button> in it, for reasons like it being in a buttongroup or similar to satisfy accessible content relationships, would the <x-button> be able to be <x-button popovertarget="">?

smaug---- commented 4 months ago

I'd rather make invokers/commands/actions global and let one to use those. The popover specific attributes could be deprecated.

annevk commented 4 months ago

It doesn't seem like a great idea to globally hijack the default activation behavior? Allowing custom elements to opt into it seems preferable.