-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Popover does not know what triggered it #9111
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
Comments
Afaict the CSS anchor proposal also doesn't support multiple |
I agree this could be a useful addition. I think extending the (BTW needs labels
topic: popover
|
I'm just looking at @keithamus How can I get the right set of eyes on this issue? Any tips? Or just patience? :) |
There is mention of an 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 |
@mfreed7 or @josepharhar might want to give this some attention. |
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." |
It looks like the |
I am going to open a HTML spec PR for the anchor attribute soon. If we added a new IDL like
Is having multiple potential invoking buttons and knowing which particular one opened it essential to this use case? |
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 |
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. |
Making element.popoverInvoker return the existing popover invoker would be quite easy |
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(); |
So with CSS Anchor positioning, |
@josepharhar So since a That way you can just use |
The spec PR for this is in progress, but the plan is definitely not to make an element with |
I'm looking at @oddbird/popover-polyfill again: I would like to use the polyfill and then use @mfreed7 Forgetting |
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 |
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. |
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 @xiaochengh @josepharhar @nt1m @annevk any objections/thoughts? |
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 |
Any other anchor definition would cause you to opt out of this? So any CSS anchor, |
Yes, that should be the recommended ways to opt out. |
I see there is a PR for this in #10236. Why is the name something vague like "relatedTarget"? That's basically just saying "an |
Agreed with @domenic about making the property more specific. Consistency between The |
I just wanted to call out the connection between this issue and #10442, plus the resolutions made this morning at the form controls task force. Those two links, respectively:
Given that at least part of the request on this issue seemed related to anchor positioning - does the second bullet address the use case? If not, note that the first bullet (adding So I guess my hope would be that we don't need to add an API to retrieve the invoker element? |
Another use case for adding an invoker property to ToggleEvent would be to see which button a user clicked on to close a popover. I don't think that would be addressed by the anchoring related stuff. I'm also not sure if the new invokers/commands proposal adds this capability or not. |
The current limitation of the toggleEvent is preventing Consent Management Platform (CMP) like ours to consider adopting Dialog or Popover to display Consent Management UIs on the Web. Using these APIs can provide immediate benefits to CMP panels across the web, improving both accessibility and performance. However, there's a significant issue:
From my understanding, this limitation makes these APIs suitable only for simple use cases. If we had a way to identify the triggering button, it would enable more complex and flexible implementations. Additionally, leveraging toggle or beforetoggle listeners could help mitigate Interaction to Next Paint (INP) performance issues that modal interactions can introduce. Since Popover and Dialog also inherently improve accessibility, addressing this issue would be a win-win for many developers working on similar use cases. For now, a potential solution could be to use the Popover or Dialog APIs in combination with the click event and manage the modal state using the existing API. However, CMP JS SDK still need to ensure optimizations are made to prevent any INP issues. So, the key problem statements are:
|
Thanks for the comments. I'm supportive of adding something like |
This adds a new Element attribute to ToggleEvent called invoker. The ToggleEvent's invoker attribute is set to the element which invoked the element which the ToggleEvent is being fired on. Invokers can be set up as a parameter passed to element.showPopover(), an element with the popovertarget attribute, or an element with the commandfor attribute. Fixes whatwg#9111
I created a PR to add ToggleEvent.invoker here: #11186 |
Some thoughts and questions (I haven't read the actual PR yet)
|
The spec PR runs the retargeting algorithm to prevent leaking nodes in shadowroots.
I don't have a preference, it sounds like Element is better. The spec PR currently uses Element.
It is aimed at the declarative API, where the author wants to asynchronously know which button closed the popover via the toggle event.
Yes
I think that depends on what the source/invoker parameter for showPopover() was made for.
If you use a command invoker element to toggle the details element, then it would be set to that command invoker button. |
What about the summary element? Does that get returned or just null? |
Just null. Do you think it should return the summary element? I don't have an opinion on this. |
No strong opinions. I guess I'd think maybe it should be summary in the case that that's the source?
Does this mean we should add the ability to do |
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);
orelement.showPopover({ trigger: this });
The text was updated successfully, but these errors were encountered: