html icon indicating copy to clipboard operation
html copied to clipboard

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

Open tabatkins opened this issue 1 year ago • 14 comments

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.

tabatkins avatar Oct 04 '24 20:10 tabatkins

cc @josepharhar @nt1m @whatwg/css

annevk avatar Oct 07 '24 06:10 annevk

+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?

mfreed7 avatar Oct 15 '24 21:10 mfreed7

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.

domenic avatar Oct 16 '24 01:10 domenic

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.

tabatkins avatar Oct 16 '24 19:10 tabatkins

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 avatar Oct 18 '24 17:10 lukewarlow

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).

lukewarlow avatar Oct 18 '24 17:10 lukewarlow

Ok, if I'm reading all of the comments correctly, it kind of sounds like rough consensus on the shape of https://github.com/whatwg/html/issues/10675#issuecomment-2415232403, 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 (https://github.com/whatwg/html/pull/9841).

mfreed7 avatar Oct 18 '24 19:10 mfreed7

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

lukewarlow avatar Oct 18 '24 21:10 lukewarlow

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?

mfreed7 avatar Oct 18 '24 22:10 mfreed7

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?

annevk avatar Oct 19 '24 12:10 annevk

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

lukewarlow avatar Oct 19 '24 12:10 lukewarlow

Sure, but that seems like a separate issue.

annevk avatar Oct 19 '24 13:10 annevk

I guess that would be https://github.com/whatwg/html/issues/10442

lukewarlow avatar Oct 19 '24 13:10 lukewarlow

It is a separate issue, yes.

tabatkins avatar Oct 20 '24 18:10 tabatkins

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 avatar Oct 21 '24 18:10 mfreed7

I put up a spec PR containing the proposal detailed above: https://github.com/whatwg/html/pull/10728

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

mfreed7 avatar Oct 29 '24 00:10 mfreed7