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

Popover does not know what triggered it #9111

Open
jpzwarte opened this issue Apr 3, 2023 · 25 comments
Open

Popover does not know what triggered it #9111

jpzwarte opened this issue Apr 3, 2023 · 25 comments
Labels
addition/proposal New features or enhancements topic: popover The popover attribute and friends

Comments

@jpzwarte
Copy link

jpzwarte commented Apr 3, 2023

As far as I can tell from reading the spec, a popover doesn't know which element invoked it (a "trigger element").

Is this something that should be added?

I suppose this could be useful if the CSS Anchor positioning hasn't shipped yet, but popovers have. You could then use @floating-ui/dom for anchoring the popover. But that all depends on the popover knowing what triggered it.
Another example would be to style the trigger element when the popover is open.

One solution would be to specify the trigger when calling showPopover/togglePopover?

element.showPopover(this); or element.showPopover({ trigger: this });

@jpzwarte
Copy link
Author

jpzwarte commented Apr 3, 2023

Afaict the CSS anchor proposal also doesn't support multiple popovertarget invokers: w3c/csswg-drafts#8671

@keithamus
Copy link
Contributor

keithamus commented Apr 3, 2023

I agree this could be a useful addition. I think extending the ToggleEvent to include a relatedTarget property, which may be null or may be the invoking button. This would allow implementations to adjust the anchor on beforetoggle based on the relatedTarget, which seems to support the use case.

(BTW needs labels topic: popover The popover attribute and friends addition/proposal New features or enhancements )

@jpzwarte
Copy link
Author

jpzwarte commented Apr 5, 2023

I'm just looking at @oddbird/popover-polyfill and I'm running into this issue. Afaics there is no way to know what triggered the popover, so I have no information I can use to position the popover with @floating-ui/dom.

@keithamus How can I get the right set of eyes on this issue? Any tips? Or just patience? :)

@jpzwarte
Copy link
Author

jpzwarte commented Apr 5, 2023

There is mention of an anchor attribute in the OpenUI research explainer: https://open-ui.org/components/popover.research.explainer/#anchoring

The only thing I can find in the spec is this line: "(e.g., a popover with an anchor attribute that points back to the same popover)"

A logical followup of that might be an anchorElement property?

@keithamus
Copy link
Contributor

@mfreed7 or @josepharhar might want to give this some attention.

@keithamus
Copy link
Contributor

There is mention of an anchor attribute in the OpenUI research explainer

CSS anchoring is a separate spec to popover. Here's an explainer: https://developer.chrome.com/blog/tether-elements-to-each-other-with-css-anchor-positioning/

@jpzwarte
Copy link
Author

jpzwarte commented Apr 5, 2023

CSS anchoring is a separate spec to popover. Here's an explainer: https://developer.chrome.com/blog/tether-elements-to-each-other-with-css-anchor-positioning/

I can't find any mention of an "attribute" here https://w3c.github.io/csswg-drafts/css-anchor-position/

The only thing specific to popover is this: "The Popover API, for example, defines an implicit anchor element for a popover—the element that the popover is attached to."

@keithamus
Copy link
Contributor

It looks like the anchor attribute wasn't pursued, in favour of using a name reference in CSS.

@annevk annevk added addition/proposal New features or enhancements topic: popover The popover attribute and friends labels Apr 6, 2023
@josepharhar
Copy link
Contributor

I am going to open a HTML spec PR for the anchor attribute soon.

If we added a new IDL like element.popoverInvoker which returned the element which is pointing to the popover element via popovertarget, would that be good enough?

I agree this could be a useful addition. I think extending the ToggleEvent to include a relatedTarget property, which may be null or may be the invoking button. This would allow implementations to adjust the anchor on beforetoggle based on the relatedTarget, which seems to support the use case.

Is having multiple potential invoking buttons and knowing which particular one opened it essential to this use case?

@keithamus
Copy link
Contributor

popoverInvoker doesn’t seem right to me because there can be many invokers (so which one would it return?), and it also wouldn’t be able to track the most recently used one. A relatedTarget on the event can also be null if the popover was opened via JS which can also be a useful signal).

I’m unsure if it’s @jpzwarte’s usecase but I can definitely imagine use cases of multiple buttons accessing a shared popover, and switching the anchor to the most currently used button.

@jpzwarte
Copy link
Author

I’m unsure if it’s @jpzwarte’s usecase but I can definitely imagine use cases of multiple buttons accessing a shared popover, and switching the anchor to the most currently used button.

Yes, that's my usecase.

I understand that the CSS Anchor Positioning will handle this on its own (a popover will have an implicit anchor based on popovertarget), but we also need something on the DOM side of things. For example to display data related to the popovertarget in the popover. For that to work, you need to know which button invoked the popover.

@josepharhar
Copy link
Contributor

I see, then adding relatedTarget to ToggleEvent would be good enough.

We could also make element.popoverInvoker only return the most recently used invoker, and set it to null while the popover is closed or when it is opened via script. If it is set prior to dispatching the beforetoggle event, then script could also access it when needed without the need to add it to ToggleEvent.

@josepharhar
Copy link
Contributor

Making element.popoverInvoker return the existing popover invoker would be quite easy

@jpzwarte
Copy link
Author

There's also the use case where you invoke the popover programmatically (showPopover/togglePopover). Do you want to pass the anchor as an argument? Or just set it before calling the function?

popover.anchorElement = this;
popover.showPopover();

@jpzwarte
Copy link
Author

So with CSS Anchor positioning, popoverInvoker doesn't necessarily have to be the same as anchorElement (assuming there will be an anchorElement property). So it probably doesn't make sense to set the popoverInvoker manually.

@jpzwarte
Copy link
Author

@josepharhar So since a popovertarget will automatically become the implicit anchor for the popover (see w3c/csswg-drafts#8671 (comment)), wouldn't it be logical for the element with the popovertarget attribute to automatically be set as popover.anchorElement (or whatever name get's decided on)?

That way you can just use anchorElement instead of having to define something like popoverInvoker? And by changing anchorElement, you would also change the implicit anchor in CSS? cc @tabatkins

@mfreed7
Copy link
Collaborator

mfreed7 commented May 4, 2023

@josepharhar So since a popovertarget will automatically become the implicit anchor for the popover (see w3c/csswg-drafts#8671 (comment)), wouldn't it be logical for the element with the popovertarget attribute to automatically be set as popover.anchorElement (or whatever name get's decided on)?

The spec PR for this is in progress, but the plan is definitely not to make an element with popovertarget an implicit anchor. The idea is to use an anchor attribute on the popover to connect it to an implicit anchor. And then popover.anchorElement will return that anchor element. I think this makes a lot more sense than trying to use popovertarget because as has been mentioned above, there can be many of those, or none. There are either zero or one anchor attributes.

@jpzwarte
Copy link
Author

jpzwarte commented May 21, 2023

I agree this could be a useful addition. I think extending the ToggleEvent to include a relatedTarget property, which may be null or may be the invoking button. This would allow implementations to adjust the anchor on beforetoggle based on the relatedTarget, which seems to support the use case.

I'm looking at @oddbird/popover-polyfill again: I would like to use the polyfill and then use @floating-ui/dom in beforetoggle to position the popover relative to the invoker. However, just using the beforetoggle event, I have no idea what the invoker element is.

@mfreed7 Forgetting anchor related behavior for now, could we at least move to have the relatedTarget property of the beforetoggle event point to the invoker element when used declaratively? And also perhaps an { invoker: element } optional parameter to the show/hide/toggle functions?

@jpzwarte
Copy link
Author

I went ahead and created a PR for the polyfill which adds this: oddbird/popover-polyfill#100

The only thing that I can see serious discussion on is that togglePopover(force?: boolean) changes to togglePopover(options?: { force?: boolean, invoker?: HTMLElement });. This would make it different from toggleAttribute.

@jpzwarte
Copy link
Author

The spec PR for this is in progress, but the plan is definitely not to make an element with popovertarget an implicit anchor. The idea is to use an anchor attribute on the popover to connect it to an implicit anchor. And then popover.anchorElement will return that anchor element. I think this makes a lot more sense than trying to use popovertarget because as has been mentioned above, there can be many of those, or none. There are either zero or one anchor attributes.

So in your scenario both the button and popover would have to have DOM ids so they can point to each other: popovertarget to #popover and anchor to #button.
So I see the use case where a popover only has 1 button associated with it pretty common. Perhaps the most used actually. Does it make sense then that you have these elements pointing at each other? Would it not be a better DX to have the invoking button automatically become the implicit anchor?

@mfreed7
Copy link
Collaborator

mfreed7 commented May 22, 2023

So in your scenario both the button and popover would have to have DOM ids so they can point to each other: popovertarget to #popover and anchor to #button. So I see the use case where a popover only has 1 button associated with it pretty common. Perhaps the most used actually. Does it make sense then that you have these elements pointing at each other? Would it not be a better DX to have the invoking button automatically become the implicit anchor?

This is an interesting point. I suppose it would be pretty clean to make the active invoker element (i.e. the one that invoked the currently-open popover) an implicit anchor element, only if the popover doesn't also have an anchor attribute.

@xiaochengh @josepharhar @nt1m @annevk any objections/thoughts?

@josepharhar
Copy link
Contributor

josepharhar commented Sep 4, 2023

This is an interesting point. I suppose it would be pretty clean to make the active invoker element (i.e. the one that invoked the currently-open popover) an implicit anchor element, only if the popover doesn't also have an anchor attribute.

So popovers would by default just get anchored to their opener buttons? Sounds good to me I guess. Would there be a way to opt out of this?

I could spec this after the anchor attribute gets specced: #9144

Note: this was also suggested here, which contains examples that show why it may be nice to have: #9311

@jpzwarte
Copy link
Author

jpzwarte commented Sep 4, 2023

So popovers would by default just get anchored to their opener buttons? Sounds good to me I guess. Would there be a way to opt out of this?

Any other anchor definition would cause you to opt out of this? So any CSS anchor,anchor attribute or anchorElement in JS?

@xiaochengh
Copy link
Contributor

Any other anchor definition would cause you to opt out of this? So any CSS anchor,anchor attribute or anchorElement in JS?

Yes, that should be the recommended ways to opt out.

@domenic
Copy link
Member

domenic commented Apr 1, 2024

I see there is a PR for this in #10236.

Why is the name something vague like "relatedTarget"? That's basically just saying "an EventTarget related to the event", when I think you could be more specific about what the relation actually is. E.g. triggerer or invoker.

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 topic: popover The popover attribute and friends
Development

No branches or pull requests

7 participants