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

The declarative popover invoker feature needs to automatically establish an implicit anchor element #10675

Closed
tabatkins opened this issue Oct 4, 2024 · 16 comments · Fixed by #10728
Labels
integration Better coordination across standards needed topic: popover The popover attribute and friends

Comments

@tabatkins
Copy link
Contributor

tabatkins commented Oct 4, 2024

What is the issue with the HTML Standard?

#9144 attempts to define the anchor attribute on popover elements, and ties the creation of the popover's "implicit anchor element" to its presense. That pull has stalled for months, but the implicit anchor element part is vital for the feature to work at all in a reasonable manner, and should be handled separately. This allows an author to style the popover as an anchor-positioned element, without having to mint unique names for every single popover and invoker in the document.

In short: if "show popover" is called on X with an "invoker" element Y, then X's implicit anchor element needs to be set to Y. "hide popover" should clear the element's implicit anchor element.

@annevk annevk added the topic: popover The popover attribute and friends label Oct 7, 2024
@annevk
Copy link
Member

annevk commented Oct 7, 2024

cc @josepharhar @nt1m @whatwg/css

@annevk annevk added the integration Better coordination across standards needed label Oct 7, 2024
@mfreed7
Copy link
Contributor

mfreed7 commented Oct 15, 2024

+1 to the OP. I've prototyped this in Chrome (see the chromestatus for details), and I've chosen this API:

boolean togglePopover(optional (TogglePopoverOptions or boolean) options);
void showPopover(optional ShowPopoverOptions options);
void hidePopover();

with the options dictionaries like this:

dictionary TogglePopoverOptions {
  boolean force;
  HTMLElement invoker;
};

dictionary ShowPopoverOptions {
  HTMLElement invoker;
};

This is just the prototype, so I'd love comments and suggestions.

Essentially, you can set the invoker via popover.showPopover({invoker: button}) and that does all the "normal" things that popovertarget already does. Plus as part of that change, both popovertarget=foo and the new {invoker: button} set up the button as the implicit anchor for the popover.

Thoughts on that shape?

@domenic
Copy link
Member

domenic commented Oct 16, 2024

I think that shape is reasonable. It allows

element.togglePopover({ force: true, invoker });
element.togglePopover({ invoker });

Alternates would be

boolean togglePopover(optional boolean force, optional ShowPopoverOptions options);

which allows

element.togglePopover(true, { invoker });
element.togglePopover(undefined, { invoker });

or maybe

boolean togglePopover(boolean force, optional ShowPopoverOptions options);
boolean togglePopover(ShowPopoverOptions options);

which allows

element.togglePopover(true, { invoker });
element.togglePopover({ invoker });

I mention these alternates because that stays a bit more consistent with other toggle() methods on the platform in accepting a positional boolean argument. But, it's not clear whether that consistency is actually good, since the explicitness of the force name is probably a win.

@tabatkins
Copy link
Contributor Author

Yeah, fwiw, I personally always find myself hesitating when using some of the other toggle() methods; it feels weird to just pass a true/false to force it on/off. A force option reads better to me personally.

@lukewarlow
Copy link
Member

Fwiw I would like to say please no new 'undefined' args if at all possible. They always confuse me as a developer more so than just a raw Boolean argument. But I would agree {force, invoker} is better.

Apologies for doing this because I know bikeshedding is a pain but we should also work out if we want to use the invoker terminology.

As I understand it we've gone with 'source' as the command event property for the 'invoker' element perhaps we could use that here?

@lukewarlow
Copy link
Member

cc @keithamus if this gets in to spec before command/commandfor we should remember to apply the same behaviour there (if there's something we need to actively do to make that happen).

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 18, 2024

Ok, if I'm reading all of the comments correctly, it kind of sounds like rough consensus on the shape of #10675 (comment), does that sound right?

Apologies for doing this because I know bikeshedding is a pain but we should also work out if we want to use the invoker terminology.

No, I was expecting this question. Suggestions appreciated. To me at least, source doesn't really tell me what it means, but I guess it's kind of clear in context? popover.showPopover({source: button}). Other suggestions appreciated. I'm fine with almost any name that makes some kind of sense. I do like that source lines up with the attribute in the CommandEvent spec PR (#9841).

@lukewarlow
Copy link
Member

One question I have, does the invoker that's passed work even if it's in a shadow root?

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 18, 2024

One question I have, does the invoker that's passed work even if it's in a shadow root?

Good question. Based on the fact that there's no way to imperatively retrieve the invoker (right?) and obviously if you're passing it in, you have knowledge of it, then I'd say that should be ok?

@annevk
Copy link
Member

annevk commented Oct 19, 2024

I don't understand how this API proposal relates to OP. OP is asking for the existing mechanism to set the implicit anchor element, no?

@lukewarlow
Copy link
Member

OP mentions the declarative way but it seems right that it's possible imperatively too?

@annevk
Copy link
Member

annevk commented Oct 19, 2024

Sure, but that seems like a separate issue.

@lukewarlow
Copy link
Member

I guess that would be #10442

@tabatkins
Copy link
Contributor Author

It is a separate issue, yes.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 21, 2024

Essentially, you can set the invoker via popover.showPopover({invoker: button}) and that does all the "normal" things that popovertarget already does. Plus as part of that change, both popovertarget=foo and the new {invoker: button} set up the button as the implicit anchor for the popover.

See my prior comment (copied here) - it is two things combined into one for simplicity/convenience. Sentence one is related to #10442. Sentence two is related to this issue (#10675). I thought we could just consider them together.

mfreed7 added a commit to mfreed7/html that referenced this issue Oct 28, 2024
This includes two related changes:
 1. The `showPopover()` and `togglePopover()` methods now include an
    options bag that allows setting the popover invoker.
 2. Popover invokers (declaratively or imperatively set) now create
    an implicit anchor reference for that popover.

This new behavior was resolved in the [WHATWG/CSSWG/OpenUI joint task
force meeting on June 26, 2024](whatwg#9144 (comment)).

Closes whatwg#10442
Closes whatwg#10675
@mfreed7
Copy link
Contributor

mfreed7 commented Oct 29, 2024

I put up a spec PR containing the proposal detailed above: #10728

As mentioned above, it also includes a solution for #10442.

domenic pushed a commit to mfreed7/html that referenced this issue Nov 15, 2024
This includes two related changes:
 1. The `showPopover()` and `togglePopover()` methods now include an
    options bag that allows setting the popover invoker.
 2. Popover invokers (declaratively or imperatively set) now create
    an implicit anchor reference for that popover.

This new behavior was resolved in the [WHATWG/CSSWG/OpenUI joint task
force meeting on June 26, 2024](whatwg#9144 (comment)).

Closes whatwg#10442
Closes whatwg#10675
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration Better coordination across standards needed topic: popover The popover attribute and friends
5 participants